Category: Tricks of the trade

Github has added some new features to pull requests that make for a richer code review cycle. To commemorate this, I thought I’d share some words about code review–not the Github stuff specifically (I’m still exploring!) but the process in general.

Code review is a cornerstone of good team-based software development. It isn’t the same as pair-programming, and it isn’t a replacement for it. By the same token, pair programming is not a replacement for code review. The two practices work in tandem–and, as I have suggested elsewhere, code review can also make solo coding a viable part of the development process. I consider it absolutely essential.

In this article I’m going to go over three guidelines for successful code reviewing:

1. Do your code reviewing alone
2. Be fair and flexible about style
3. Treat unclarity as a red flag

I’ll say something about each of these guidelines in turn.

Do your code reviewing alone

If you are pairing on code review, your pair is either (a) one of the people who wrote the code, or (b) someone else who didn’t write the code. Both options are problematic.

Reviewing code with someone who originally worked on the code inevitably means that you’re seeing the code through their eyes. You’re really asking for a guided tour, not doing a review. In review, code should speak for itself. You don’t want someone there explaining it to you. That’s not representative of what it would be like to work on it yourself at a later date, or for someone new to the team to pick up a story that required them to work on this code.

Moreover, you might–without meaning to–put your pair on the defensive. “Why did you do this?” you might ask, pointing to some line or lines of code. The person who wrote or co-wrote those lines then has to mount a defense of them, and that can be frustrating as well as unproductive. Lines of code in isolation sometimes don’t explain themselves; you, as the reviewer, might have to look at the whole class or file to see the rationale for the code’s structure or naming conventions. But quizzing the coders on why they did something is not the same, cognitively, as looking through the code yourself and letting the code speak (or fail to speak) to you itself.

Doing code review in a pair where neither of you worked on the original code is also not advisable. Each of you will pick up on different things, and pointing those things out–not to mention scrolling up and down and flipping among files out of sync with each other–is simply not the same process or experience as examining the code yourself. Your different paths through both seeing and understanding the code will cause interference patterns and almost certainly make the task less efficient and less effective.

Now this doesn’t mean that you should never sit down with someone else and look at code! I’m talking specifically about code review: someone has just committed some code, and it needs that next pair of eyes. You may have occasion to discuss code you didn’t write with someone who did write it, and you may have occasion to discuss code with someone else when neither of you wrote it. (Think of a pair picking up a ticket on some code by people who have left the team.) What I’m saying is not that it’s bad for these things to happen. What I’m saying is that they are not code review.

Be fair and flexible about style

Your team should have a working agreement about code style. It may be a blanket agreement (“When we write Python, it has to pass Pylint”). Or it may be a very specific agreement (“No empty parentheses in Ruby method definitions”). How you determine your team or “house” style is a matter for the team and the organization.

When you do code review, you should not hesitate to point out violations of team style agreements. However, if the code you’re reviewing does not violate team style–but happens not to be in the style you yourself would have used–leave it alone. Code review isn’t about making everything look as if you had written it.

If you spot something stylistic that you think no one on the team should be doing, make a note of it and bring it up later as a possible addendum to your team’s style agreement. But don’t flag it in code review if it isn’t already part of that agreement. Code review is not the place for stylistic ping-pong matches.

Treat unclarity as a red flag

Perhaps the main thing you should be looking for during code review is code that is not clear. Unclarity can take many forms; but whatever form it takes, it should be treated as a show-stopper. Unclear code means someone is going to get confused at some point down the road–possibly including the people who wrote the code in the first place. (We’ve all had that happen!) You should immediately write a comment when you come across something unclear. In your comment you should include some analysis as to why you think it’s unclear: a method that’s doing too much and is hard to follow, confusing variable naming, and so forth.

This guideline is actually not only important in its own right, but also figures as another reason not to do code review alone. If you do code review in pairs, it’s too easy to explain the code to each other as you go. You don’t get enough of a chance to determine whether or not the code per se is clear.

Don’t be afraid of insulting someone’s code by saying it’s unclear. By the same token, if someone reviews your code and says they didn’t understand or couldn’t follow something–whether because of logic or naming or anything else–don’t defend or explain your code. It’s likely that you could explain the unclarity away. But you shouldn’t have to, and in the future when other people look at your code, you won’t have the opportunity to. Consider the finding of unclarity to be a reality check, and a very compelling indication that you need to change the code.

