Code reviews are a critical part of the modern software development cycle. Unfortunately, a lot of cycles are burned, egos flare, and morale is damaged because reviewers don’t know how to offer suggestions constructively.
Here are some things that I think are very important to improving your code review process.
Figure out why you do code reviews
Is it to find bugs? Is it to spread knowledge of the codebase? Is it to find architectural problems? What do code reviews offer your team? If you don’t know the answer to this question, you don’t know how well they’re working.
Automate your style guide
If the answer to “why you do code reviews” includes “making sure the code follows a consistent style,” cut it out. Style comments are a huge waste of time. It is very difficult to read the code for both correctness and style at the same time, and it is often easier to make a bunch of style comments and feel like you’ve accomplished something than to dig deep and find real issues with the code.
A deluge of style comments are often overwhelming, and drown out conversation about the code itself.
Tools exist in most languages (we use Rubocop and Scalariform) to handle code formatting automatically, or at the very least, to provide warnings to the author during build. In no circumstance should paid engineers be scanning code for whitespace problems in 2016. While automated tools will never provide a level of aesthetic that a human eye can offer, they can get plenty close enough at a much lower cost.
If you haven’t automated your style guide, you don’t have a style guide, you have a tool for your team to abuse each other. Automate this problem away.
Offer suggestions, not orders
Don’t tell a committer how they’ve done a thing incorrectly in a code review remark. Ask them what they think about your suggestion. For example, a comment like this:
Don’t repeat this here, extract a function.
obviously feels very negative. The author may feel defensive and start a pointless argument, even if there are few benefits to doing so; or worse, the author may feel like a less valuable contributor, and follow the suggestion blindly. In either case, actual harm is done by comments like this.
The best thing you can do is to offer a suggestion, not as a gatekeeper, but as someone who is trying to learn about the changes to the code.
What do you think about taking this with the above and extracting a function?
Why should you make your comments sound so tentative? Because you are reviewing your peer’s code. And you have to invite them to politely explain what’s going on, or to notice that you’ve made a better suggestion. Having no manners in this regard eliminates the possibility of discussion for a significant segment of the population.
Let me talk about my number one pet peeve in code reviews.
Why didn’t you…
Asking a “why didn’t you” question implies a great many things, none of which are constructive or positive. It assumes that the author of the code considered your suggestion and explicitly chosen a different one, which may not be the case. It implies that you feel your alternative is obviously superior (especially in the special case of “why didn’t you just…“), and that you require the author justify their solution against it. It implies that what may be intended as a mere suggestion is, in fact, the default.
Whenever I am reviewing code and my hubris needs to be checked, I try to offer suggestion. “How do you feel about…” or “what do you think about…” are good drop-in replacements for this horrible phrase.
Use overtly positive language
It is important to recognize that written communications have a profoundly negative bias. We tend to interpret neutral communications as negative, and positive communications as neutral, which is a source of a lot of consternation in code reviews. To combat this, strive for overt positivity.
Terseness can be interpreted as rude or inconsiderate. In particular, avoid adding a two or three word comment.
My genuine unsupported-by-data opinion is that code quality and correctness are only marginally improved by having a code review by a gatekeeper. It is rare that a reviewer shares the same level of insight as the person who authored the code. A better model is to have code reviews treated as a learning exercise for the reviewer, i.e. to understand the changes to the code and to grow in understanding of the code base. I think this leveling gives their questions and remarks appropriate weight, and allows for a healthier discussion about the larger codebase and how the changes interact with it.
It is easy to imagine that brutal code reviews are a ritual that leads to the best outcomes. “We’re attacking the code!” In my experience, engineers are a lot more sensitive to criticism than they let on, and they are a lot more willing to be found wrong if they feel like collaborators rather than defendants.
I recommend taking a few minutes to check your hubris, and see if there are steps you can take to be more encouraging in your reviews.