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.

9 Responses to “Adventures in Code Review and Pair Programming”

  1. Alex Forbes Says:

    I’m also a fan of pair programming after seeing its effect within a previous employer’s development group. The manager especially liked having an extra set of eyes and knowledge about the code when the other developer was out on vacation, sick, etc.

    For those who are interested in the Pros and Cons of several other methods of code review, including pair programming, here is an article written by Jason Cohen:

    http://smartbear.com/white-paper.php?content=docs/articles/Four-Kinds-Of-Review.html

    Best,
    Alex

  2. James Burke Says:

    Hi Atul, I have enjoyed doing pair programming as long as it had similar constraints as you mention above. It sounds like you and Brian were in the same location.

    Have you tried pairing with a remote person at all? It would be great to figure out a workflow that made remote pairing productive. Although I am sure just having a knowledgeable person to bounce off ideas in real time would be useful. My first guess though, using voice would be better than IRC, to tighten the feedback loop and help maintain focus. Loss of focus seems easier with remote pairs.

  3. Josh Says:

    I really like how well this works. Clearly the benefits outweigh the issues of this, and it would be great to see it done more frequently.

  4. Robert Kaiser Says:

    I surely see the pros of pair programming, but the downside (next to potential of less in-code documentation and therefore for a third person to understand it) is that it only works if you’re in the same physical place. Unfortunately, a significant portion of the Mozilla community is spread over the planet – and actually, one of the strong points is that it can and does work that way. That doesn’t mean that pair programming is bad for it, but it brings up some danger of privileging those that work in the same place over those that don’t.
    So, I’m happy to see this used to make our development go better but be careful you don’t end up closing out people in other locations or making things harder for a third person looking at the code later.

  5. Atul Says:

    Thanks for the comments!

    @Robert Kaiser: I totally agree with your concerns. I think, though, that if we do things the right way, we can leverage lots of newer technologies like audio/video streaming and screen-sharing to make our processes more transparent and allow folks to collaborate more easily across a distance. I’ve heard stories about UX design at Mozilla Messaging being done over a Skype screen sharing session, for instance, and I believe those can be recorded.

    @James Burke: Totally! I think moving forward on better collaboration technologies using open tools is a great way to push the Web platform further. This is part of why I’m really psyched to see Anant’s work on Rainbow move forward. Without it, we’re stuck either using proprietary technologies like Flash or Skype to collaborate, or falling back to IRC and Bugzilla, which are increasingly feeling more archaic.

  6. Ian Thomas (thelem) Says:

    Pair programming can be great, but it’s much more time consuming than reviewing a patch (especially if no problems / changes are found).

    Are there other tweaks to the review process that could help improve the asyncronous process. For example, maybe developers should be encouraged to review as many lines of code as they commit. You do have to be careful to maintain the quality the code review though – you don’t want people saying they’ve reviewed code when they haven’t just so that they can commit their own code.

    I don’t tend to work with a formal code review process, but with paper cuts, I find they are best dealt with as a group. Every so often I’ll review my assigned bugs and see how many I can resolve in half a day. Developers who work like this could then package these up together and ask a reviewer to look at them as a package. By their nature they should be pretty small fixes, and therefore quick to review.

  7. Irakli Gozalishvili Says:

    Hey Atul,

    Great post! I have being practicing peer programing in my previous employment and it worked pretty well for us as well. From my experience “back-seat driver” issue can be easily solved by swapping roles of the driver and navigator.

  8. Axel Hecht Says:

    Hi Atul,

    you’ve kinda addressed this as a side note, but one risk of peer programming is to actually code without written comments. Where comments are less about the review process, but about the result. I recall that I r-ed a patch just recently because the comments were like “the new thing does foo instead”, which is hard to grok if that comment is two years old.

    Another question I’d add is, how much impact would you think communication style has? Like, my brain works *way* better with speech than with text, both in sending and receiving. I bet that other people are the other way around.

  9. Silona Says:

    Skype voice with a split screen view of their computer screen and mine works for me when doing remote work. Has to be voice though… and oddly helps if I can hear some typing so i can tell lag speeds.

    While this is not as nifty as being in person, but if I can see what they are typing at the same time as I am, I’m good.

    Course I am weird in that I can sing along w music in my headphones while reading/typing a letter…