I'm from Aguascalientes, Mexico! now based on New York. Most of my experience is related to code websites and applications, using JavaScript stack-based.
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.
functioncan_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
I've been a professional C, Perl, PHP and Python developer.
I'm an ex-sysadmin from the late 20th century.
These days I do more Javascript and CSS and whatnot, and promote UX and accessibility.
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.
I'm from Aguascalientes, Mexico! now based on New York. Most of my experience is related to code websites and applications, using JavaScript stack-based.
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:
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.
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.
I build things with my hands. The human behind Shift - https://laravelshift.com, master of Git - https://gettinggit.com, and author of "BaseCode" - https://basecodefieldguide.com
I'm from Aguascalientes, Mexico! now based on New York. Most of my experience is related to code websites and applications, using JavaScript stack-based.
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.
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
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.
I build things with my hands. The human behind Shift - https://laravelshift.com, master of Git - https://gettinggit.com, and author of "BaseCode" - https://basecodefieldguide.com
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.
I build things with my hands. The human behind Shift - https://laravelshift.com, master of Git - https://gettinggit.com, and author of "BaseCode" - https://basecodefieldguide.com
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->
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 ;)
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.
I've been a professional C, Perl, PHP and Python developer.
I'm an ex-sysadmin from the late 20th century.
These days I do more Javascript and CSS and whatnot, and promote UX and accessibility.
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.
For further actions, you may consider blocking this person and/or reporting abuse
We're a place where coders share, stay up-to-date and grow their careers.
Nice Article,
I think that example can be shorter (avoiding unnecessary declarations and/or sentences) also is mixing
snake case
withcamel case
and is not a good practice.Thanks for share!
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
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.
Ungly code 😡
?
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.
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.
I don't consider a single
return
with a compound condition readable. In general, simply having less lines doesn't improve code.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.
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...).
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
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
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.
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.
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 ;)
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.
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.