DEV Community

Cover image for Writing Clean Code

Writing Clean Code

Jason McCreary on August 14, 2017

I recently started a new job. With every new job comes a new codebase. This is probably my twentieth job. So I've seen a lot of codebases. Unfortu...
Collapse
 
mdor profile image
Marco Antonio Dominguez • Edited

Nice Article,

I think that example can be shorter (avoiding unnecessary declarations and/or sentences) also is mixing snake case with camel case and is not a good practice.

function canView($scope, $ownerId) {
  return ($scope === 'public' ||
     Auth::user()->hasRole('admin') ||
     ($scope === 'private' && Auth::user()->id === $ownerId));
}

Thanks for share!

Collapse
 
gonedark profile image
Jason McCreary • Edited

I don't consider a single return with a compound condition readable. In general, simply having less lines doesn't improve code.

Collapse
 
conaltuohy profile image
Conal Tuohy

I agree with Marco. To me "if (boolean-expression) return true" is a definite code smell. I would format the compound boolean expression so that each part has a line of its own though.

Thread Thread
 
gonedark profile image
Jason McCreary

That's interesting you consider it a code smell. I have some examples for future articles where I explore this raw boolean return statements.

In this case, I still wouldn't pack them down into one. At least not without some intermediate steps to improve the readability beyond just line breaks.

Thread Thread
 
algusdark profile image
Carlos.js

I agree with Marco, even we can do more functions:

