The Emily Post guide to code review etiquette*
Code review is a fundamental part of our development workflow. It goes something like this:
Code review is a fundamental part of our development workflow. It goes something like this:
Developer A gets assigned ticket, checks out story branch and feature branch
Developer A does some work on feature, pushes it to the origin and creates a pull request against the story branch
Developer A asks Developers B and C whether they can look at that work
Developers B and C look it over, provide advise, and then if they’re happy with it, approve it
Developer A merges the feature into the story
Repeat
For years at a time, this is our approximate workflow — it hasn’t changed so much. However, given that it’s such a fundamental part of our workflow it’s perhaps worth spending a bit of time examining why we do pull requests, as well as some things to help us all progress in an inherently confrontational process.
Why
Primarily, code review serves as a sort of social double-check of the code that we are writing. It serves a few functions, but perhaps a couple stand out as most important:
Discouraging some … less elegant solutions
As opportunities to teach another developer another way to tackle their problem
So, given this justification let’s have a look at how everyone involved can make review juust a little bit easier!
Reviewee
noun. A person who is subjected to a review.
The reviewee is the hardest position to be in during a review; our work is being critiqued, and that’s always a confronting process. However, some additional work can grant insight to your reviewer, and allow them to make better recommendations.
Split style changes into a separate commit
Sometimes when we’re working on old code, it’s easier to refactor it such that it adheres to code conventions, or make variable names more descriptive.
It’s super difficult when reviewing to tell apart these changes, which should not affect business logic, from the changes that should. As a reviewer we must spend a lot more time investigating each variable change, understanding the context behind it simply to judge that nothings changed.
Style changes are fairly risk free. Separate them, and save some of your reviewers brain power.
Write detailed commit messages
Though this is more elaborately explained in another article, I will cover it here also.
Commit messages provide context behind our changes. It is the only documentation that we can provide about our mindset when writing the change, pressures that we may be under to deliver by deadlines or design considerations and alternatives that we have considered.
Writing a commit message that explains:
The justification behind this change
Design considerations (or alternative designs)
Non development pressures (business deadlines, etc)
considerably reduces the cognitive burden on our reviewer, who now does not have to guess what we’re thinking. As well, the reviewer can help critique our assumptions.
Answer questions by way of amending commit messages
Invariably, when we’re submitting our code review, not everything that we put up is immediately understood. Particularly in the case of new technologies or non-standard approaches to problems, our reviewer will ask questions.
It’s important not to answer these directly! Rather you should either:
Amend the code such that the question has an obvious answer, or
Amend the commit message such that this design consideration is explained
Doing this helps not only your reviewer, but also every subsequent person who must maintain the code, understand what and why you did what ayou did.
Once you’ve amended your code or commit messages, point the reviewer to that change and ask simply “is your question now answered?”.
Reviewer
noun. A person who formally assesses something with a view to changing it if necessary.
In a sense, being a reviewer is still hard; we must patiently question the code review, or explain why some technical implementation is not suitable for some reason or other. However, it is a lesser burden than to have our code reviewed; it’s in this light that we must review.
Be a guide, not a guard
Most importantly, our job of reviewers is not to guard our code bases jealously, and refuse to allow code that does not reach our (often arbitrary) standards from entering.
Instead, code review is a moment to help other developers grow, or to discover new solutions ourselves to problems. We must be endlessly patient and not slip into lethargically picking at things we do not like (something, incidentally, I am bad at).
As well, we need to learn to accept code that is not (in our mind) 100% correct. Code is a collaborative process, and we need to be open as a community to code that is functional but not elegant. It is not our code base, but rather simply an extension of us as a community.
In Summary
Code review is a super nice tool to help us collaborate. However, it’s an inherently confrontational one, and we need to try and be as empathetic to each other as possible.
* Not actually anything to do with Emily Post.