Code reviews: quick and easy?

I decided to not write any intro to this blog post. A little confession should work better this time.

I do not enjoy code reviews and tend to avoid them

For as long as I have been practicing code reviews as a reviewee and a reviewer I face the the same discomfort again and again. And here’s why.

What causes negative emotions?

I am surprised but code review comments hurt participants. Sometimes, it seems way too hard to reach an agreement: Everyone has an opinion but the arguments are not strong enough to convince the others.

As human beings we’re often offended when results of our work are criticized. As developers we’re probably even more sensitive when it comes to code criticism.

People get emotionally or physically exhausted of arguing and a worse thing happens — a decision is made without an agreement. One has to disagree and commit loosing a piece of morale. Too often, the decision picked is dictated directly on indirectly by support of the most authoritative participant. Alternatively, there may be a voting about which decision should be made (yes, I have seen that).

There is a chance that a similar flame will occur in the next review since no real agreement was reached. These are just a few things that I don’t enjoy in code reviews no matter what role I play there.

Why code reviews are not quick and simple?

Do you want to spend a few minutes writing down a list of fundamental things that are needed to make quick and simple reviews possible? Done? Check it with mine.

First, an established agreement must exist before the first code review in a project. The agreement should cover code styling, patterns, and other things that can become an apple of discord. However, referring to status quo is unacceptable. Status quo is NOT an agreement per se nor a strong argument. Ideally, an agreement should become a code analysis tool config file shared via source control system. A code review reference can be used for things that are hard or impossible to achieve with tools.

Second, subjective criteria should be preventively turned into objective. E.g. using of ternary operator is allowed as long as expression is simple leads to a discussion about what is simple enough and what is not. The resolution could have the following form. Use ternary operator when subexpressions are trivial: either literals or variables or method invocations.

Third, the agreement should recommend a clear collision resolution process. Finding a resolution in an authoritative decision should be an exception. In fact, every time such thing happens, a process smell note should be taken.

Last, the agreement is a subject to periodic reviews. The principles of code requirement rules may be written in stone. This does not apply to the rules themselves. On another hand, things should not be revisited every day — this would be a poisonous practice. It’s good to define conditions that can trigger an agreement review.

Well, is that it?

Note quite. The idea behind the things listed above is to leave as few space for in-review arguments. Then a code review might become a binary process …assuming all developers are robots rather than human beings.

Yes, this part can not be automated or process fixed. It’s exactly what Agile manifesto takes into account. There is an expectation that developers are mature enough to not take comments on their code personally. It’s hard though. There are no magic reminders in the code diff browser, unfortunately. But wouldn’t it be nice to have a note saying “hey, this is an opportunity to learn as well as an opportunity to improve quality of the product built”?

Another pitfall for a developer involved into a review is to follow the ‘we did it this (or the opposite) way on project X” reasoning. It’s in our nature to use intuition and habits to perform actions faster, often without thinking. An this is exactly where the trap is! Now, I am not saying we should not rely on our experience. It would be impossible to be efficient with even simple processes then. The question is: Where is this line between “I will use a proven solution” and “It’s time to revisit the beloved solution”? Just the fact that some project used a specific approach in the past doesn’t mean it is still the best one.

The last thing to mention is the authoritative way of disagreement resolution. It will never work well in long-term perspective. The number of such resolutions per month/quarter/year can be an interesting metric that helps understanding whether there’s too much authority in one’s hand or not. It can also point to team’s inflexibility in terms of new ideas adoption.
A leader that exercises his power this way can make the team members shy and quiet. It may or may not be useful depending on what team’s goals.

Got conclusions?

There are very generic ones for the code review time:

Think harder when you feel emotions affect working processes.
Think harder when you start rushing. In contrast, try to go a bit faster when stuck on thinking hard.
Don’t choose the easy way: combine the intuition and thinking.

Leave a Reply

Fill in your details below or click an icon to log in: Logo

You are commenting using your account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s