On Code Review

One of the discussions happening right now in the Mozilla Foundation software team is whether mandatory code reviews are a good thing.  I've had versions of this conversation a number of times in the past few months, and today I'm going to write my thoughts down so I can point at them when it comes up in the future.  Before I begin, I'll be honest and voice some frustration, and wonder aloud why this is even necessary.  To me, the question of code review is resolved: you should do it, and the effects on your code, community, and project will be positive.  However, I think it's a useful exercise to defend things you hold to be true; so here are some of my thoughts on the subject.

My introduction to code review, and the type I know best, is the one practiced by the Mozilla project, specifically what happens with code changes making their way into Gecko, Firefox, etc.  The easiest way for me to describe what happens is to show you some examples.  Before I started writing this, I went and picked a handful of the most recent changesets landed in the mozilla-central repository.  The first thing to note is that these changes explicitly point at the review process, calling out the the reviewer by name:

What kinds of code needs a review?  What does it take for that code to get an r+ instead of an r-?  Let's look at a few real-world cases, starting with Bug 830871.  Here's a bug about evolving code, laying ground work for things that need to happen down the road.  It's a small but common type of bug: We used to do X, now we want to start doing Y.  Let's take a look at the fix:

-extern JS_FRIEND_API(bool)

+extern bool

This fix removes 15 characters. It's trivial; hardly worth bothering anyone with, so why not just land it without review? Jeff, who wrote the patch, is an expert developer. Jason, who is being asked to spend time reviewing this, is incredibly busy with much more important patches of his own. So what's going on here?

The first thing to understand about code review in a project with more than one developer (Mozilla has hundreds if not thousands), is that it communicates change. This bug is a perfect example. In the past, some decision has been taken ("We want to get rid of JSProtoKey as part of the JSAPI eventually") and this is one step on the path to arriving at that change.  We get closer to understanding what's happening here if we think about code review for such a patch as a kind of Catechism, where an assertion is made publicly, then affirmed.  By reciting what we believe about the direction that the code is taking, we bolster understanding and awareness.  "Folks, remember that we're changing this API."

These so-called Catechism reviews can be seen all over the place in Mozilla.  Here's another one that landed a few minutes earlier.  Again, a tiny change that simply removes a few lines of code, and the desire for confirmation from a trusted peer: "ted: Please rubber stamp trivial change." Code review spreads awareness of changes across the community.  I'm changing X, and I want you to know about this, too.

Notice also a secondary review request in that bug, this time for testing validation: "gaston: Please verify this actually fixes the problem."  Here we see the role of review in helping test conditions beyond the means of the developer.  I wrote this patch on platform X, can you test it on platform Y?  Review distributes, shares resources, increases the power and reach of an individual to test their own assumptions.  The review ends with a moment of encouragement, "That allow m-c to pass configure & start building (so fixes the regression from 784841), and i can even use mach now \o/"

Another type of review you'll find happening a lot is one in which a strategy is developed, an architecture established, or an approach agreed upon.  In the first review I examined above, there were hints of this from the past, "We want to get rid of JSProtoKey..."  At some point that decision was made, and what was being reviewed was more the decision than the code as such.  Code enacts the decision, but the strategy has to get signed-off on as well.  You see a bit of this happening in Bug 831188.  Again, a one line patch (if you're starting to feel like Mozilla developers never write big patches, don't be fooled), and this time a review from one of Mozilla's senior developers and architects.  Benjamin goes on to not only r+ the patch, but also takes the opportunity, through the review, to spec out some new work.  Code reviews are decision points, moments where the direction of not only this code, but that other code we should do, come into view.  What's more, they are public decisions, which enable participation.

A lot of people decry code reviews as nothing more than busy-work, since many reviews do little more than identify so-called nits.  Having the work of hundreds of developers all adhere to certain standards, and display a common style, etc. means that there will inevitably be things you have to change in order to get your code landed in our code.  I write code for so many different code bases that I don't know what my coding style is anymore--I do what the current code does.  Nits don't bother me, and mainly because I know that they often evolved with good reason.  Here's an example in bug 828228:

