Code Review

Pair programming, a practice employed in numerous agile shops, provides a form of continual review. With pairing, two developers build software collaboratively. On the surface, this would seem to be an extremely wasteful use of highly paid developers. But the notion that pairing costs twice as much could only be true if programming were as simple as typing.

Programming is a complex activity involving continual design. This complexity makes it fairly easy for a solo programming head to produce a questionable design. But with two sets of eyes in two programming heads, it’s not as easy. When we bounce ideas off one another, we increase the likelihood of producing something with lasting quality and value. We create flexible designs, readily understood code that’s more easily maintained, and systems with fewer defects. The feedback provided by this two-headed interaction produces what we call active review.

Historically, after-the-fact reviews have been the more prevalent choice for trying to increase software quality. Code walk-throughs, Fagan inspections and pull requests are common mechanisms for running what I’ll call “passive review after code,” or PRAC. PRAC is similarly based on the idea that solo programming increases the risk of low-quality software. The review in PRAC is, by definition, not active — reviewers instead examine things produced previously.

More importantly, reviewers examine things that were produced without their intimate involvement. In producing code, you and I will have a detailed conversation involving things like assumptions and constraints: “We don’t have to worry about validating this data because the import process will take care of it.” We’ll also dive deep into discussions about how to best solve that problem.

Unfortunately, the important bits of our dialogue don’t always make it into the code. The code becomes a distillation of the solution and loses a lot of the nuance about why. Worse, the dialogue that does get directly represented in the code often ends up inscrutable.

To begin to understand such confusing code, we’d need to resurrect those conversations, but that’s not usually possible. We could ask for a document, but it would be costly to produce, unlikely to be kept up to date, and itself likely to be misinterpreted anyway.

It’s better, then, that we insist code is reviewed by a third set of eyes, and sooner rather than later. The code produced must be understandable, or else reviewers — and future maintainers — will waste time deciphering its intent. But will this independent review help?

The Problems With PRAC

The first problem with PRAC is that it doesn’t usually uncover more than surface-level defects. Sometimes it doesn’t even uncover what should be obvious, glaring problems.

The other week, scrolling through a bunch of past changes to code, I spotted a two-line code change that seemed not quite right. My pair partner told me that the change indeed contained a defect that was later fixed, but only after the customer discovered it. It turns out that three people should have seen the defective two lines: A pair produced them, and at least one other set of eyes reviewed their change afterward.

Yet “should be obvious” doesn’t mean we won’t gloss over two problematic lines in a sea of changes. While I knew something was wrong based on ingraining visual cues from seeing so much code, I had to reread the lines a few times for the reason behind the defect to sink in.

Perhaps the core reason we miss these kinds of errors is that if we’re not deeply committed to the logic at hand, we’re not likely to think it as fully through as necessary.

The second problem with PRAC: We’d like to think that negative feedback from these reviews magically translates into better designed, more understandable code. But for too many reasons, this doesn’t happen.

The programmer who produced the solution we’re reviewing has this wonderfully human thing called pride. They arrived at the logic that supports the problem they were trying to solve using code, and it was probably the best solution they could derive at the moment. A suggestion involving a simpler solution is likely to be received with indifference at best, and viewed as insulting at worst.

As long as the software works, the pressure is usually on us to move on. It would be nice to replace an overly complex algorithm, but that would require considerably more of our precious time. The code is good enough for now; we must move on. “We’ll clean it up later,” we say, but later availability never arrives.

Demanding Commitment

Most programmers, myself included, would much rather work on our own code (whether or not in a pair) than spend the time to review the code of others. PRAC represents yet another interruption; we must stop thinking about our code problems and solutions and temporarily think about someone else’s code. We’re reluctant to invest any more brain activity than needed, lest it chase away important thoughts about our own code.

Fortunately, there are at least a couple of simple mechanisms that eliminate the distraction of PRAC:

1. If pairing, switch pairs mid-task.
2. Mob.

Frequent Pair Switching

If we agree that a third set of eyes is important — and we should agree — then we should involve that third set before we complete the solution. A pair usually will have decided on a course of action early in their session, and they are not likely to convince themselves to change direction.

A third set of eyes sitting down to actively complete a solution, particularly a set of eyes tied to an active mouth, has some chance of steering an errant approach back to a more sensible course. They’re now stuck working with this code, and, as such, they are far more likely to insist that the logic gets cleaned up to the point where it makes sense.

Such a switch doesn’t come without cost. Context-switching is hard, as I hinted at earlier: We must drop our current line of deep thought and replace it with another. But we can get better at it, particularly when aided by focused, literary tests and readily digestible code. A modular design that is well-compartmentalized makes it easier to quickly ramp up on what you need to know for now.

But Isn’t It Still Too Late?

I’ve found pairing to be a good, sometimes great, practice, but I’m increasingly inclined to think it demands a bit too much overhead and management. It requires thinking about who’s not pairing with whom, when are we pairing, what happens when there are odd numbers, how do we deal with conflicts between person Y and person Z, and so on. And then there’s this context-switching cost I just mentioned.

A third set of eyes will still likely be met with some resistance, even if they arrive only 90 minutes after the prior pair got together to work. Granted, they have the right to now insist that the code they’re able to tackle at least makes sense. But that original course of action is probably protected by a solid amount of code — up to 90 minutes’ worth. A solution path surrounded by a mass of code has a stubborn way of resisting change.

Logical progression suggests that we might solve this problem by heading off any form of review after coding and just engaging the third party to enjoin work on a solution as it begins. Similar extrapolation leads to the notion that we may as well have everyone agree to the solution before it’s worked. In other words, skip pairing and go straight to mobbing.

Mobbing in Brief

Per Woody Zuill, mobbing is “all the brilliant people working on the same thing, at the same time, in the same space and on the same computer.” It might sound like a ludicrous idea, yet it logically solves the challenge of PRAC, as well as the mechanisms needed to make pairing more successful. But everyone working on everything at the same time?

That’s a great discussion for another blog post. For now, I’ll say that I keep hearing from teams who mob that they go faster. I’ll leave thoughts about why that might be the case to you.

In the meantime: Yes, you want at least three sets of eyes on all code produced. It’s the minimum for any legitimate review process. To get those three sets of eyes, you can mob, but you can still choose to pair (or you can even choose to not pair and suffer PRACs).

Quality code is the key outcome we seek. “Three sets of eyes” is a good starter rule for achieving that outcome. The mechanism a team chooses can still be up to them.

To explore the features of Ranorex Studio, download a free 30-day trial today, no credit card required.

About the Author

Jeff Langr has spent more than half a 35-year career successfully building and delivering software using agile methods and techniques. He’s also helped countless other development teams do the same by coaching and training through his company, Langr Software Solutions, Inc. In addition to being a contributor to Uncle Bob’s book Clean Code, Jeff is the author of five books on software development: Modern C++ Programming With Test-Driven Development, Pragmatic Unit Testing, Agile in a Flash (with Tim Ottinger), Agile Java, and Essential Java Style. He is also on the technical advisory board for the Pragmatic Bookshelf. Jeff resides in Colorado Springs, Colorado, US.

You might also like these articles