Pair Programming and Code Review - Joe Mabel

Pair programming and code review, singly or in combination, are among the best investments any enterprise can make in strenghtening developers' skills, developer team cohesion, and the quality of the code. The two practices serve many of the same purposes. They are similar but distinct, so I am covering both in one article.

This article describes my approach to pair programming and code review: if you want a historical overview or more about the variety of possible approaches to this, there is a Wikipedia article on each topic.

Terminology

In pair programming two developers sit together (or Zoom with one another, or whatever) and actively interact while writing code. Code review is a similar process for code that is already, at least in the coder's view, completed. Most often two or more developers go together through code that one of them has completed quite recently. Code review looks for comprehensiblity, coverage of edge cases, conformance to standards, and even outright bugs; participants usually also discuss how it might have been done differently, or how they'd approach a similar problem next time.

Pair programming necessarily involves exactly two people: any more than that and it's just a meeting, not a hands-on programming session. In its most orthodox form, only one of the two actually writes code, while the other is more of an interlucutor. Code review can involve more than two people — I've had my code reviewed in a conference room by four or five developers — and it can also be done by a single developer on another developer's code in the latter's absence, with comments and questions simply written into the code and passed back to the original developer. (I've even similarly reviewed code left behind by someone who had already left the company in question.) That last approach requires that the reviewer be a very good writer.

Code review can be "driven" by the coder or by an interlocutor; it depends who is more comfortable running what is essentially a meeting with a simple agenda: go through the code; understand it; one hopes, improve it. Usually, code review adds a lot of comments but code as such is not immediately changed, or only trivial code changes are made: if code changes will be needed, whichever participant is hands-on at the computer puts a specially marked comment in the code and someone — typically the developer who wrote the code — gets back to it later under less pressure, to actually fix it.

My rule of thumb is that about 10% of hands-on programming effort should be devoted to pair programming and/or code review. (The emphasis there is on "hands-on programming": the amount of code review time is independent of time spent writing specs, working out system architecture, testing, etc.) I have yet to see a project where that investment didn't at least roughly pay back within two release cycles.

Both pair programming and code review should generally be peer processes. The process should work equally well regardless of whether the reviewer or the person whose code is being reviewed is the more "senior"; similarly for pair programming. It is generally a bad idea for someone's manager to review their code, unless it is basically a lesson in how to review code. (Reviewing your manager's code, on the other hand, can be an interesting exercise.) The goal of these processes is to improve the code, not to judge people. If a team is tiny, it is usually a better idea to get review done by someone outside the team than to fall back to having it done by the manager (again, excepting training exercises).

Benefits

Among the many benefits of pair programming and code review:

• Your code will become more maintainable.
• Developers will learn each others styles, find each other's code more readable in the future, and typically begin almost automatically to converge on adopting each other's best practices.
• Developers will discover each other's strengths and weaknesses, so future work is more likely to be assigned to the person who can get it right and/or the person for whom it will be exactly the right level of challenge, and dev management has a chance to train developers on what each actually needs to learn instead of taking a generic training approach.
• Developers will learn parts of the system where they would otherwise never be hands-on.
• Done right, this is a great team-building exercise.
• You will almost certainly find (and fix) some bugs that would otherwise not be found until testing, and possibly a few "time bombs" you might otherwise have missed entirely.

More maintainable: Unless a body of code is a throwaway one-time task, it will eventually need to be maintained (bug fixes, new features, changes to third-party APIs, etc.). It typically takes an enormous amount of experience and self-discipline for a developer to have any idea what is obscure to others in their code.

Convergence: I worked for several years in a group of developers on a big telephony and messaging system, with certainly over 30 person-years of developer time invested into that code base, plus commensurate QA work, hardware integrations (phone switches, voice cards, fault-tolerant RAID disk systems), etc. We did so much pair programming and code review that by the end you could almost never tell whose code was whose unless they put their name on it: about the only clue was how good the writing was in the comments. We ended up with common conventions for naming variables, identical rules for code layout, a strong consensus on what needed commenting, and a constantly strengthening system architecture because we got consensus as to what parts of the system were becoming top-heavy (too many new "features" on too little initial architecture) and needed to be revisited.

