Skip to content

December 4, 2010

24

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.

  1. No syntax/runtime errors and warnings in the code
  2. 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)
  3. 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.
  4. 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.
  5. 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
  6. 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.
  7. Always try to initialize the variable before using that in a function.
  8. No public class attributes.
  9. 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
  10. Never ever mix the php code and template (view). In ideal condition a view should not contain any logic.
  11. 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.
  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.
  13. 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.
  14. Make sure that the top most code handles all the exceptions correctly shows correct / understandable error messages to the user
  15. In the case of a system crash never ever put up the error information that expose the internal  behavior of the system.
  16. 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.
  17. Try using the tools like PMD to identify possible bug, dead code, over complicated expression, suboptimal code, duplicate code.
  18. 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.
  19. Never allow bad code with some good comments
  20. Always try to have unit test for the new piece of code. In ideal condition we should have 100% unit test coverage
  21. 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.
  22. Always commit /Rollback database transaction at the earliest. Keep the database transaction short as possible.
  23. Always have an eye on the recursive functions.
  24. 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.
  25. 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.
  26. Lastly, when executed, the code should do what it is supposed to.
Read more from PHP
24 Comments Post a comment
  1. Dec 4 2010

    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.

    Reply
  2. Dec 4 2010

    Gud info.. keep mooving..

    Reply
  3. Anish Mohan
    Dec 4 2010

    Good..

    Reply
  4. Dec 6 2010

    PMD is for Java right ? There is another one by Sebastian Bergmann of phpunit ;) https://github.com/sebastianbergmann/phpdcd

    Reply
  5. Dec 7 2010

    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.

    Reply
  6. Tchule
    Dec 7 2010

    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

    Reply
  7. Dec 7 2010

    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!

    Reply
  8. Nils Luxton
    Dec 8 2010

    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.

    Reply
  9. Dec 9 2010

    >> 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.

    Reply
  10. Dec 14 2010

    I also wish to signal for the RSS feeds. Thank you as soon as once more and maintain up the great do the job!

    Reply
  11. Dec 14 2010

    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!

    Reply
  12. Dec 16 2010

    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

    Reply
  13. Dec 16 2010

    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

    Reply
  14. Dec 19 2010

    Keep working ,terrific job!

    Reply
  15. Aug 1 2011

    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

    Reply
    • Aug 2 2011

      Thanks Frank for sharing. I have updated the blog

      Reply
  16. Aug 31 2011

    I’m learning PHP and find this information useful. I especially like the comment from Wil Moore, It compliments your piece well.

    Reply
  17. Jan 4 2012

    Excellent Article. Thanks for sharing it with us.

    Reply
  18. Aug 28 2012

    Hi,
    Its really helping me. thank you very much.

    Thanks.

    Reply

Trackbacks & Pingbacks

  1. Tweets that mention PHP Code review checklist | Jose Antony -- Topsy.com
  2. Best-of-the-Web 2 | davblog: webdev and stuff
  3. PHP für WordPress: Den PHP-Code von WordPress verstehen und anpassen: Themes und Templates selbst entwickeln Reviews - Wordpress Video Tutorials

Share your thoughts, post a comment.

(required)
(required)

Note: HTML is allowed. Your email address will never be published.

Subscribe to comments