November 8, 2010

Adventures in Code Review and Pair Programming

A key component of Mozilla’s development process is code review, which consists of a trusted expert reviewing the material comprising the changes to a piece of software in order to fix a bug or add a feature. This is a great idea for a number of reasons:

  1. It helps increase the project's bus factor, or number of people who understand how the software works and why decisions were made. If any trusted member of the community could simply push changes without requiring another person to be aware of them and understand them, then if that person were hit by a bus or truck, some amount of understanding about the software would be lost, especially rationales that could only be uncovered by the conversation that occurs during code review.
  2. There are many qualitative aspects of code that aren't understood by a computer interpreting it. For example, if a developer writes some code that talks to another component, having someone well-versed in the other component read the changes can help correct any implicitly incorrect assumptions about the communication before flaws occur further downstream in the development process, when they will be more costly to detect and fix.

However, code review can also have its own costs. Because it requires two people instead of one, by necessity any changes one person wants to make are blocked by the other. Most code review at Mozilla is done asynchronously, which means that one developer writes and uploads a patch that a reviewer reads at their convenience. Because reviewing isn’t much fun and reviewers have lots of other things to do, the back-and-forth process of reviewing and correcting a patch can take several days, weeks, or even months—and that’s not even including the implicit task of finding a reviewer in the first place. All this can sometimes make the pace of development slow to a crawl: imagine a complex conversation occurring for one hour every other day over a period of weeks, requiring each participant to remember the state of the conversation every time their focus came back to it.

Another problem with asynchronous code review is that changes which are considered relatively low-priority, such as paper cuts, might be constantly stuck in a state of needing to be reviewed or revised because they’re constantly pushed to the bottom of someone’s queue as more important changes are discovered and prioritized above them. Even worse, developers may be reluctant to voluntarily fix such smaller bugs because they have little confidence they’ll ever get reviewed.

Enter Pair Programming

One potential solution to this is pair programming, whereby the developer writes code while the reviewer looks over their shoulder, effectively serving as a “navigator”, fixing potential mistakes before they even occur.

It sounded like something worth trying out, so I ran the idea by Myk Melez, who agreed. A little while later my colleague Brian Warner and I started fixing a Jetpack SDK bug together using this technique.

This proceeded quite smoothly. In particular, I noticed a few unintended consequences of pair programming:

  1. Coding can be a rather solitary activity, and when working on a major change, it can get lonely. Pair programming makes the activity social, which makes it more fun for me personally.
  2. There's a tremendous amount of knowledge the coder and reviewer can learn from one another in a face-to-face or voice-chat based interaction that often is too burdensome to communicate via written text. For Brian and I, at least, I definitely felt that both of us learned more from pair programming than we would've if we stuck to the standard process of writing to each other.
  3. Both Brian and I had an implicit social contract of staying focused on the task at hand, or something related to it, which allowed me to focus much better than I do on my own. When I'm by myself, it's surprisingly easy to get sidetracked by new emails or IRC messages, but because I didn't want to waste Brian's time, I never digressed while pair programming. Sometimes we'd stray off the main path to discuss the semantic intricacies of JavaScript or Python, but such excursions generally supported our goal rather than distracting us from it.
  4. Because I actually enjoyed the activity, I was much more likely to get more out of it: ask sharper questions, provide better answers, and write code with fewer flaws.
  5. Low-priority bugs were fixed rapidly because of the super-fast synchronous communication. One morning Brian and I just took a few to "warm up" and were able to make the Jetpack SDK significantly more usable by getting rid of paper cuts that may have taken months to fix using traditional code review.

Being able to simply push changes to the codebase after pair programming them made development about as much fun as hacking on my personal projects. At a very high level, in fact, I think of asynchronous code review more like asking for permission while code review feels more like simply having a conversation. Or, in pedagogical terms, the former feels more like taking a test, while the latter feels more like peer-based learning.

In short, pair programming required no context-switching and felt faster, more effective, and more fun than what I was used to.

Risks

Because pair programming introduces a strong social aspect into the development process, a lot of psychological factors can come into play. For instance, if the navigator is overbearing, like a sort of “back-seat driver”, then I suspect pair programming could quickly become less efficient, less effective, and less enjoyable than alternatives.

On the opposite end of the spectrum, I think it’s also important to ensure that the person at the keyboard has at most the same amount of knowledge about the code they’re changing as the navigator. Otherwise, it’s easy for the driver to simply go off on their own way and leave the navigator in the dust. Keeping the principle that the navigator was effectively the reviewer made it easy for Brian and I to ensure this: whenever we had to make a change to code that Brian understood more than me, I’d be the driver, and vice versa. This meant more peer-based learning would occur, improving the bus factor.

Another potential problem with pair programming is that it can become harder to record the review process for posterity. Unless the dialogue occurring between the two programmers is recorded and made available to the public, part of the development process effectively becomes closed. That said, though, Brian and I tried to remain vigilant about other people reading our code in the future: when we spent more than a few minutes discussing how to do something, we’d take a moment to write a comment explaining why we picked the solution we did.

It should also be remembered that alternatives have their own share of risks: for instance, I’ve noticed that when I review code asynchronously, I have a tendency to only skim over it and comment on the surface-level details. In contrast, while pair programming, it’s much easier to ask intricate questions about how or why something works because the cost of doing so is much lower.

Variations

Pair programming need not be at odds with code review. In fact, a fair amount of the “pair programming” Brian and I did actually consisted of one of us writing the code alone and later walking the other person through the changes in a kind of synchronous code review, fixing any problems on-the-fly. In other words, simply turning the standard asynchronous code review process into a synchronous one looks a lot like pair programming.

In any case, I’m glad that all of these development processes are available at Mozilla, since different ones seem more appropriate depending on the personalities and technical challenges involved. None of them will be effective, however, unless both participants have a vested interest in making them work.

© Atul Varma 2017