Which format for your code review?

We mainly use two code review formats in our projects: collective review which is rather formal and peer review, which is lighter. Both have advantages and drawbacks: Let's look into these formats together and see how to implement them within a team.

But first things first: what is a code review and what benefits can we expect?

In most areas involving writing, we cannot imagine that what is written is issued without proofreading. An article will always be proofread before publication (e.g. the one you are currently reading), either to check the substance – is the subject of the article well treated? – or the form – spelling, grammar, structure and text readability. Similarly, the code review practice consists in having one's code read again in order to track as many defects as possible, be it on the content – does this code work, and correctly implement the required feature? – or the style - clarity, readability, standards compliance, etc.

What about you: how many lines of code have you ever deployed in production without proofreading?

Code review is a practice almost as old as software development and widespread among companies like Microsoft or Google. There is a good reason for this: it allows defect detection earlier in the development process, yet the practice is relatively uncommon in the teams we meet. Sometimes this is because it is unknown, but it’s more usually for the wrong reasons: "We have no time, it's too expensive" or "We tried but it turned into a troll discussion" or even "I do not want to, it's just policing!". We realize that it's not a practice that simple to set up!

Detect defects as early as possible

The main objective of a code review is the same as other insurance methods of software quality: find code defects as soon as possible. The benefits of code review are well established: according to studies compiled by Capers Jones on more than 12,000 projects, collective code review helps to detect about 65% of the defects, 50% for the review by a peer, while tests detect only 30% of the defects on average.

Note that we do not say you should give up other quality assurance practices: it is an essential practice to combine with other practices, such as testing, to maximize the product quality before making it available to users. There are other benefits to code review:

  • Improve intrinsic software quality and develop standards
  • Collective code ownership: having the code being read by someone other than its author helps reinforce the collective code ownership, and reduce the risk of excessive specialization, when only the author of the code is able to change it
  • Learning: code review is a way to train younger developers while sharing standards, showing them some techniques, or how some features are implemented. It's also an opportunity for those who arrived more recently to bring a different perspective and propose new approaches
  • Improve the quality of exchanges between developers.

All these points highlight code review as an indispensable collective practice that contributes to spreading a culture of quality within the team.

Collective code review

Collective code review consists in organizing formal inspection sessions, regular and within a group, dedicated to the detection of defects.

We suggest the following mode:

  • Bounded Time and Scope

    • Sessions of 1h30 maximum, once every two weeks
    • A review rate of 300 lines of code per hour is a good target
    • Beware, spending more time or trying to read more code in one session decreases the quality of the review
  • Dedicated to detection of defects: the session should be dedicated to point the defects, not to correct them straightaway or discuss standards and design issues: up to one minute should be spent on each defect

  • Status and follow up: at the end of review, it is decided whether the reviewed code is accepted or rejected. If necessary, the selected defaults are logged and and their correction is verified

  • Participants

    • The author of the code is present, and several reviewers
    • To facilitate the processing, should be elected:
      • a moderator, to ensures that people focus on the discovery of defects, with exchanges of quality
      • a scribe, to collect identified defaults
      • a timekeeper
  • The review has been prepared: the code to review is selected and communicated in advance, so the reviewers have time to read the code

  • The review is tooled: we gradually build checklists to facilitate the conduct of the review

How to select the right code?

In order to detect new defects, we often select the code related to the latest features.

  • Ideally, we should read all the code recently produced, but it is only feasible if the team is small enough
  • Otherwise, we select in the recently modified code a sensitive or complex part, or one that introduces new concepts, new piece of software architecture, etc.

You can also select existing code that "scares", that is complex or that generates frequent regressions, for example if you know you will have to make it evolve soon.

Peer code review

The peer review consists in adding a step in the development process. This is a more widespread practice because it's simpler and less burdensome to implement. There are several variations of this type of review, from pair programming to a format close to the collective review, except that the review is made by only one other developer.

Here is a format that we successfully used on several projects:

  • In common with the collective review
    • Dedicated to detect the defects, not to fix them
    • Prepared
    • Status and follow up
  • With a pair
    • In order to obtain similar benefits with those of a collective review, the review entails an exchange between the author and the code reviewer
    • During the review, once the author has submitted the code and shared the intention of the code that has to be reviewed, only the reviewer browses the code
  • Restrain the scope to one feature: as soon as the development of a feature or a user story is considered complete by the developer, a review is triggered on this code
  • Systematic: review is systematic and mandatory for all code to be deployed in production environment
  • Tooled:
    • Since the revised code is directly related to one or more commits, we generally use the differential changes as a skeleton
    • A more advanced tool is frequently used for logging the defects, like Gerrit or directly Github.
    • On some projects, we also simply attached the list of defects associated with the user story on the board.

Who does the code review?

Ideally, the first available person handles the review. As in the collective version, it shouldn't always be the same person who handles reviews, to promote collective code ownership.

This is a mandatory step in your development process, display it as a new column to your board before the functional review stage.

What to do with defects identified during the review?

To be effective, a code review should be dedicated to address the defects, and corrections or any discussions should be held separately:

  • When the correction to be applied is clear and unanimous (functional defect, deviation from an established standard), the author of the code implements the correction after the review and a follow-up is planned to ensure that the defects have been corrected.
  • Two implementation approaches can often be argued. In a pair review, involving a third party or the whole team will facilitate decision making.
  • When the point of attention is a non-established standard, the team’s written standards must be updated, ensuring that they are shared by everyone.

It is common that some complex issues require an exchange with the entire team:

  • You can book an extra half an hour after the collective review or solicit the team for a dedicated point when you are in peer review
  • On several projects, we also ritualized a timeframe of one or two hours at the end of a sprint, known as "The Tech Hour" or Dojo, in which we can address these points

Collective or peer review?

The table below gives a (non-exhaustive) list of comparison criteria between the two types of code review:

Collective reviewPair review
Efficiency (number of defects detected)+++++
Collective Code Ownership and learning curve+++++
Quality improvement, standards evolution++++++
Cost++++
Setup ease++++

The various studies mentioned in Best Kept Secrets of Peer Code Review and the chapters on software quality and collaborative construction practices in Code Complete show that collective review is generally more efficient than peer review, but has the disadvantage of being more difficult and expensive to implement.

These studies also show that the vast majority of defects are identified during the review preparation. This comforts us in the effectiveness of the peer review, provided it is well prepared: it provides a good compromise, avoiding the cost of a meeting with many participants.

You can also use formal collective code reviews in addition to pair reviews:

  • in short format, limited to the items which are not obvious and/or not covered by the standards
  • for some algorithms or complex designs
  • during the initial period of reviews setup, so the team will collectively learn to practice reviews

Conclusion?

There are so many blog titles labelled like "The [benefits | dangers] of [Code | Pair Review | Pair Programming]", but in the end, the debate is elsewhere. Even if we issue an opinion in favor of a preferred format, it does not matter if you do pair programming, collective or peer review: the key is to find the approach that matches the culture of your team.

This article offers an interesting analysis on the subject and we’ll sum up its conclusion as follows: analyze and admit your weaknesses in your collaborative development practices. If you have not yet chosen an approach, exchange with developers who regularly use these practices. Experiment and try to improve, during a retrospective for example.

We have so far described code review formats: through a following article, we will share with you the pitfalls encountered that could put at risk this practice.