Over the years, I have had the opportunity to work with various code review systems, and I have come to believe that one particular approach works better than the other.  Here is what I’ve learned.

There are basically three ways to approach code reviews:

  • No code review.  Everybody checks in code liberally, and while everyone is welcome to examine change lists as they please, there is no incentive and no obligations to do so.
     
  • Non-blocking code reviews.  When code is submitted, a reviewer is associated to the change list and they are expected to review the changes at some point, but this review does not block the submission of the code, which gets incorporated in the depot right away.
     
  • Blocking code reviews.  In this scenario, submitters associate a reviewer to their change list but that change list does not get submitted until the reviewer gives their approval.

Let’s get rid of the first case quickly:  I strongly believe that projects that work without any peer review will end up with code of significantly worse quality, regardless of how talented or experienced the developers are.  It doesn’t matter how good you are, you can’t produce top quality code all the time.  We all get sloppy at time, and code reviews are here to address these times.

When the code reviews are blocking, the committer is not allowed to check in their code until the reviewer has responded with a positive comment.

When they are not blocking, the code gets checked in right away but the reviewer still receives an email giving all the details about the code so that it can be reviewed later (I suggest setting up a nag system that will remind lazy reviewers to do their reviews at least once a week).

In my experience, the only system that works is non-blocking code reviews, but before I go into details, let’s examine exactly what code reviews do, because I found that very often, the perceived benefits are different from the real benefits.

Here are a few facts about code reviews:

  • Code reviews don’t reveal critical flaws in your code.

    This is disappointing, but it’s the sad truth.  As the system becomes complex, so do the change lists, and you won’t often hear a reviewer say "If the servlet receives more than twenty connections, you will have a deadlock between A.java:723 and B.java:1410".

    More likely, reviewers comments will be cosmetic ("Please rename mI to midIndex, it makes the intent clearer") or they will request more comments.
     

  • Code reviews do keep you honest.  They force you to take some extra time to make your code more readable, because you know for sure that someone will be reading it with a critical eye very soon.

I found that proponents of blocking code reviews usually feel very strongly that anything more liberal is bound to be the cause for a significant drop in quality as well.  The argument often goes like "If reviewers do not feel a strong obligation to review the code, they will slack and the review won’t be done".

First of all, my experience tells me that this is incorrect.  I found that even in the most liberal non-blocking code review systems, reviewers were very diligent with their reviews and they usually got done in a fairly short amount of time (less than a week, and usually within a couple of days).

But what’s inherently flawed with this assumption is that it fails to recognize that both blocking and non-blocking code reviews rely on the honor system, so saying that one will work while the other won’t doesn’t make sense.

In the blocking code review scenario, when you submit your code, you rely on the reviewer realizing that you are now unable to perform any additional work (or at a considerable price, see below) until the review comes in.  Despite this pressure, it’s not uncommon to see reviewers take several days before reviewing code, because they are just swamped and they also know that the committer is taking measures in order not to stay inactive while she’s waiting on the review.

Problems with blocking code reviews

I haven’t had good experiences in environments that mandate blocking code reviews.  Here are the main drawbacks to such a system:

  • While they wait for the review, developers are stuck and have to work on something else or put in place a system of patching, which they will have to resolve next time they do a sync and try to commit their approved code changes.
     
  • Developers know that they will be stuck as soon as they commit, so they accumulate huge change lists in order to keep working and "stay in the zone".
     
  • These huge changes lists become problematic for reviewers who decide to not take a look at the change list until they can carve out a significant portion of time, which delays the review even further, therefore creating a vicious cycle.

While non-blocking code reviews work better

I strongly believe that non-blocking code reviews address all the problems listed above and come with their own set of good properties.  Here are some of them:

  • Being a reviewer when the review is not blocking works well, because a conscientious developer will be aware that reviewing the code as soon as possible is part of their job, just like writing tests or comments.
     
  • I am much more likely to take an extra ten seconds to write a comment if I know for sure that somebody will be reading my code, and possibly comment on it (which is why I said above that "code reviews keep you honest").  Conversely, I have very often noticed that code that was never reviewed by a peer is usually harder to follow because it is more sloppy and less commented.
     
  • Reviewing code written by colleagues forces me to a daily and healthy exercise.  I have to change my mindset and get familiar with idioms and styles that are not mine.

What about pair programming?

Pair programming is definitely one way to do code review, but it suffers from a few limitations that, in my opinion, make it a less desirable option than non-blocking code reviews:

  • Pair programming only catches micro-errors.  Contrast this with non-blocking code reviews that expose your reviewer to an entire change list, which will force them to see a bigger picture than they would have if they were pair programming with you.
     
  • Pair programming naturally forces the two participants to converge toward the same thinking.  Once you both decide that you need a class A, you will naturally agree that it implies the creation of classes B, C and D, and after a while, you have a "design of a single mind".
     
  • Pair programming can also take longer.  The usual argument to defend pair programming is to say that programmer A can do task X in three minutes and task Y in twenty-five minutes, while the times will be reversed for programmer B.  Put these two programmers together and they’ll be able to complete tasks X and tasks Y in three + three minutes.

    This is nice, but quite idealistic.  In reality, the outcome of pairing A and B can have vastly varying results, such as a task that would take three minutes by programmer A alone suddenly takes thirty minutes to accomplish with B in the picture.  Or A and B spending a lot of time arguing about the "right" way to solve the problem, while ultimately, there are more than one right way and A would have solved the problem much faster on their own.

Anyway, the point of this post is not to criticize Pair Programming but simply to make it obvious to everyone that the full impact of Pair Programming hasn’t been studied very well so far, and that it’s not a very good substitute to simple peer code reviews.

Finally, Brian Slesinsky wrote an interesting entry praising Pair Programming and criticizing code reviews.  Take a look if you want to get a counter-opinion to mine, but in general, I find that Brian’s arguments are a demonstration that blocking code reviews are not optimal much more than he demonstrates that pair programming is the right solution.  I think that non-blocking code reviews have all the characteristics of a happy medium between these two radical approaches.

What about code ownership?

Shared code ownership is vastly over hyped.

Shared code ownership is presented as a desirable consequence of Pair Programming and it simply means that since at all times, there has always been at least two people working on the same region of code, the entire team has a much better knowledge of the code base and that everyone can therefore step in at any time to fix a problem even in the absence of a key member.

First of all, reasonable shared code ownership can be reached with peer reviews.  It’s pretty easy to see why:  when you spend some time reviewing a teammate’s code, you gain a very familiar knowledge of it, and it won’t take you long to dive back in if you need to.

But more generally, shared code ownership is simply unnecessary if the code has been written by competent developers under careful code reviews.  It is very common for me (at least once a week) to stumble upon an area of code that I’ve never seen before, and I usually have no problem finding my way around it if the developer has followed some simple rules (commenting, naming the classes, methods and variables appropriately, using design patterns, etc…).  Code reviews will buy you a micro-knowledge of the code, but I find that I usually don’t need to know the code in so much details most of the time.

Finally, if you need a proof that code ownership is vastly over hyped, look no further than open source projects.  In typical open source projects, reviews are not mandatory and everyone can check in code at their will.  New developers come and go and they don’t seem to have much problem learning the new code base, assuming that’s it’s been commented and written with good design principles.

Conclusion

If your organization is not mandating code reviews, you should reconsider this decision and, ideally, opt for non-blocking code reviews.  There are various other ways to implement code reviews that I did not cover in this document, but whatever you choose, make sure that members on your team regularly get exposed to code coming from other members, even if superficially.

How about yourself?  Are you doing code reviews in your company?