The code reviews

A code review aims to check all changes in order to improve the code general quality. The goal is to make comments or observations linked with :

• Integrated features : do they respect the product specifications ?
• Omissions : an element to add of useless code to delete
• Technical choices : Advices about other options, code complexity, etc.
• Specific project guidelines

For visual modifications like spacing, variables names... it is recommended to have automatic comments or corrections with tools like phpcs or php-cs-fixer.

When to do a code review ?

We generally do code reviews at the end of a feature development or a patch, but we can do it as soon as the code is shared.

The most famous versioning platforms directly integrate code reviews systems. You can find it in Github, Gitlab, Bitbucket, Azure DevOps, Phabricator. If your system does not allow it, you can use tools like gerrit that can be integrated directly into git.

For more informations on this subject, you can see thee documents Google put online about their engineers practices.

The interest of a checklist

Sometimes in your team, you will notice that some errors will come back often. It could be useful in this case to create a checklist every reviewer could use to be sure to don't forget anything.

For the start, you can find below the checklist the New York University uses in its Effective Code Review et Software Carpentry classes, that are projects made to learn the good practices in programming.

General

• Does the code works? Is it working the way it should, is the logic good, etc.
• Is the code easily understandable?
• Does it match your coding conventions? This generally includes the braces positions, fonctions and variables names, lines size, indentation, format and commentaries.
• Is there redundant or duplicated code?
• Is the code as modular as possible?
• Can a global variable be replaced?
• Is the code commentated?
• Do loops have a specified size and a correct ending condition?
• Are the functions and variables names relatable and linked to their meaning?

Performance

• Can you do obvious performance optimizations?
• Can some part of the code be replaced by a library or a function from the language?
• Is there logs or debug related code left to delete?

Security

• Are all entry datas are checked and encoded? Type, size, format and reach.
• When third party services are used, are errors checked?
• Are exit values checked and encoded?
• Are incorrect values properly handled?

Documentation

• Are there commentaries and do they describe the code objectives?
• Are all the functions commentated?
• Are there unusual behavior or specific cases described?
• Is the use and behavior of third party libraries documented?
• Are data structures and units of measure explained?
• Is there any incomplete code? If yes, should it be deleted or branded by a TODO or another way?

Testing

• Can the code be tested? It must be structured to not add to much or hide any dependency, and be able to initialize objects. Testing frameworks can be used, etc.
• Are tests existing and are they understandable?
• Do the unit tests really check the expected code behavior?
• Can some testing code be replaced with an existing API?