Handbook Of Manners: How To Behave In A Code Review

In most companies code reviews are part of the process today. In theory they help to distribute knowledge, onboard new people and improve learning and quality.

A code review, no matter if it is performed in a pairing manner or asynchronously, is a situation in which one person inspects the work of another person.

This can lead to the assumption that the reviewer has to accept the changes the author wants to introduce and put the reviewer in a position of power over the author who has to seek approval.

Agile development teams strive for an identity in which everyone is equally important, equally worthy and in which hierarchies typically make no difference on functional matters (as long as the manager doesn’t have to protect the business, but that’s another story).

So what does an ideal code review look like?

Let’s start with the assumption that every reviewer has the same intent - which is to convince the author to follow the suggested changes (to improve the quality of the contributed code).

When this is the main intent, different approaches to code reviews can only be different strategies to achieve the same goal.

The following advice can help to achieve this goal and at the same time improve the team spirit and culture.

Consult - Don’t Preach

The basic mindset in which an engineer should approach a code review for a change introduced by another engineer is the mindset of a consultant. Whatever you say or write; do it with caution. Sentences that start with I would approach the problem this way… or I believe this way would be more efficient are a good approach if you want to encourage the author to accept your recommendations. Way better than This solution is inappropriate! or Do it like this!. Command-like comments are guaranteed to result in damage, conflicts, decreased creativity and motivation and bad team spirit. Don’t do it.

By the way, using please to soften a harsh sentence doesn’t help. You already clearly communicated a difference in status. You have already switched your consultant “hat” for something that’s simply inappropriate in modern software development teams. The result will be a bad taste in the mouth at best.

Even more, given as a command, even if correct, the advice won’t be accepted wholeheartedly and the team morale is damaged.

If the reviewer had invested just a little more time and watched the way they communicated: the author might have been very thankful and might have accepted all advice with a smile.

The effort is well invested if one takes a break from time to time and makes sure the mindset is tuned, communication style is respectful and reflects the intention to help.

Don’t Enforce Consensus

Engineers often tend to forget that there is more than one valid opinion in a lot of cases. Not everything is determined to false and true.

So if the reviewer has communicated their opinion appropriately and was not able to convince the author, the code review is the wrong forum to let discussion escalate and enforce consensus. If the code is robust and fulfills the requirements, let it pass the review and discuss the matter in an appropriate format. You may have tech meetings or other forums in which you can discuss the topic (ideally in general and not by specific example) with a broader audience.

Letting the topic lie for a week can also help. Often times you realize that consensus isn’t important at all. That not only there are more valid opinions than yours. But also that your team maybe can afford to let both opinions stand.

Don’t Point At Rules - Give Explanations

Rules and processes kill creativity. You want to give as much freedom to everyone as possible without hurting someone else (that’s part of the German constitution by the way [Article 2.1 Every person shall have the right to free development … insofar as he does not violate the rights of others …]).

Developers construct processes and methods. They align on conventions. Sometimes other developers violate them; often even deliberately. How to handle those violations?

Often when the reviewer changes from consultant to finger-pointing judge (and hangman) who points at the wiki in which he documented the rule three years ago (without ever discussing it properly); the tone gets rough. Catch some breath, adjust your mindset….

This behavior is bad because creative people have the habit to question rules if they are in their way. If the result is that the rule doesn’t create value (but suppresses value-creation) the engineer, striving to achieve a goal, will try to navigate around it. In that case: question the rule, not the violator. In fact questioning and violating rules is a healthy habit.

Feedback should be stated like this: At some point we decided to approach things like this like that because we were convinced that’s better because of reason X and Y. Don’t you agree? Shall we re-evaluate?

Usually the matter then resolves itself. The author will probably accept the suggestion because the reviewer acted in a consulting role and took the author seriously by asking for his opinion.

The author will likely feel not so pained by the rule, and decide to follow it. If not, a healthy discussion arises that might change something for the better for the whole team.

As a general rule of thumb: do not define too many rules and processes. In a review: start with understanding the requirements, think about potential solutions also from a high-level, architectural and design-perspective. How would you have solved the problem? Then look at the solution. Find real bugs. Only beginners focus on cosmetics (and usually miss real flaws).

Cosmetics are cosmetics

Cosmetics are called cosmetics because they are not substantial. Bad formatting, incomplete documentation, bad naming or other trivial things should be handled as what they are: cosmetics. Don’t ignore them, good make-up can make a difference. But as a reviewer, double-check to make sure your attitude fits the importance of the advice. Nothing is worse than bad vibes because of things that do not have a real impact.

Don’t Override - Contribute

It is generally a good idea for the reviewer to fix small issues during the code review. You can still point the author at the changes, but it doesn’t make sense to invest the time twice. If you can fix a spelling error on the fly - why not do it?

But don’t change the design completely. Do not override the substance of what your author wanted to do. Help the author, don’t control the author.

Don’t Only Focus On The Bad Things

Try to emphasize good things about the code under review. One of the developers from my team, Peter Tackage, recently stated: If I give compliments to someone’s implementation then it can lessen the unwanted negative or criticizing aspects of a review. Something like: ‘nice solution’ or ‘neat’ when merited, can reinforce a positive mindset in reviews and communicate to the author that the reviewer is truly interested in solving the problem, rather than simply solving the problem their way - and I agree. Sometimes that’s a cultural thing. Germans, like me, for instance tend to focus on problems while people from other cultural backgrounds may be more well-trained in praising other peoples sweet spots. In communication and management training, people learn about the “Burger” strategy: if you want to criticize someone, wrap it in two nice things. Whereas I resent tactical or manipulative behavior there is a point of truth in this. If you try to see the good, talking about the bad has a completely different taste.

And The Author…

This text has focused on the reviewer’s behavior until now. Of course there is another book to be written about the author and their behavior. About how to handle and accept critical feedback; not only about how to give such.

In general you can adopt everything said; try to trust the reviewer, assume positive intent.

Be aware of the fact that the written word always invites interpretation.

Be aware of social, cultural and language barriers.

As a single, simple and practical piece of advice: structure your code, comments and commits in a way that helps the reviewer understand what you are doing.

Summing up

It is all about your mindset and team dynamics. People need to resist the urge to use the power they have in a review situation. Teams need to strive for a state in which openness and transparency are worshipped as virtues. A state in which being asked to perform a review is seen as a compliment from the author who trusts you by exposing their work and asking for comments and contribution. A state in which everyone treats the others’ weaknesses as being as valuable as their strengths and where everyone actually wants to get better together. You do not need a book of behavior. You do not need strict rules for a review. What you need is respect for others and some emotional intelligence to anticipate how they will feel reading or listening to your review comments.