PHP Code review checklist
I love to do code reviews because it give me chance to see how other people write code and improve mine also. I have seen many people who are afraid of doing code review. Which made think of creating a code review checklist for php. Please note this is not full checklist for code review and following all the conditions in this will not end up in a great code. But following this will end up in code that can be maintained by others in the later stage of code development.
- No syntax/runtime errors and warnings in the code
- No deprecated functions in the code – You can get the list of deprecated function in php3 from the url http://nl.php.net/manual/en/migration53.deprecated.php (Thanks for Frank for sharing me this link)
- There should be an explanation for any code that is commented out. “Dead Code” should be removed. If it is a temporary hack, it should be identified as such.
- Check if code has file/class/function level comments. And each of this comments should explain what the file/class/function is doing inside it.
- Check that each function is doing only a single thing. That is never function named createCustomer should delete the existing customer and create it again
- A method should not be larger than 40 lines of code. The basic idea is always a function should be viewable in single screen with out scrolling.
- Always try to initialize the variable before using that in a function.
- No public class attributes.
- Always try to use constants in the left hand side of the comparison. That is instead of doing if ($variable == 2) always use if (2 == $variable) because this will help to identify the errors in the earlier stage of development even if we miss and “=” from that statement
- Never ever mix the php code and template (view). In ideal condition a view should not contain any logic.
- Always try using single quote ( ‘ ) when working with the php string because it has a minor performance improvement when compared to the double quotes ( ” ). The difference come from the factor that ( ” ) can be used to build a string with variables inside it.
- There should be no catch blocks which catches an exception and throw that again. This is because exception can bubble up to the top function call stack automatically.
- Never ever throw a general exception with a custom message. Always try to create a custom exception so that all the other code can handle this situation correctly.
- Make sure that the top most code handles all the exceptions correctly shows correct / understandable error messages to the user
- In the case of a system crash never ever put up the error information that expose the internal behavior of the system.
- Make sure that a proper and uniform coding standard is followed through out the files. I have seen many projects where people mix up the coding style. For php I usually use PHP Code Sniffer for validating the coding style.
- Try using the tools like PMD to identify possible bug, dead code, over complicated expression, suboptimal code, duplicate code.
- No magic numbers. There should be no magic numbers like 6,10 etc… any numbers like this should be define as a constant. And Constants should be well commented about the purpose.
- Never allow bad code with some good comments
- Always try to have unit test for the new piece of code. In ideal condition we should have 100% unit test coverage
- Never allow unit test that are written to show 100% coverage and doesn’t do anything that unit test is supposed to do. I have seen unit test code which just call the function to get 100% coverage and doesn’t have any assertions.
- Always commit /Rollback database transaction at the earliest. Keep the database transaction short as possible.
- Always have an eye on the recursive functions.
- Optimizations may often make code harder to read and more likely to contain bugs. Such optimizations should be avoided unless a need has been identified. Has the code been profiled? Check if any over optimization has led to functionality disappearing.
- As a reviewer, you should understand the code. If you don’t, the review may not be complete, or the code may not be well commented.
- Lastly, when executed, the code should do what it is supposed to.
23 Comments
Post a comment
Great list. Thanks.
I would suggest breaking this out into syntax types. For example a review of xhtml would be different than php or mysql. This because you will be look for different things such as semantics, execution speeds. And or normalization.
Really a good suggestion, Will look into it
Gud info.. keep mooving..
Good..
PMD is for Java right ? There is another one by Sebastian Bergmann of phpunit
https://github.com/sebastianbergmann/phpdcd
I think 11 should be expanded a bit: you should prefer ‘, unless using ” makes it more readable.
For example don’t do this: ‘hi \’hello\’ hey’, but instead do “hi ‘hello’ hey”. The same can apply sometimes when outputting variables in strings.
Nice list.
PMD is a java tool, did you mean PHPMD ?
You can also have a look at PHPCheckstyle (shameful link)
http://developer.spikesource.com/wiki/index.php?title=Projects:phpcheckstyleDocs
Hi Tchule
PMD(http://pmd.sourceforge.net/cpd.html) can handle PHP to an extend.
But the suggested PHPMD (http://phpmd.org/) can handle the situation much better than PMD. Really a good suggestion.
Thank you
7) Regarding PHP, this looks kind of confusing. I would recommend that code should be E_STRICT – safe but not to have $myVar; declarations on top of your function.
9) These so called “YODA” Conditions are extremely hard to understand when you have stuff like if (2 = $var) … I would not recommend using them. Instead, if you use PHP Code Sniffer, you are warned that an assignment in if (…) is not a good practice. Just in case you really forget one “=”.
11) Why forbid the convenience of using “$stuff in double quoted $strings”? Such micro optimizations (only use single quotes…) don’t make a remarkable difference in performance.
12) There are situations where it definitely makes sense to catch an Exception and throw it again.
Nevertheless, some good points mentioned!
Decent article, however I agree with David (above) on point 9; Yoda conditions are much more difficult to read and personally, I think they’re downright ugly.
>> 9) if (2 == $variable)…
While I see nothing wrong with this practice, I do believe it is unnecessary. Unit tests render this type of safety-net useless. It effectively undoes any espressiveness the code had for no gain.
>> 11) Always try using single quote ( ‘ )
Always a quite a strong word. Use discretion and use the most appropriate tool for the job. Sometimes ‘ is the right thing, sometimes “” is what you want.
>> 12) There should be no catch blocks which catches an exception and throw that again. This is because exception can bubble up to the top function call stack automatically.
It is true that exceptions can bubble up; however, that isn’t always the correct solution. If something can be caught and handled, it should be caught and handled, otherwise, it is fine to allow your global handler to log the problem. It is also good to catch a generic exception (like library code) then throw a more specific exception injecting the generic exception into the $previous argument of the constructor. Given that an exception is a normal class, you can also provide more fields and methods to provide more detailed info. The $previous exception is there for inspection so it is apparent where the problem originated.
I also wish to signal for the RSS feeds. Thank you as soon as once more and maintain up the great do the job!
I know this is really boring and you are skipping to the next comment, but I just wanted to throw you a big thanks – you cleared up some things for me!
Is there anymore information you can give on this subject. It answers a lot of my questions but there is still more info I need. I will drop you an email if I can find it. Never mind I will just use the contact form. Hopefully you can help me further.
- Robson
I’m still learning from you, but I’m making my way to the top as well. I absolutely liked reading all that is posted on your blog.Keep the posts coming. I loved it
Keep working ,terrific job!
Thanks, great list.
Regular maintenance or updates would be really nice.
I’ve got one for #2. All deprecated PHP functions can be found at php.net. (e.g. http://nl.php.net/manual/en/migration53.deprecated.php)
cheers Frank
Thanks Frank for sharing. I have updated the blog
I’m learning PHP and find this information useful. I especially like the comment from Wil Moore, It compliments your piece well.
Excellent Article. Thanks for sharing it with us.