Last week, I ran into ‘PHP Dos and Don’ts aka Programmers I Don’t Like‘ on reddit’s PHP group. It features a list of 11 points that the author hates finding in PHP code. I am always keen on reading from such references : they always hold some some interesting insights, some tips and some more polemic aspects. This one didn’t disappoint, once again. It only lacked an way for enforcing the coding reference.
Enforcing coding reference
The most interesting point is that coding references are used to asses code. They may very well be automated, thanks to static analysis. I thought it was a good challenge for Exakat.
Here is the list. The original blog from Oliver Radwell has details, examples and explanations, so go and read it.
- Too many nested if statements
- Extra brackets and braces
- Unnecessary casting
- Useless checks
- Slow PHP built-in functions
- Long functions
- Too many function arguments
- Long lines
- Long if-else blocks
- Wrong function / class name casing
- Lack of coding standards
Review of the rules
‘Too many nested if statements’ is a good rule, where the hard part is to define the level of nesting. I know (at least) a company that doesn’t accept more than one level. Exakat reports any nesting over 2, and the blog example is quite extreme. This has to be defined.
The same problem comes with ‘Long if-else blocks’ (how many elseif before switching to switch?), ‘long functions’ (how many lines is a long function) or ‘too many functions arguments’. Each time, there is no general value for this limit, and it should be chosen project by project.
‘Unnecessary casting’ came as a surprise to me. I do agree that (string) trim() is useless. Since I experience it so rarely, it is not a concern for me. I can easily imagine it would if it was frequent. So, I created an exakat analysis for this : coupled with PHP manual, this is actually an easy static analysis.
‘Useless checks’ is also extracted from the PHP manual, and it an excellent recommendation : do no use isset() AND empty(), togther, they are redundant. Sometimes, one has to remember that the PHP Manual is full of good recommendations.
‘Extra bracket and braces’ also list classic ‘require(‘file.php’)’ and « $var » situations that are actually not even worth the talk about. Usually, everyone agrees they should not be. Fixing such issues in the code is actually faster than starting to explain it. Problem is, no one has the time to find them all. Although, when they are listed by a tool, they make a very relaxing coding time.
‘Lack of coding standard’ is the last of the list, and it is much more general than the previous ones. This is one that should be given to static analysis tool such as PHP Code Sniffer. There are too many rules to choose or omit. I agree, as a project’s coding convention.
All in all, this reference is quite objective. Each violation direct attention to lines in the code that needs it. This is very important : static analysis target exact location. Sometimes, the fix is as simple as the rule states, sometimes it is very different. In any case, leading me to such places is the most important result of a coding reference.
Exakat now has a ‘RadwellCode’ report, that covers all the points in the reference, except the coding convention, the long function and lots of arguments rules (for lack of target value). I ran it on my own code (php exakat report -p <project> -format RadwellCode -file <file>), and found 211 violations over 870 files and 137330 LOC. Most of them are ‘Too many nested if statements’. Here are a few other reports :
- Zend Framework 1.12 : 899
- openclinic : 716
- wordpress : 693
- openconf : 450
- phpmyadmin : 311
- SPIP : 273
- Exakat : 211
- composer : 39
- pomm : 38
- in2pire : 25
- Sylius : 12
- Behat : 8
- polr : 7
- decision-tree : 1
- ofxparser : 0
Based on the short and arbitrary list of projects, there are indeed some projects that follow the Radwell coding reference, albeit unwillingly. It seems to be a good coding reference to differentiate projects.
Coding reference is always yours
A coding reference, like this one, is good to have. It helps guide coding, and assess external contributions. After the reference, it is important to have an inforcement tool that checks repositories without wasting developer’s time : this is were static analysis can help. Download Exakat and run the report on your code.
In terms of rules, it is obvious that my code don’t comply with Radwell’s reference. I do have my own, and it is not important to know which is better. I did learn from Oliver Radwell, and that’s the important part.