That proved to be a very good thing for our employer, because (back to "maintainable") over five years later in a series of mergers and spinouts, that code base constituted a major asset which got sold to two different firms on a mutually non-exclusive basis. One of those firms had pretty solid continuity of developers, but the other needed only a few person-months of consulting from members of the old team to get a new group of a dozen or so developers, testers, etc. completely up to speed.

Georgia O'Keeffe, Red Canna, 1919. Van Gogh could not have painted this.

Vincent Van Gogh, Irises, 1889. O'Keeffe could not have painted this.

Strengths and weaknesses: Developing coding skills isn't like learning to beat a 1980s arcade video game, where everyone is more or less on the same path and it's just a question of how many tricks you've learned. It's more like becoming an artist: Van Gogh couldn't paint a Georgia O'Keeffe painting of a flower, and O'Keeffe couldn't paint a Van Gogh. One developer may be great at meticulous detail; another at devouring a new API; yet another at getting a good overview of the system, or seeing just where to abstract a parameterized function so you don't have to write almost the same code fourteen times. Someone may actually like maintainance programming and making incremental improvements; they might be the only one on the team who does. Someone else might be much better at keeping up with the "latest and greatest" extensions to a computer language or function library, another might read StackOverflow the way the average person binge-watches a mini-series; yet another might be great at coming up with names for variables.

And (back to "convergence") some of them will learn a lot from each other. It will surprise a non-developer to learn this, but in the absence of processes like pair programming and code review, few developers ever look at all closely at colleague's code unless and until they have to change it to fix bugs or add features. Developers talk to one another about database design, about neat new language features or an API they just found, about system architecture, etc., but in the absence of overt encouragement, they don't talk about, "Here's how I handle having multiple loop variables" or "Here's how I decide what information to put in an error message."

