Code review is a really important process of software development life cycle that helps to ensure both code quality and maintainability. “You missed semicolon!”, “Are you sure these two implementations will play nicely?”, “We need to split logic” – these are just a few examples of what one may say during said review.
Since everybody might have a little bit different priorities we decided to compile our feelings into set of strict rules followed by general tips we’d like to share with you today. The guideline was written having PHP development in mind and as such can be specific to it (like PHPDoc syntax or linked coding standard document), however, the core concept behind rules or hints will stay no matter the language.
The key words “MUST”, “MUST NOT”, “REQUIRED”, “SHALL”, “SHALL NOT”, “SHOULD”, “SHOULD NOT”, “RECOMMENDED”, “MAY”, and “OPTIONAL” in this document are to be interpreted as described in RFC 2119.
Any new code that is added MUST be documented. To be consistent, documentation SHOULD be written using Present Tense – 3rd person.
When changing an existing code that does not have any documentation, developer SHOULD add it.
This should describe general purpose of the class/interface/trait. The more information one will include here the clearer the picture will be for a fellow developer when interacting with the code. Some examples of what SHOULD be described:
- What purpose the interface serves?
- Where it will be used?
- Is any bigger picture required to understand what is going on?
Class implementing an interface
- What makes this implementation special?
Class not implementing an interface
- See interface
To not repeat same information @link or @see tags MAY be used.
Method documentation MUST include the following information (but it is not limited to it):
- General purpose of the method, please be descriptive (i.e. “Gets pw.” is not acceptable, “Gets picture’s width” is).
- @param with expected variable type. Usage of alternative (|) SHOULD be explained.
- @return MUST be as specific as possible, e.g. “@return array” is not acceptable as nobody will know what is in the array. In case an associative array must be returned, please describe all available keys (and reconsider using Value Object).
Point 2. and 3. MAY be omitted if same information is available with type hints.
The type of an attribute MUST be specified.
API endpoint documentation
Whenever an API endpoint changes (new one is added, existing one is modified or removed) corresponding documentation (e.g. Swagger spec) MUST be updated.
Developer MUST adhere to PSR-2 coding standard. Some deviations MAY occur but majority of the team MUST agree.
General rules of thumb
This and following sections are no longer strict rules that MUST be followed but rather tips to help you to grasp what goals we’re trying to achieve with Code Review and as such will be written in less formal way.
In addition to below tips you may review this great list of code smells you can be on the lookout for.
Do you understand the change?
There is no need for you to know each and every detail of implementation but you should understand what and why is happening to not treat a part of code like a magic black box that is fed some data and returns something else. Do not hesitate to ask about unclear code, there are high chances that you’re not the only one and possibly will save another person’s time who otherwise would be struggling with the same problem 6 months after.
Are there any gotchas?
If something is not straightforward it’s always good to ask a developer to add a line of comment with clarification. At the time of reading subtracting 2 from $result may seem obvious and self explanatory, however, once the dust settles down that obvious “2” will become a magic number with no evident reasons for existing. Often “gotchas” places in code indicate problem with architecture – maybe logic could be refactored a bit or responsibilities split to make the flow more obvious and graspable. Such changes always pay off.
Is the code testable?
In some cases, there is no possibility to do things The Right Way (be it technological debt or system constraints) but a developer should never be tempted to compromise code quality because “this part of system is hideous anyway”.
Are things named correctly?
Naming is 2nd hardest thing in computer science ,yet, we must do it everyday. Let’s take a minute and think if names are appropriate and descriptive enough. If you have a better idea than the original developer, let him know and talk about it.
Do YOU want to maintain code to be merged?
Original developer may not be available for several reasons and you just might be the one to fix “his/her” code. Are you comfortable with it? Are there any shortcomings you still haven’t mentioned but think are important? It’s high time you spoke up! Also remember, this is no longer just “his/her” code as you made and accepted the change – it’s also your code now.
Are project-specific agreements honoured?
Sometimes (especially when the agreements are new) people tend to forget about them or it’s just old habits that die hard. Please, politely remind the original developer of such agreements should he ever forget about them (e.g. we recently decided to drop “Interface” suffix for interfaces and instead prefix concrete implementation with their specific purpose).
The list of hints could go on and on, despite a number of points touched I am still inclined to say we have barely scratched the surface. On the other hand though, the scratch is significant enough to be a solid foundation, or at least starting point for a discussion, on how Code Review should be conducted in systems we create. How about you and your team? Do you think we have missed something or stressed some point too much? We’re eager to hear what you’re doing differently and how we can improve our process!