The above tweet has become an inspiration for this quick post. As a first thing, let’s clarify what we should not do during code review. @andyaaaas highlighted the top worst practices:
- stating opinion as fact
- avalanche of comments
- asking devs to fix problems they didn’t cause
- judgemental questions
- sarcasm.
These tips come from “Unlearning toxic behaviors in a code review culture” based on a presentation with the same title. It’s worth to check them for more context.
Fortunately, at the Tigerspike, the company I’m currently working at I didn’t spot such behaviors. That is definitely great but doesn’t stop people from improving things. Here’s a list of good, unusual, and unobvious practices I’ve spotted during code reviews in Tigerspike:
- labeling comments
- explaining motivation
- marking core changes
- showing result
- highlighting state
Let me uncover them in details:
As reviewer:
1. Label comments
Indicate your intentions by labeling your comment. Labels may also help to define if your suggestion is required or optional. It’s good to agree with the team on using a set of common labels for comments (using them for every comment is not required, but it’s super-handy).
Here’s an example of labels we’re currently using, but nothing stops you from creating your own set.
[GOOD TO HAVE]
(..comment with your suggestion of what reviewee may change, but it’s not blocking Pull Request from being merged..)
[QUESTION]
(..comment with a request for further explanation about the specific part of code/process behind, etc..)
[BLOCKER]
(..comment highlighting what and why need to be changed to get your approval..)
By the way: thanks for Tom Koptel, for bringing that idea to our daily code reviews.
As reviewee:
2. Explain motivation
When you feel some additional information for the reviewer may help in understanding or quicker approval, write it down in comment for a specific line. Reducing unknowns in complex code may result in fewer unnecessary and long threads. If you had a couple of ways to accomplish some tasks, and you’ve picked a specific one, it may be worth to write down your motivation — especially if you feel it may be an unpopular choice.
3. Mark core changes
Mark code with core changes in your pull request. Especially in larger PR, it’s beneficial as it increases understanding by your reviewers.
4. Show result
If code change is related to GUI/Design, one screenshot added to PR’s description will tell more than a thousand words.
5. Highlight the state of work
If your PR is in a work-in-progress state, highlight it with a comment or even include this fact to PR’s title. In that case, it’s also worth adding a to-do list with remaining work.
Conclusion
The preceding list is short and concise, but for me, that is its strength. — simplicity and pragmatism behind the scene. Implementing them may help you and your team in getting a better understanding of the code and intention of both sides, less frustration, and more efficient code reviews.
Thanks for your time!
Please let me know if you find it useful or if you have any questions/suggestions about improving code reviews.
If you’re interested in more content like this follow me on twitter