Keeping code review sessions friendly

Code review is something that sends a chill down the spine of some developers, while others let off a radiant smile upon hearing it (I know I do). Code review shouldn’t be seen as some kind of activity where the reviewer challenges the reviewee’s recently written code. Indeed, code review is a co-operative activity. We do it to help each other reach a common goal, which is to achieve higher code quality, reduce frequency of bugs and last but not least: learn from each other. After all, the code we write is the company’s property, and we as a team has a collective responsibility for keeping its quality as high as what is practical.

I will primarily focus on the “people aspect” of code review in this post, but I’ll also end with briefly describing what to look for during code review. So, without further ado, I will discuss a few principles of code review which I think are very important.

It’s the code which is being reviewed, not the coder

The only thing that should be of interest to the reviewer is the code. Who wrote it doesn’t matter, and nor should it matter. The reviewer should criticize the code in an objective and factual way. They should not put any value into how bad, in their mind, some potential deficiency in the code is. It may be tempting to make a bit fun of someone’s solution to something if you find it to be a bit asinine. But keep in mind that the person who wrote that piece of code doesn’t necessarily know as much as you do. Heck, you may not even know as much about the context of the solution as the reviewee does, thus you can’t for sure know whether the solution really was asinine or if there was a sound reason behind it. Only by talking with the reviewee will you know. Whatever the case, you should be aware that the reviewee is a human being just like you (I assume you are one), who has feelings. Be conscious about how you express yourself. This goes for both the reviewer and the reviewee. Having trouble taking criticism is something that is inherent to human nature; we usually don’t like having our hard work criticized. That doesn’t mean you shouldn’t criticize it of course, but giving constructive criticism is one step towards greater code review sessions.

While it’s the responsibility of the reviewer to give objective and constructive criticism, the reviewee has the responsibility to be open to suggestions. They should be able to take criticism. If not, and if this becomes apparent through passive-aggression and sarcasm, for example, people will be less likely to review your code. You will not develop your skills, team morale decreases and potential deficient code enters the code base at a higher rate, simply because people would rather avoid pointless and hostile arguments with you. And rightfully so, because it’s a waste of time.

Criticism given by the reviewer should not be taken personally (unless the reviewer is making explicit personal attacks, in which case the reviewer has gotten the point of code review completely wrong). It is the code that is being reviewed, not the coder.

It should be viewed as teamplay

Co-operation in code review is important. There shouldn’t be anything that divides a reviewer and a reviewee from each other. You should, together, discuss the code that has been written. In a code review, the reviewer and the reviewee can pretend that none of them wrote the code. They just stumble upon it, some foreign ethereal creature has decided to create a pull-request and merge it into the develop-branch. Their job is to, together, find and discuss any potential deficiencies in it. The reviewee should be open for suggestions as well as questioning any criticism they’re given, when deemed necessary. The reviewer should be open-minded as well and realize that their job is not to degrade somebody else’s code. Remember, view the code as if it wasn’t the reviewee who wrote it. Be objective and factual.

Give compliments

Of course you shouldn’t shower someone with compliments over how brilliant you perceive their code to be, but whenever you find something in the code that especially stands out to you as something good – let the reviewee know about it. This has the potential to raise team morale and motivate people. People love appreciation, after all. But it’s important that the compliments are genuine; people also tend to be good at sniffing out whether a compliment is sincere or not.

Look for actual problems with the code

This is a bit more about what the reviewer should look for during code reviews. The following things are important, ordered by most important to least important:

1. Does the code solve the actual problem that was defined? In other words, does it do what it’s supposed to do?
2. Are there any design deficiencies which make a noticeable negative impact on the maintainability of the code?
3. Are there any potential bugs?
4. Has any tests been written? Are they self-documenting?
5. Is the code understandable?

All those points are almost equally important and all of those points should be addressed. Code style and such should be automated when possible. Reviewers should ideally only look for problems which a computer can’t find; catching unreadable and unmaintainable code is exactly that.

Summary

Code review is important for maintaining high code quality and spreading knowledge throughout the team. However, if code review is done poorly, it may turn out to cost more than the value it actually provides. So I think that the principles I’ve been talking about in this post could be used to achieve a better code review culture and a greater return on investment for it in the long run.