1. Home
2. Blog
3. Sins of a code reviewer

Code review is a wonderful thing, which, like most good things done wrong, can do very much harm. However, there is no need to worry, because below you can find a list of things to avoid in order to get the best and dodge the worst while reviewing code.

Code review is good, mkey

Code review is really powerful. There’s no need to stress this out, there are countless articles about all the benefits it brings. Most people agree that code review is the key process in having quality code. Fewer people spot the rest of the benefits, like increasing the analytical skills of programmers, the argumentation skills, the communication skills, facilitating knowledge sharing inside the team, to name some. But it is generally accepted as a good thing to do. Like brushing your teeth.

Different people do it differently

Some teams do code review as an optional process, recommended but not imposed. Some team leads allocate a few hours of capacity to it and strongly encourage developers to review their peers’ code. There are also organizations in which this process is mandatory: the code produced by a developer must be reviewed by one or more colleagues, usually those considered more experienced, and gain their approval to be merged into the overall code. No need to discuss if this is overkill or the only responsible way to do software, as there have been countless debates on this topic. But that’s how much importance these organizations attribute to code review: they will not accept code that is not reviewed, postponing production code, slowing the development process. They strongly believe in the power of code review.

With great power comes great responsibility

As we all know since the first Spider-Man movie. Code review is hard to do. The reviewer must understand the need that led the developer to write that code, and that’s not easy because software is a highly complex endeavor. The developer must explain that need and the rationale behind the choices he did when writing the code. That’s not easy either. Without falling into stereotypes, we can say that most programmers have a difficult time explaining in their work and choices clearly. Especially regarding choices that were made years ago and which have turned into habits since then.

Most of all, reviewing code is hard for the ego.

When two people discuss some piece of code, one must be declared right and one must be declared wrong. That’s brutal. What makes things worse is that the reviewer is seen as being in a position of power: usually a more experienced programmer, sometimes the team lead, there to make the code “better”. Code review can go very bad, with dramatic consequences: negative impact on workplace relations, tensions, resentment, a feeling of a lack of control, lack of code ownership, lack of fulfillment etc. A man convinced against his will still have the same opinion. And will also be bitter.

The grand thesis of this whole thing

Mark these words: A code review which greatly improves the code at the expense of work relations and the good spirit of the code writer is not worth doing. Improving a particular piece of code on a Wednesday evening is not the main benefit of code review. Creating and maintaining good relations between coworkers while collaborating and mentoring younger programmers, these must be the main goals, there is much more to gain from them.

Of course influencing is not an easy task.

For diplomats and politicians I mean, because for programmers it is really out of our league. Switching from bluntly stating our point of view, declaring it self-evident and forcing it on others to collaborating, influencing or mentoring is definitely not as easy as learning a new programming language. But don’t worry, we can ace this like in college, with a cheat sheet.

Just don’t do any of the below, mkey

1. Exaggerated generalization

 Seeing something wrong, and exaggerating by generalizing:

• Is this what we’re doing now, build interfaces for all classes?
• Where is this going to, wrapping all SDK classes?

Don’t generalize before observing a clear, recurrent behavior which is harmful. Start with the assumption that the code writer evaluates each end every situation separately.

2. Personal taste

Invoking personal taste is an easy way to avoid confrontations and the effort of finding logical arguments.

• It’s just my strong personal opinion that we should do it this way.
• I always like to do things this way, I think it will grow on you if you try it.

The code reviewer must always use logical arguments, compare benefits to costs and point to generally accepted references like good books (Clean Code, or Effective Java or others).

3. Deaf to the need of the writer

No solution is flawless, without any cost, simply perfect. But this can’t be fixed by not having the code at all. The writer needs a solution, the current code can’t be dismissed without an alternative.

• I don’t understand why you need this, but it doesn’t look right.
• I don’t want this, we’ll find a good solution some other time, but this doesn’t seem right, please revert.

This will annoy the code writer to no end. If there’s no time for a better solution, the current solution is the best one.

4. Accusing of being possessive

• The problem is that you only want to accept your version
• You don’t like when someone questions your code

This brings down the quality of the debate, and the review is better off dropped.

5. Position of power

Using a position of power, either based on experience or job title, instead of arguments, will not convince the code writer. On the contrary, it will make him/her question the reviewer’s authority.

• Trust me, from my experience I know that this is the better solution.
• I get your point, I have my opinion, and at the end of the day, I’m the Team Lead.

But what if it’s a tie?

What happens if valid, logical arguments come from both sides, but no side is gets convinced by the other one? Then I recommend that the code should remain as written initially. Why? Because, by coding it, the writer invested effort in it, and it would affect him/her more to have to change it against his/her will. Remember that the truth is generally seen, rarely heard. If, after a few weeks, the code’s problems surface on their own, the code writer will remember the reviewer’s suggestion and, finally, he will end up accepting it.

Does it make a difference?

It kind of does. Bad reviewers are avoided by their colleagues. People whisper so that a bad reviewer doesn’t intervene in the conversation. The review is a dreadful experience. No one is a bad reviewer by choice, some developers focus exclusively on the technical side of the review and forget to be considerate and respectful towards their colleagues. That's a deal breaker.

A good reviewer is courageous - he states his point and supports it, but at the same time he's considerate towards the code writer. He is liked even by those who don’t agree with him, and he is asked for his opinion out of the blue. He gives code collaboration a good name and influences programmers in many ways, most of them almost invisible. It’s magic.

Ionut Bilica
Software Development Consultant