Happy reviewing!

Pair programming is a wonderful practice. But it isn’t a cure-all for code-quality issues, and it isn’t the only game in town.

My reasons for not being a fan of a 100% pair programming approach fall generally under two headings:

• pair programming has some (sometimes serious) drawbacks
• other practices often produce good results

Let’s look at these in turn.

Pair programming has some (sometimes serious) drawbacks

I’ve heard it said many times that the chief strength of pair programming lies in its incorporation of instantaneous code review. But this is a dangerous half-truth. I am an unshakable believer in peer review of all code. Pair programming, I’m here to tell you, isn’t what I mean by peer review.

Yes, the members of the pair do catch each other doing things they shouldn’t. The locus classicus in my case is the time Brian Olore talked me off the ledge when I, fed up with undefined method 'blah' for nil errors, threatened to write something along the lines of expect(nil).to_receive(:blah).... Would I really have done it had Brian not been there? Was it just a cry for help? We’ll never know. But yes: pair programming can save developers from themselves, and from each other.

But it can also become a folie à deux. The pair becomes a team-within-a-team, and when both members of the pair find a code problem equally exasperating, it’s not uncommon for them to agree, perhaps against their better judgement, on a suboptimal solution. (I’m talking mostly about design and style, not sweeping non-working code under the carpet.) Getting a third person on the team to look at the paired code is peer review. Pair programming isn’t.

Pairing can be an unpleasant experience. I’ve been on teams with a 100% pairing approach, and I’ve had some very bad days. Not everyone is born to pair with everyone else. You can, if you like, say that developers who don’t like working with their pair should just suck it up and do it anyway. I don’t buy that. I want my software development practice to be enjoyable and rewarding, not only in the long run but as it happens. I believe in programmer fun. I do not want to feel uncomfortable or bored or in conflict very much of the time at all. I dislike the Procrustean approach to pair programming, which says (or implies) that pair programming is Always Right and everything else, like whether or not you like your job, has to fall into place around that.

Another serious problem with a doctrinaire approach to pair programming is that it relegates other, potentially very fruitful approaches to the scrap heap. That’s no good either.

Other practices often produce good results

I write some of my best code when I work alone for a while, and then come up for air and get a code review. The working alone takes me to depths of concentration that I cannot achieve when I pair. My colleagues are there to catch me if I fall (and I them).

I’ve been soloing on code, in one form or another, since 19[REDACTED]. It’s one of the great pleasures of my life, and it works to the benefit of teams I’m on because it results in good code. A doctrinaire approach to pair programming, however, rules this kind of work out of bounds.

This makes no sense to me. If you have a path that leads to good code, why would you abandon it? By what standard of measure is that better than not abandoning it?

Res ipsa loquitor, I would have thought. Nor do I just toss in mention of code review to make my pronouncements more credible. It’s not uncommon for me to come up with either new ideas or solutions to old problems that are right on target but, in the doing, a bit over-engineered: too many new classes, too much indirection, and so on. My colleagues can spot both the goodness in what I’ve done and, in many cases, a simpler way to do it. And I reciprocate. It’s a symbiosis that differs from that embodied in pair programming, but which produces excellent results.

And here’s another thing: Github. If there was an equivalent of Github pull requests fifteen or twenty years ago when (as I remember it) pair programming started to become popular, I don’t know what it was. Today it’s possible, and easy, for teammates to go line-by-line through each other’s code, compare it to what came before, comment on it, and suggest changes to it. The landscape is not the same with these tools as without them. That’s a major reason why I find the notion of 100% pair programming constrictive. It strikes me as outdated.

I have whimsically referred to soloing-plus-pull-request-review as “asynchronous pairing.” There’s something to that–though mind you, I strongly advocate pull request review for all code, whether soloed or paired. And, to come full circle, pair programming has a lot going for it. I’ve been on teams where no one does it, and I prefer teams where they do. At its best, it’s highly satisfying. But it isn’t the only way to produce code of high quality, and we cannot afford to jettison other practices that allow us to do just that.