function canView($scope, $ownerId) {
  return (hasScope('public') || isAdmin() || (hasScope('private') && isOwner($ownerId));
}

That way is easy to read. Obviously, that can be refactored.

Thread Thread
 
gonedark profile image
Jason McCreary

I'll explore this in a future post.

As a tangent, I never understood developers need to wrap the entire compound condition in parenthesis.

Thread Thread
 
kodikos profile image
Jodi Winters

I'd agree with carlos.js' solution, albeit each of the conditions being on separate lines. This supports the functions being just return statements, and giving clearer meaning (context) to the conditions you're checking. Also, it completely eliminates the branch logic in a function designed purely for information, a good nod to readable code.
The wrapping parenthesis must be a typo as there's no corresponding closing outer parenthesis. But that is an example of why you shouldn't wrap the condition in parentheses. There is another issue with Carlos.js' example, $scope is no longer used which means this must be a function in a class which means all those nice function calls are missing $this->

Thread Thread
 
joegaudet profile image
Joe Gaudet

100% agree that conditional returns of booleans is a smell, generally one of the first things I look for in code review.

Collapse
 
mdor profile image
Marco Antonio Dominguez

At least for this example, is a good option use a single return statement, in case that we have more comparisons or complex ones would be better split on several statements.

The code is readable also I checked the code using these pages: phpcodechecker.com/ and piliapp.com/php-syntax-check/, it doesn't have issues.

Thread Thread
 
cyr1l profile image
cyr1l

Hi,
I did not find a php checker that indicates some bad practices in the code contrary to those of the javascript language (esprima.org/demo/validate.html, extendsclass.com/javascript-fiddle...).

Collapse
 
mariordev profile image
Mario

Exactly, packing conditionals into a single statement just to reduce the number of lines doesn't necessarily make the code more readable. I avoid compound conditions as much as possible. If I have to absolutely use them, I try to wrap them in a function with a name that better documents its purpose. Thanks for the great article.

Thread Thread
 
danidee10 profile image
Osaetin Daniel • Edited

I agree with you. I also avoid compound conditions as much as possible. They can easily introduce silent bugs into your code and there's the mental Overhead that comes with them When reading the code Compared to simple if Statements

Also the code with if statements is more maintainable and open to new features/improvements since each decision has its own block. For example you could easily add a Log message for each condition... Can't say the same for a single return statement.

Any programmer. (Even a newbie) can easily grok the if statements in one glance

Collapse
 
_andys8 profile image
Andy

In general concise code can be unreadable. But this example has the same content as the code in the blog post. There are more lines, brackets and boilerplate, but there is no additional information.

And I'd prefer getting rid of mixed cases, too. "Mix cases but be consistent" is not an option for me ;)

Collapse
 
ilmam profile image
ilmam

For me, I strongly agree with you, having 15 clean lines is much better than one condensed line. Consider someone new reading the both codebases: one look at the 15 lines will be enough to read and understand it. While they will struggle to evaluate all the booleans in their heads in the second option. Most probably will take longer to grasp.

Collapse
 
moopet profile image
Ben Sinclair

I think that all these attempts to reduce the character count reduce the readability.
Making a compound like this, split across several lines, means that the function as a whole will take someone longer to scan.

There is no benefit to it.

Collapse
 
dvanherten profile image
Dave van Herten

The initial example really separates the 3 ways you can "view".

The shorter example still does that, but the condensing makes it harder to separate the content IMO. For instance if the 3rd line was a bit longer that the if check needed two lines, it might require some more mental power to keep things separate in your head. First way wouldn't have had this problem as much.

That said, to support at a glance, what does this do, I may opt to pull the conditions into their own functions such that the check becomes:

return IsPublic(...) || IsAdmin(...) || IsOwner(...);

At a glance I can tell what my conditions are for when I canView. It captures essentially the requirements in very plain english. If I care about how those rules are defined I can look into each one explicitly to find what I might be looking for.

Collapse
 
moopet profile image
Ben Sinclair • Edited

Put it this way - I can read and understand the verbose example on my phone. Your condensed version takes longer for me to parse, partly because it's too dense to easily separate out the parenthesised conditions and partly because I have to scroll horizontally back and forth to even read it.

Collapse
 
cmilr profile image
Cary Miller

To my eyes this is totally unreadable. The whole point was to make it easy to read and comprehend, not to just make it short for the sake of shortness.

Collapse
 
dallgoot profile image
dallgoot • Edited

because i don't trust frameworks blindly:

function can_view(string $scope, int $owner_id) : bool {
    $user = Auth::user();
    if(!$user || !method_exists($user, "hasRole") || !property_exists($user, "id")){
        throw "Error : Current User state is undetermined"
    }

    return $scope === 'public' || $user->id === $owner_id || $user->hasRole('admin');
}

taking advantages of boolean shortcuts : assuming that the owner of content is always allowed to view whatever $scope it is

Collapse
 
raffacabofrio profile image
raffacabofrio

Ungly code 😡

Collapse
 
mdor profile image
Marco Antonio Dominguez

?

Collapse
 
kayis profile image
K

You are totally right!

Coding style is a pretty big thing, but most developers get this right in the first year. Some languages even come with their own tools to enforce style guides. Using Pettier in my JavaScript projects really helped me to stop worrying.

Naming things is generally a bit harder, I think, but many developers really use bad names for things. Strange abbreviations or single character variables just because they think a 10 char name would be ugly... because they want to reduce package size, etc. So while I think naming is sometimes really hard, 80% of all code can really benefit from simply writing out everything.

Avoiding nesting is really important and often not followed by even experienced developers. Probably because, as you get better, you think you can handle things better, haha. I think it should be kept in mind over the whole process, not only for conditionals or functions, but also for objects and general software architecture.

Collapse
 
billiegoose profile image
Billie Hilton

Coding style is a pretty big thing, but most developers get this right in the first year.

I think this depends heavily on company culture. I know plenty of PHP developers who have been at it for well over a decade who don't take coding style seriously.

Collapse
 
kayis profile image
K

Ah yes, I saw some 'nice' PHP codebases in my time too :)

Collapse
 
slifin profile image
Adrian Smith • Edited
function can_view(string $scope, int $owner_id) : bool {

    $is_public = $scope === 'public';
    $user_is_admin = Auth::user()->hasRole('admin');
    $is_private = $scope === 'private';
    $is_owner = Auth::user()->id === $owner_id;

    $current_user_can_view = $is_public || $user_is_admin || ($is_private && $is_owner);
    return $current_user_can_view;
}

Inline conditions inside if statements discard the opportunity for a name, as a maintainer I care more about the intent of the code at the time of creation than its implementation, please keep telling me intent through names at every opportunity

