In the previous article, we introduced a general overview of the code review practice as well as two specific formats we use in our projects.

Nevertheless, successfully introducing a new practice is not an easy task. It’s a bit like setting sail for the first time: once in the water, the first meters are always chaotic. There are lots of waves, we wonder whether it was really a great idea. Wouldn’t it be wiser to go back ashore? However, after a bit of dedication, we finally get to the quieter open sea: we just had to hold onto it.

During our code reviews, we came across several pitfalls that were detrimental and that could jeopardize code reviewing in the team.

Let’s explore, through real-life use cases*, why the first code reviews will certainly be difficult and what are the essential fundamentals to carry out successful code reviews.

This article is sequel of the following article: What format for your code review?

* None of the characters in this paper are fictional. Any resemblance to real persons, is certainly not coincidental. Should you identify to these characters, we may help you.

My team does not progress

Within a team I just joined, I heard Bob complaining in front of his computer:

“Damn! How many times did we tell Martin that we don’t build SQL request with Strings anymore but with Hibernate Criteria?!!!”

I went to Bob to understand what was happening. He explained that he reviewed Martin’s code and once more time had to correct a SQL request that was not following the standards.

Me: “But, do you keep a written version of your standards, are they stored or displayed anywhere?”

Bob: “Nope, they aren’t, but we all know them. Martin is the only one who doesn’t follow these standards. But then again, I can understand, he arrived recently.”

Me: “Indeed… I have another question : you said that you were the one who corrected the code?”

Bob: “Yes, that’s how we do it, I’d rather correct the code myself to make sure it is well done. And I wouldn’t interrupt him for that.”

Me: “Let’s go talk to him?”

The review does not foster any exchange

In the previous discussion, why hasn’t Martin integrated the standard practice?

Proof-reading someone else's code is a good opportunity to give him feedback, to help him improve his own coding practices. Without interactions, the proofreader will successfully detect the bits of code that need refactoring, however the need for refactoring will tirelessly resurface with time since the code author didn’t receive feedback about it.

Code review should be prepared by the proofreader, by doing a first pass over the code to raise the defects or questions to ask. This method allows to do part of the review asynchronously, but might prevent to create the essential exchange, ideally verbally.

The code owner does not correct the defects.

The previous example also demonstrates a lack of trust from the proofreader towards the code author. It is a shame because, the best way to get better, to learn and assimilate a standard, is to apply it by yourself.

If the code review is a good opportunity to share knowledge, the correction step is as important than the former one:

• it is of paramount importance that the author corrects the defects himself
• if the proofreader doesn’t trust the code author to correct the bugs himself, they can start a Pair Programming session and build the correction together

This in turns empowers the code author: correcting his defects may infantilize him whereas helping him to correct his own mistakes will make him aware of his responsibilities.

Standards are not shared among the team

Using code standards is another compulsory practice to make code reviews efficient. These standards are built collectively by the team  and are written and kept accessible, for example in a wiki.

If standards are not written, people will inevitably forget or reinterpret them and thus not assimilate them. Usually, we initiate the standards at the beginning of a project and make them evolve as necessary as the project goes along. We modify them as necessary during, for instance “tech hours” dedicated to improve the standards.

My code reviews always end up becoming endless discussions or “trolls”

During the first code review organised within the team:

Kent: “hey Becky, look at line 984, the curly bracket is not in line, plus your variable $moneyList does not meet the standard case: we use snake_case here!”

Becky : “So, first of all, I don’t agree, there is no such thing as a “standard case”. And anyway, this is has been!! If followed the norm, we would be using PSR 42, and we wouldn’t need to talk about it!”

Kent: “Mmmh, you might be right, there might be some good things but I don’t agree with their naming rules!”

The discussion keeps going for a while, let’s just stop here.

Whatever type of review, collective or between pairs, the first code reviews organised within a team are often “polluted” by endless debates, generally about topics that should be straightforward.

As we’ve seen previously, in order to curtail these discussions, it is mandatory to establish written standards from the beginning. This is even more important for things that look obvious like naming standards (“what is the PSR norm used in PHP?”) or disputed distinctive features of the language (“should we use var in C#?”).

It is highly unlikely that the whole team will be perfectly aligned on these points. At OCTO, we neither agree on everything. The important point is to find a compromise and to write it down.

Despite that, we also come across aspects that are not addressed by the standards, or even complex topics which need a more advanced discussion before considering a fix.

This is why, to keep making efficient code reviews, rules of thumb are:

• Whatever the format of the code review, any topic which is not about detecting defects should be postponed until after the review
• In a collective code review, each defect should be discussed no more than one minute. A moderator and a time-keeper must make sure of it.

If a specific topic spurs debate, it must be written down in a “to decide, not standard” list: it is utterly important that no debate occurs during the review.

So, how to make successful code reviews?

Through these two testimonies, we have already given a few clues about how to make your code reviews successful. However if there had to be one rule to remember: be extremely vigilant when organizing your first code reviews sessions.

Should they go wrong from the beginning and should you not manage to quickly fix the problems, you will shoot yourself in the foot. Code reviews will certainly be abandoned among your team or will certainly be abandoned or misused.

Here are the key points that we suggest:

• If your team is inexperienced with code reviews, start by training yourself and favour collective sessions, at least in a first phase
• Build a formal code review process and make sure it is respected: how should the team proceed? when? with what tools?
• Make the practice evolve and improve during retrospectives
• Use standards that you can develop overtime and establish checklists

Next episode will be called: “We are swamped with bugs even though we are doing code reviews.”

In the meantime, feel free to share your feedbacks through the comments section, whether you had a good or bad experience!

Thanks to the OCTOs from the Software Craftsmanship tribe for their valuable inputs.

More articles on the same topic:

1. What format for your code reviews?