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:
| diff browse |
4a1b771880d8 2013-01-16 18:39 -0800 |
Matthew Noorenberghe – Bug 829995 – Ignore case of .url extension when migrating bookmarks from Internet Explorer. r=felipe |
| diff browse |
7d49dd8c58dd 2013-01-16 18:39 -0800 |
Matthew Noorenberghe – Bug 496471 – Silence satchel warning about ORDER BY without an index since an index can’t be used. r=dolske |
Along with who wrote the code, the bug being fixed, and a description of the problem, we also see who did the review, r=reviewer. The person who wrote the code, and the person (or people), who did the review, both show up in the history of the code. That’s a fundamental part of how we work.
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.
11 Comments
I fully agree with this post. I’d like to share some related experience.
In a recent startup experience, we started as a team with people from different backgrounds, habits. We’ve come to understand the need for code reviews.
First and foremost, in a small team, knowledge about the codebase is scattered across team members. Soon enough, each person has a couple of “the guy who wrote the code that does X” badges. That’s not a problem in itself, but it shouldn’t mean that if there is a problem in that part, only this person can solve it. You’re describing it as “code review communicate changes”, I would go further and say that it adds one more person who will be able to answer questions about this part of the code. If the code writer is pregnant [1], gets hit by a bus or maybe doesn’t remember, another person carries the knowledge of that part of the code and that’s a good thing.
Also, having someone else’s reviewing code is a way to share responsibility. The code base isn’t the story of a bunch of individuals pushing code to a common repository. When this happens and some bug happens, the finger-pointing can begin “this is x’s code, it’s his/her fault!”. No code review creates boundaries in the code.
When a reviewer accepts a patch, it’s a way to say “I consider this work to fit our needs and requirements. I take as much responsibility as the writer if something goes wrong.”
In turn, this forces people to agree. This encourage people to communicate more, share knowledge, that’s a good thing.
In his post, Atul suggests that mandatory code reviews are “asking for permission”. I disagree. I think it’s rather “asking for agreement”.
Among the direct advantages of having another pair of eyes focusing on the code is that the other person will likely have other priorities or taste or knowledge. Someone security-averse may see security issues while you didn’t. Likewise if someone perf-averse reviews. Maybe the reviewer is aware of some major refactoring happening in this area that you weren’t and will ask you to hold your patch until this happens.
From Atul’s post:
“Many people who ask for mandatory code reviews have no idea what they’re asking for—because it’s mandatory, so they just have to—”
=> I think the problem is not that it’s mandatory. I think the problem is that people don’t understand what are the numerous benefits that make code reviews absolutely necessary for a project good health. In turn, this absolute necessity is turned into a policy making it mandatory.
[1] I use the word “pregnant” in a gender-neutral way considering that a male feeling concerned about the physically pregnant woman besides him is pregnant too. I do that for “giving birth” too.
And when anyone (male or female) is pregnant, they sometimes don’t go to work or have their mind some place else.
Oh, I forgot. About the “productivity” argument. I strongly agree with you and would like to add that after some time, I wonder whether no code review doesn’t end up in less productivity. After some time, it forces the people who want to make a change to fully understand things they haven’t written and few people can answer questions in case of doubt, etc.
In lack of serious studies on the topic of productivity (both short and long term) in software, I wish everyone stopped making assumption.
In decent size projects, software is a social process in my opinion. It’s much less about the code than it is about the group of people who’s writing it. The code is only the mere machine-readable codification of the group’s agreement of what the software should be doing (and how it does it). From a project standpoint, I think the discussions people have around the code is vastly more important than the code itself.
I sometimes wish people talked more and coded less… and I’m aware it’s quite a controversial statement to make in Mozilla’s culture of “more hack less yack”
As a Mozilla contributor, I found that code reviews were an excellent way for me to learn how to understand the architecture of a project. Knowing that you can write code to solve a problem and people more familiar with the project will be looking it over and helping guide you to the most correct solution removes a barrier to contribution.
(Hey, that’s me up there!
Gavin Sharp did a great lightning talk last year on good review practices; it’s worth pointing out here… http://www.gavinsharp.com/blog/2012/05/03/code-review-lightning-talk/
I don’t have much to add, but I’ll emphasize the point you made towards the end of the post. Every review system will have some issues, and those are things to be improved. It would be a shaky argument to use fixable flaws as a justification for skipping the review process.
Thanks David for this post, I could not agree more. Reviews were a great help for me to get started in different parts of the code base, since it made it much less scary to get a patch landed (and yes, I was scared a *lot* when my first patch landed). I knew, if I break the tree because I didn’t fully understand the consequences of my patch (who can in a code base as big as Mozilla’s*), it wasn’t entirely my fault — somebody more experienced also didn’t see this coming, so it’s probably not a extremely stupid mistake only I could have made but simply a mistake as they tend to happen from time to time.
Also, reviews forced me to think much harder about the changes I made. I always had the mindset “I need to be able to explain my changes in depth, and cannot just have a feeling it’s correct”. This way, reviews taught me many things in programming, sometimes because the review pointed it out, sometimes because I read deeper into a subject beforehand to know exactly what I was doing.
Finally, I remember when I first found out that *everybody* needs to get a review, even extremely experienced developers like Boris or Brendan, I told a couple friends how great Mozilla was, that everybody’s contribution was equal in a sense that it went through the exactly same process to get accepted. That feeling is still there today, so keep the reviews, please!
* bz is probably as close as you can get
I really like the example of a constructive nit that you pulled from https://bugzilla.mozilla.org/show_bug.cgi?id=828228#c2
In general I am a firm believer in code review, but one thing I have always disliked is using review to catch stylistic nits (as well as syntax errors) that are easily caught by machine, rather than wasting the reviewer’s time. If something like this slips by and gets merged, then it can have a huge negative effect (breaking builds, and contributing to a swampier section of the codebase).
Almost all of the (mozilla webdev) projects I work on have a jenkins bot that runs at the time that a pull request is opened, and a human being does not bother reviewing until it’s passed (pep8/pyflakes as well as unit and integration tests).
I have noticed that for most Mozilla projects, the latter is done informally via tryserver (if at all) and the former seems to be up to the reviewer to spot, which is distracting.
Turning this on it’s head and using automated systems to make sure a patch is worthy of reviewer time might help make the review situation less painful, and make people more likely to do it – or at least quickly turn a patch down if it’s wrong for architectural or non-technical reasons. That has been my experience in projects we’ve converted over, although of course it’s important to have more conversations about why code review is seen as a problem before throwing solutions at it I suppose
Thanks for pushing this conversation forward.
Atul Gawande wrote a great article about coaching: http://www.newyorker.com/reporting/2011/10/03/111003fa_fact_gawande. Having a coach is common practice in some fields: sport, music, acting. But in most it’s not, which makes it hard for people to improve their skills once they reach a certain level.
Gawande’s a surgeon, and he got another retired surgeon to act as his coach for a while. The experience helped him identify numerous things he was doing sub-optimally and improved his results (e.g. lowered the rate of complications from his procedures). And yet, when some patients asked “who’s that guy sitting in the corner?” Gawande avoided saying “he’s my surgery coach”, because that suggests that he doesn’t know what he’s doing! So there’s definitely a cultural aspect that needs to be understood before people will appreciate it.
So: one thing that code review does is act as a form of distributed coaching. This is a good thing — you can learn a lot of new things from other experienced people.
One other benefit: people write code very differently when they know it’s going to be reviewed. I can’t count the number of times I’ve heard “that will never pass review” when discussing how to fix a bug.
Back when Firefox was an experiment named “m/b” and went through fast prototyping-like development, we had a time where no review was required for patches to it. In that time, a lot of code was added that showed severe problems later, and some that was just written in an extremely hackish way. Things like that are very often caught by reviewers and safe a lot of time later (that time we spent with fixing those things up or rewriting them to be more sane).
Of course, when prototyping a project, the line there is much harder to draw than for an established product – I’m not saying what was done there was all wrong, but we saw how we payed for that lack of review in the early project.
Great article here. I like the discussion on whether reviews should be mandatory. I work for a company (SmartBear) that has a code review tool. When I’m talking with end users, there is a distinct split — some have a mandatory review practice; some don’t. Interestingly, some of the largest companies — particularly those in regulated industries — sometimes have tool-based code review, followed by a long meeting where they review the code AND the diffs. Of those that don’t have mandatory code review, there is always the question of what should and should not be reviewed. My advice, based on the research, is to review as much code as possible, preferably using a software tool. Doesn’t have to be ours, but tool-based reviews allow developers to send code out for review on their own time — and the reviewer to review on their own time. Regardless, I always advocate for doing reviews. The ROI on finding bugs before the product reaches the customer is pretty convincing.
Thanks for your post. I found it a useful and timely reminder as I just made some changes that broke some things that probably would not have done if I’d gotten a review. Sometimes it feels slower to wait for review after you think you’re finished. In this instance, if I had, I’d not now be backing out my new code.
Glenn
7 Trackbacks
[...] his blog, Mozilla’s David Humphrey discusses the project’s use of mandatory code reviews. “I’ve had people tell me that so-and-so [...]
[...] his blog, Mozilla’s David Humphrey discusses the project’s use of mandatory code reviews. “I’ve had people tell me that so-and-so [...]
[...] January 21, 2013 / in: Linux / Comments Off At his blog, Mozilla’s David Humphrey discusses the project’s use of mandatory code reviews. “I’ve had people tell me that so-and-so [...]
[...] On Code Review (David Humphrey) [...]
[...] On Code Review comes from the Food For Thought department [...]
[...] been discussing ‘process’ at work, and in particular, code review, garnering various thoughts on the [...]
[...] David | “One of the discussions happening right now in the Mozilla Foundation software team is whether mandatory code reviews are a good thing.” [...]