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.
Unfortunately they all suffer from the same fundamental issue - inconsistency. Likely the result of years of code patching, large teams, changing hands, or all of the above.
This creates a problem because we read code far more than we write code. As I read a new codebase these inconsistencies distract me from the true code. My focus shifts to the mundane of indentation and variable tracking instead of the important business logic.
Over the years, I find I boy scout a new codebase in the same way. I apply three simple practices to clean up the code and improve its readability.
To demonstrate, I’ll apply these to the following, real-world code I read just the other day.
function check($scp, $uid){
if (Auth::user()->hasRole('admin')){
return true;
}
else {
switch ($scp) {
case 'public':
return true;
break;
case 'private':
if (Auth::user()->id === $uid)
return true;
break;
default: return false;
}
return false;
}
}
Adopt a code style
I know I’m the 1,647th person to say, “format your code”. But it apparently still needs to be said. Nearly all of the codebases I’ve worked on have failed to adopt a code style. With the availability of powerful IDEs, pre-commit hooks, and CI pipelines it requires virtually no effort to format a codebase consistently.
If the goal is to improve code readability, then adopting a code style is the single, best way to do so. In the end, it doesn’t matter which code style you adopt. Only that you apply it consistently. Once you or your team agrees upon a code style, configure your IDE or find a tool to format the code automatically.
Since our code is PHP, I chosen to adopt the PSR-2 code style. I used PHP Code Beautifier within PHPCodeSniffer to automatically fix the code format.
Here's the same code after adopting a code style. The indentation allows us to see the structure of the code more easily.
function check($scp, $uid)
{
if (Auth::user()->hasRole('admin')) {
return true;
} else {
switch ($scp) {
case 'public':
return true;
break;
case 'private':
if (Auth::user()->id === $uid) {
return true;
}
break;
default:
return false;
}
return false;
}
}
Naming things properly clearly
Yes, something else you’ve heard plenty. I know naming things is hard. One of the reasons it’s hard is there are no clear rules about naming things. It’s all about context. And context changes frequently in code.
Use these contexts to draw out a name. Once you find a clear name, apply it to all contexts to link them together. This will create consistency and make it easier to follow a variable through the codebase.
Don't worry about strictly using traditional naming conventions. I often find codebases mix and match. A clear name is far more important than snake_case vs camelCase. Just apply it consistently within the current context.
If you’re stuck, use a temporary name and keep coding. I’ll often name things $bob or $whatever to avoid getting on stuck on a hard thing. Once I finish coding the rest, I go back and rename the variable. By then I have more context and have often found a clear name.
Clear names will help future readers understand this code more quickly. They don’t have to be perfect. The goal is to boost the signal for future readers. Maybe they can incrementally improve the naming with their afforded mental capacity.
After analyzing this code, I have more context to choose clearer names. Applying clear names not only improves readability, but boosts the context making the intent of the code easier to see.
function canView($scope, $owner_id)
{
if (Auth::user()->hasRole('admin')) {
return true;
} else {
switch ($scope) {
case 'public':
return true;
break;
case 'private':
if (Auth::user()->id === $owner_id) {
return true;
}
break;
default:
return false;
}
return false;
}
}
Avoid Nested Code
There are some hard rules regarding nested code. Many developers believe you should only allow one nesting level. In general, I tend to ignore rules with hard numbers. They feel so arbitrary given code is so fluid.
It's more that nested code is often unnecessary. I have seen the entire body of a function wrapped in an if. I have seen several layers of nesting. I have literally seen empty else blocks. Often adding guard clauses, inverting conditional logic, or leveraging return can remove the need to nest code.
In this case, I'll leverage the existing return statements and flip the switch to remove most of the nesting from the code.
function canView($scope, $owner_id)
{
if ($scope === 'public') {
return true;
}
if (Auth::user()->hasRole('admin')) {
return true;
}
if ($scope === 'private' && Auth::user()->id === $owner_id) {
return true;
}
return false;
}
In the end, coding is writing. As an author you have a responsibility to your readers. Maintaining a consistent style, vocabulary, and flow is the easiest way to ensure readability. Remove or change these and maintain readability you will not.
Want to see these practices in action? I'm hosting a free, one-hour workshop where I'll demo each of these practice, and more, through live coding. Sign up to secure your spot.
Oldest comments (66)
This is a beautiful post. Thanks. I'd keep this in mind.
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.
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.
Ah yes, I saw some 'nice' PHP codebases in my time too :)
Nice Article,
I think that example can be shorter (avoiding unnecessary declarations and/or sentences) also is mixing
snake casewithcamel caseand is not a good practice.Thanks for share!
I don't consider a single
returnwith a compound condition readable. In general, simply having less lines doesn't improve code.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 ;)
At least for this example, is a good option use a single
returnstatement, 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.
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...).
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.
That's interesting you consider it a code smell. I have some examples for future articles where I explore this raw boolean
returnstatements.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.
I agree with Marco, even we can do more functions:
That way is easy to read. Obviously, that can be refactored.
I'll explore this in a future post.
As a tangent, I never understood developers need to wrap the entire compound condition in parenthesis.
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->100% agree that conditional returns of booleans is a smell, generally one of the first things I look for in code review.
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.
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
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.
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.
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.
because i don't trust frameworks blindly:
taking advantages of boolean shortcuts : assuming that the owner of content is always allowed to view whatever $scope it is
Ungly code 😡
?
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.
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.
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.
You're right. I corrected the initial examples.
As far as the
nullcheck, it is indeed missing. But to your point, adding it would alter the existing behavior. I have left it off for simplicity.Simple and easy tips to create maintainable code.
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.
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.
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! :)
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.
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 evaluatingfalse, 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).