By breaking everything down I wonder if $is_private is even required?

Collapse
 
rafaacioly profile image
Rafael Acioly

Hi Jason, great article congratulations.
By the way, i did not see anything commenting about this so there it go, you can clean even more replacing the 3 ifs with a single switch using multiple case validation like this;

switch ($scope) {
    case 'public':
    case 'private' && Auth::user()->id === $owner_id:
    case Auth::user()->hasRole('admin'):
        return true;
    default:
        return false;
}


`

let me know what you think.

Collapse
 
gonedark profile image
Jason McCreary

The original code (from Part 1) was a switch statement. Personally, I don't find switch statements very readable. Especially with conditions in the case.

Collapse
 
rafaacioly profile image
Rafael Acioly

Cool!
I prefer "switch" because i like to write return true only one time. 😁

Collapse
 
antonfrattaroli profile image
Anton Frattaroli • Edited

If you use C#, you can add the StyleCop.Analyzers nuget package and it highlights inconsistencies. It also comes with code fixes that allow you to fix all instances of a violation in the entire solution with one click.

Then there's static code analysis. Between the two, all that is really left to the programmer is models, appropriate layering of your code, security, performance and business logic... well... on the backend anyway.

Collapse
 
davidhaile profile image
David Haile

This article brings up one of my Pet Peeves - inheriting or resurrecting projects that have very poor coding style and confusing or non-existent naming conventions. I quit one job because of it, and have completely reorganized a project in the 2nd job. At some point you just have to come to terms with either making wholesale changes to make it readable or abandoning the project because it can never be maintained. If a the sourcecode doesn't have a good, solid style and use understandable naming conventions, the code is crap, full of bugs, and a maintenance nightmare. Always!

Collapse
 
ryanwinchester profile image
Ryan Winchester • Edited

Nice job, I've been liking these kind of posts you've been doing lately, and your final function is way better.

However, I feel like playing along, too. How about this? :)

  <?php

  function canView(string $scope, int $owner_id): bool
  {
      return $scope === 'public' || $this->userCanView(Auth::user(), $owner_id);
  }

  function userCanView(User $user, int $owner_id): bool
  {
      return $user->hasRole('admin') || $user->id === $owner_id;
  }

I don't know the context, but I'm pretty sure it doesn't even matter if the $scope is 'private', if the current user is the owner.

Collapse
 
haraldkampen profile image
Harald Kampen • Edited

I don't think a return before the function ends is good code. A function or method should only have one return.

function canView($scope, $ownerId) {
$canView = $scope === 'public';
if (! $canView) {
$canView = Auth::user()->hasRole('admin');
}
if (! $canView) {
$canView = $scope === 'private' && Auth::user()->id === $ownerId;
}
return $canView;
}

Collapse
 
gonedark profile image
Jason McCreary

As noted in the article, I don't believe in only one return. I admit there are trade-offs on both sides, in this case you are tracking $canView which carries it's own complexity.

Collapse
 
haraldkampen profile image
Harald Kampen

BTW: can somebody explain which keyword are starting and ending code snippets?

Collapse
 
gregorgonzalez profile image
Gregor Gonzalez

Yes! I Totally agree with you. The code you provide is a perfect example.

I also consider variables should be relevant, short doesn't mean better, even with database tables and columns. On my last work they use "a_1, a_2, b_c" for clientes names!! D:

always consider that at some point on time you will forget about the code and you will need to back and read it again, so keep it clear. Also for new co-workers.

Collapse
 
codemouse92 profile image
Jason C. McDonald • Edited

Excellent article, and very good points!

In some languages, the nesting is about more than just style. Branch prediction and instruction cache misses (from jump instructions) are very real things, and they can completely tank performance when structured incorrectly.

When possible, I prefer to use conditionals to catch off-cases, rather than the most frequent case, simply because that takes the best advantage of the performance gains from the branch prediction. The computer can get used to the condition evaluating false, which it will most of the time; if that prediction misses, I'm already going to have to deal with additional jump instructions and whatnot to handle the off-case, so performance can reasonably be considered (somewhat) shot at that point anyway. :wink:

On an unrelated subject, I also advocate commenting intent as a part of clean code. We should always be able to tell what the code does, but why is not something the code naturally expresses. (I discuss that more in my article, Your Project Isn't Done Yet).

Collapse
 
mortoray profile image
edA‑qa mort‑ora‑y

I disagree that formatting, and formatting consistency, is so important. I've seen enough perfectly formatted garbage code and poorly formatted clean code to doubt the major significance of formatting. I will agree that it makes legible code even easier to read, but I disagree that on its own it brings anything.

Too often I see projects start to go overboard with their style guidelines. They can get in the way and even consume a project.

Defintily good names help. I don't think we should blame people for having bad names though. It's a very contextual thing. While working on a piece of code one name seems totally appropriate, but later, when missing context, it seems a bit lacking. But we can't just add full context to all names, we have to rely on people understanding the surrounding code. Otherwise we'd end up with ridiculously long names for everything -- and they still wouldn't be enough.

Avoiding nesting code is good. But alas, there are a camp of people that are, for some reason, utterly opposed to early returns and break statements. Make sure you show that piece of code during an interview to avoid hiring those people! :)

Collapse
 
joeymink profile image
Joey Mink

Consistently formatted code is surely ideal. But previous efforts I've been a part of have suffered from some combination of the following:

  1. Lack of interest from all members of the team
  2. Differing opinions on what looks/reads best
  3. Too much legacy code to visit

So I've basically landed here: to all who are driven to format new or legacy code, go nuts! People tend to fall in line if there's already momentum.

However: make any formatting-only changes In. Their. Own. Commit. If someone has to review a commit where the whole file changed to figure out why a bug was introduced, that will be a huge fail...

Collapse
 
gonedark profile image
Jason McCreary

Agreed about momentum and separate commit.

However, most of your points are moot when automating this process with a formatter/linter as suggested.

Collapse
 
joeymink profile image
Joey Mink

If it's a new code base or new file, agreed. But what about modifying a legacy file to introduce/change actual functionality -- the automated formatter/linter may obscure the actual changes with a complete reformat, correct?

Thread Thread
 
gonedark profile image
Jason McCreary

To your point, do it in a separate commit. Run the formatter/linter one time manually across the entire codebase. This will provide a basis (momentum) and from then on only changed files need to be formatted/linted (which could be automated).

Collapse
 
Sloan, the sloth mascot
Comment deleted
Collapse
 
gonedark profile image
Jason McCreary

Seems like you don't agree with any of the post then…

Collapse
 
oleksiirybin profile image
oleksii-rybin

I agree with the idea of clean code in general. And I agree that code example at the end of an article became much better than it was before. And I just added some more things I assume as "clean code" principles.

Thread Thread
 
gonedark profile image
Jason McCreary

Fair. Nevertheless, they contradict mine. One of the contributing factors of messy code is that everyone disagrees on "clean code".

For clarity, I don't believe in a having single return statement. And there is nothing wrong with temporarily naming a variable something obviously incomplete in the spirit of maintaining momentum.

Collapse
 
ruth20 profile image
Ruthran

you're right, this makes the programmer to code more and even make them fall in love and easy to spot the error and the understanding of the code will be ease for other programmer to testify it ! .

And naming the variable is toughest job in the world .ahaha. seriously though ,many are lazy to naming the lengthy variable. specially in upperCamel letter . I practiced to naming the variable in upperCamel even-though it was long ,its hard to follow first after being practiced with that now its easy to naming the variable ,I don't worry how much the length of the variable name because u wont forgot during the middle of the code ,I dont need to go check the top of the code to refer our variable name.

Collapse
 
nickspag1 profile image
Nick Spagnola

To get too granular for anyone's good- can you say why the break in the private case of the switch is indented but not in the public case? Guessing it has something to do with the one liner vs the logic, but curious to see a professional's reasoning?

Collapse
 
emandtf profile image
emandtf

Sorry but your final "avoid nested code" alters the original method:
1) "owner_id" was never compared to anything in the original code, but you compared with "user()->id" field. In the original code it was just checked to be Valid/NonNull.
2) you never checked if "Auth::user()" returns a NOT NULL value before use it, so your code could give a NullPointer Exception if it returns NULL and you directly access to "->id" field.

Collapse
 
gonedark profile image
Jason McCreary

You're right. I corrected the initial examples.

As far as the null check, it is indeed missing. But to your point, adding it would alter the existing behavior. I have left it off for simplicity.

Collapse
 
espoir profile image
Espoir Murhabazi

1 week I read something about clean code in python , and the PEP8 style guide , i learn about it and start practice now i'm using Pylinter for sublime text and found it very cool it help me to write clean and readable code and discover that the code i have wrote before was very dirty and ugly,.....

Collapse
 
oleksiirybin profile image
oleksii-rybin

I agree that writing a clean code is necessary. But I have something to add:

1) Decide whether you're using a camel case or not and stick to it, don't mix it up (I personally prefer using camel case in my projects).
2) Only one return statement per function - makes it much easier to read and debug, less likely to have hidden bugs.
3) I don't agree that naming a variable $bob or $whatever to rename it later is a good solution, you could easily forget to do this or be to "tired and lazy" after finishing with the feature. I always name variables clearly from the moment I create it, yes, it could be not easy at first, but you get used to it and it becomes automatic. And if later you decide that you don't like the name, you could always change it, but even if you don't - it still would be meaningful.
4) Also, it doesn't feel right when I see OOP mixed up with functional programming.

Here's my variation of this code:

Collapse
 
danieldubovski profile image
Daniel Dubovski

Couldn't agree more. Was very close to writing my own article emphasizing the need to be clear when writing code.
To be more specific, the guidelines you iterated, help read code as if it was a story, or a self explanatory recipe. Bad naming often helps find bad code separation patterns, while nesting tends to make code hard to follow. Very good post!

Collapse
 
juankoss profile image
Juan Koss

I think that example can be shorter too!
function canView($scope, $ownerId) {
return ($scope === 'public' ||
Auth::user()->hasRole('admin') ||
($scope === 'private' && Auth::user()->id === $ownerId));
}

Collapse
 
gonedark profile image
Jason McCreary

As noted in other comments, it's not about shorter, it's about more readable.

Collapse
 
programmingdive profile image
Programming Dive

I think in the first image you first check for authorization of the user and then check the scope. This way we will avoid code execution for unauthorized user.

Collapse
 
onyekaijeh profile image
Ijeh Onyeka

This is a beautiful post. Thanks. I'd keep this in mind.

Collapse
 
nicolasvanhecke profile image
NicolasVanhecke

Great read. This might push me over the edge to actually start doing it. Been thinking about it for a while to have 1 style.

Collapse
 
iamgoangle profile image
Teerapong Singthong

I would say that we should think about 'write code as a document'

Collapse
 
victoryshoe profile image
Victor Shi

Great article, love that you went step by step through your proposed process and showed each small change and how much individual impact it had.

Collapse
 
gonedark profile image
Jason McCreary

Thanks. Definitely check out BaseCode as it has over 40 such examples.

Collapse
 
raffacabofrio profile image
raffacabofrio

Great article. 👍

Collapse
 
krishnasagar14 profile image
krishnasagar14

Good article. I also experienced the same and I believed in "Naming things clearly". That's where I am find this article relevant.

Collapse
 
asynccrazy profile image
Sumant H Natkar

Simple and easy tips to create maintainable code.

Collapse
 
vasilvestre profile image
Valentin Silvestre

The first one make me want to cry.. Great tips. Thanks !

Collapse
 
sarbash profile image
Sergey Sarbash • Edited

ATM I code in Go. It has an awesome formatting tool named gofmt. It allows me to not worry about formatting completely. It does it's job pretty well.
Ah, yes. The article is nice, thank you.

Collapse
 
gonedark profile image
Jason McCreary • Edited

Exactly. Many languages have a built in coding style, such as Python and Go. While this may seem rigid at first, it improves developer focus as it removes the distraction to meticulously format code.