Learning the whole system: Pair programming and code review are both really great ways to expose different developers to parts of the system they might not otherwise ever look at. Under Agile approaches where there is relatively little by way of technical specifications, this can be particularly important. (Yes, you should retroactively write the equivalent of a technical specification, and I've done a bunch of those, but how many companies ever really allow time for developers to do this? Pair programming and code review may not solve that problem, but they mitigate it.)

Team-building: Properly approached, pair programming and code review can really help to build a jelled team. If there are tricky egos involved, definitely start with pair programming. It's a lot easier in human terms to get two developers to cooperate with one another than to have one later critique the other's supposedly finished code. If there are no tricky egos involved, code review is probably better "bang for the buck," because you never have one person sitting there while the other hems and haws for several minutes thinking about something.

Bug fixes: A lot of people are under the misimpression that code review is about finding bugs. Code review will probably incidentally find some bugs. It is pretty much the only way other than extensive white-box testing, which few companies do, to find a bug in code that implements a capability that has no features built upon it yet. Ditto for "time bombs" that work fine now but which make an assumption that will someday be false. However, the purpose of code review is much more to make sure that code is readable and conforms to standards (or close enough that no one is bothered) and to let developers see what each other are doing than to find bugs. (Want to find bugs? Much more important than code review is to put together a good test team; ideally, have at least one person on it capable of whole-systems thinking and another person with a hacker mentality who likes to work out a way to break your lovely new system. Also, put your Beta in front of prescisely those end users who will pound it mercilessly and expect it to work perfectly. They may not look like they're your friends, but they are.)

Ramping into this quickly

If your development team doesn't regularly review code and/or occasionally use pair programming, I can pretty much guarantee that you'll gain a lot from introducing these practices. They have enough in common that for the rest of this article, I will just focus on code review.

The key to making code review succeed is to have is a mandate from management: "Yes, it's OK with us that some time be allocated to this. Yes we understand that this is an investment in better code and a more mutually connected dev team." If management treats code review strictly as a cost (or, worse yet, as an indulgent waste) it probably won't happen, and certainly won't happen right.

It is probably best to start with relatively senior developers' code being reviewed by less senior developers. This won't necessarily have the most rapid initial payoff in terms of improving code (though you just may discover some surprising talents among your junior developers) but it is far easier socially than the other way around.

A few things to keep in mind when first doing code reviews:

• Before you even start this, come up with a convention to mark issues to get back to later. It should be something easy to search for, and you probably want at least to come up with a way to mark outright bugs differently from other sorts of issues. I've been at companies with as many as 50 different codes for different types of issues. For example:
• >>>00002: Error not getting logged
• >>>00003: Not the data we need in the log
• >>>00007: Vestigial, kept for the moment as harmless, get back to this
• >>>00012: Bad variable name(s): misleading, uninformative, obscene, or inappropriately multiplexed
• >>>00021: Scalability issue
• >>>00026: Outright bug
During code review, you then add comments like the following (using my initials here and a random date):
• >>>00002 JM 2021-02-04: need to log error
• >>>00007 JM 2021-02-04: Vestigial, keeping for now
• >>>00003 JM 2021-02-04: if this error pops, we need to know what file(s) it affected: add that to the log. Probably also who (if anyone) is logged in
• >>>00003 JM 2021-02-04: May want each caller to this function to pass in a unique ID so on error we can log that and have context. Or have this trigger a stack dump
• >>>00003 JM 2021-02-04: Database error, so log the failed query (and maybe the immediately preceding query as well, because its return is how we built this query)
• >>>00026 This function pretty much explodes on a negative value of 'count', so it needs to check its inputs better.
Someone — typically the initial developer — needs to follow up afterward. Once they address the review comment, they typically remove or reword it (and certainly remove at least the ">>>" so searches don't turn up a bunch of resolved issues). Typically there is then another shorter review of the specific diffs since the first round to determine whether the concerns have been met.
• If the code has a lot of shortcomings, most likely you needn't try to find them all in a single round of review. "You don't have to eat a whole egg to know it's bad." Just cut your losses and recognize where you stand: e.g. that you identified several issues, marked them up, the developer needs to work on them (quite possibly in a pair programming situation!), and later this should actually be ready to review. Further review at this time is pointless.
• Not every shortcoming needs to be fixed right away, just noted. For example, (a) if certain code won't scale as the database gets bigger, but for the next three years the system isn't going to need to scale to the point where that's a problem, then this release is not necessarily the time to rewrite it. (b) Abstracting common code into a function to eliminate dual maintenance is rarely an emergency.
• Conversely, some shortcomings need to be fixed right away. (a) You should not wait to fix a security hole. (b) You probaly do not want to release code that does not consistently check to see whether each SQL query succeeded or not, or one in which the failure cases don't each lead to a clear, predictable behavior such as "Write to log. Bail out" or "Log problem, but otherwise ignore."
• Typically, there should not be a ton of actual bugs and omissions in code that an experienced developer thinks is ready to be reviewed. On the other hand, otherwise good developers who have not been through a lot of code reviews from either side usually have a lot of room to improve in terms of commenting their code, choosing good variable names, noticing when they should abstract a function, etc. This is especially true of self-taught programmers.
• Comments at the head of a file and before each non-trivial function within a file are really important. (a) What does this file/function do? (b) Was it cribbed from a published example on the Internet? If so, provide a URL! (c) What inputs does it take? In PHP, for example, inputs are typically implicit, passed as values in the superglobal associative array $_REQUEST. If the code in a particular file calls a function that accesses $_REQUEST, then an implicit parameter, possibly quite an important one, may not be referenced overtly even once in the present file. Reviewing these, or generating them during review, can be as important as reviewing the code itself.
• Other than those header comments, code can usually be "sufficiently commented" without writing it all a second time in prose. You do not need to say overtly that each normal for-loop behaves like a normal for-loop or that total_cost = cost_per_hour * hours multiplies cost_per_hour by hours and stores the result in total_cost. Presumably, code is going to be read only by someone who can read code.

My e-mail address is [email protected]. Normally, I check this at least every 48 hours, more often during the working week.