One other nit: I don't like using constants for short strings, like TELEMETRY_SERVICE_INIT/TELEMETRY_BUILD_CACHE - it's a level of indirection that makes the code harder to read/grep, and doesn't really serve a purpose (typos would be caught otherwise, and we don't need to change these values often).

"We've found that if you do it this way, it's better, and here's why..." Often the rationale for a nit isn't given (sometimes it's clearly understood, and just an oversight by the developer). Gavin chooses to use this code review to not only get his way with the code, but also to help educate another developer as to why this is practiced. Code review is where one learns how and why we do certain things.

How about a larger code change? Sure, bug 824864 will do.  Go scan that bug, look at all the various interactions.  What do you see?  First, notice that Bobby confirms the code is passing automated tests.  This review isn't going to be about "does it work?" since that's already been established.  Here, code review is about how and why things have been done.  What else do you notice?  I notice Bobby asking for review from two different people.  Next I see three separate reviewers helping out with the effort.  It's a big and complicated set of changes.  Code review is something we need to do.  What do the reviewers find?  Style issues, re-introduction of unwanted APIs/code, things this change exposes we should follow-up on, changes that maybe shouldn't be made, brittle code, inconsistent use of APIs, and on it goes.

A reviewer like Boris (truth be told there probably isn't a "like Boris" anywhere) spends a lot of his time just doing reviews of this kind.  I've had people tell me that so-and-so is spending too much time on reviews, implying that if they were writing code instead, they'd be more productive.  The thing this misses is how one developer can only write so many patches, but their influence and ability to affect many more is increased through reviews: Boris doesn't need to write every line of code, but the 4 or 5 he discusses in a review can make all the difference.  Code review is leverage, which allows top people to be in more than one thing at a time.

It's important to state that all of the people involved in that last bug are expert developers.  How many times have I heard people claim that their code doesn't need to be reviewed?  Don't believe it.  Code review has nothing to do with how good you are as a programmer and everything to do with how complex programming is, and how impossible to get right.  When Brendan (yes that Brendan) writes code, it goes through review too, fails, and gets better.  Code review isn't something that gets done to you, it happens to the code you write.  It's not a personal assessment.  It's something we do in order to not get crushed under the complexity of what we're building together.  Programming is hard, and Mozilla code reviews are filled with people missing things that others help them see.

Code review is humbling, and it's good to be humbled.  Code review is educational, and it's good to be taught.  Code review is public, and it's good for open source to happen where people can see what's going on.

I also think that code reviews can be done badly.  People can use code review as a weapon to endlessly tie-up some change from ever happening.  Some might argue that the examples I chose above, while current, are all changes that actually landed.  What about those that didn't, got WONTFIXED, or the like?  I would argue that the failures there are not about code review, and only show up because of the public nature of a review.  If person A and B are fighting, that review may expose things.  If one camp is against using tool X and others are trying to get it landed, the real issue is communication between those two groups.  Things come to a head in review, which can be uncomfortable, but is also necessary if those issues are latent within the community, project, company.

You can and should call out bad review practices.  Is someone being a jerk with nits?  Talk to them about it.  Is someone infinitely delaying a feature with pointless follow-up review failures?  Go have the real discussion that should be happening instead of the secondary arguments about the code.  The problem isn't code review, so don't lay it there.

Code review is also one of the best ways to enable participation, and though there are many other things I could say, I'll end on this one.  Why not just use pair-programing?  Why not talk to so-and-so at her desk, and get some advice before doing it myself?  Code review, at least in the Mozilla context, happens in public view.  The evolution of the code is something I, as a non-employee, can witness, understand, discuss, and participate in productively.  If we replace code review with higher-bandwidth interactions, which let's face it means things happening at the office, we eliminate the potential for the same degree of open collaboration.

Mozilla taught me the value of code review a long time ago, and now I want to help continue that culture within the Foundation.  It's easier to work alone, or in pairs, to not have to pass everything by other people.  It's also how you stay small and insular.  If you want to grow, if you want to be open, if you want community, and if you want to be Mozilla, I think code review is one of the best ways to get there.