Stories about Software


Prerequisites for Effective Code Review

Editorial Note: this is a post that I wrote originally for the SmartBear blog.  You can read the original here, at their site.  You should check out the posts by some of the other authors over there as well.

I’m betting that, at some point in your travels, you’ve seen a blog post or a document in a software shop that details prerequisites for good code review. It probably contains items like, “make sure you’ve run all the unit tests” and “make sure you’ve given the reviewer(s) enough notice.” This information certainly has value, but it’s not what I want to talk about today.

Today, I want to talk about prerequisites for effective code review culture, rather than prerequisites for an actual code review. The question thus is not, “are you prepared for this code review?” but rather “is your group prepared for code review, in general, to be as effective as it can be?”

Asking a question like this makes sense, if you stop and think about it.  A director of software engineering can’t one day stroll through her group’s team space, declare, “Alright, folks, start doing code review,” and expect awesome productivity to follow straightaway.  Various elements of preparation are necessary, first.


The Right Review Tools

Imagine if we were teammates working on a code base of half a million lines, and I asked you for a code review.  The scenario would possibly look like this: I zip up the code base, and email it to you to take a look at.  I note that I’ve edited CustomerDao.cs and a couple of the Javascript libraries.  You should be good to review my code now, right?

It’s easy to laugh at this folly, and it’s easy to take some of your tools for granted, but they are vital to code review being helpful.   For a code review to be effective, you need to be able to find the needle of modified code in the haystack of code.  And, furthermore, you need to be able to find a bunch of those needles in a row and cycle through them.  Here are some features of your review tools that are essential.

  • Seamless source control integration.
  • Savvy diff highlighting.
  • Navigation between changes.
  • Incorporated syntax highlighting and other contextual help.

Armed with all of these things, I’m no longer asking you to pick through raw code and figure out what I did.  You can piggyback on the source control system to figure out exactly what I changed and look at each bit of modified and old code, side by side, one at a time, in an environment that’s familiar.  If any of those elements are missing, the reviews will be inefficient and error prone.

The Right Workflow Tools

Of course, there’s more to the code review process than the part where you look at the changes that I made to the code.  This, too, makes sense, since if there weren’t any more to it, you’d look at the changes, close your diff window and never speak of the matter again.  At the very least, you need to offer me feedback.

But, in reality, it tends to go well beyond that as well.  There’s an entire workflow to code review and this workflow can be optimized.  Think of all the decisions that go into managing a code review process.

  • Who is qualified and available to review my code?
  • How do I request a review?
  • How do I know when the review is finished?
  • What do I do with the feedback I receive?
  • Can I reply to or debate pieces of feedback, and, if so, how?

If you’re managing all of this with an ad-hoc combination of email and spreadsheets, you’re wasting developers’ time, confusing people, and allowing things to fall through the cracks.  If you’re going to get serious about reviews, you need tools specifically designed to manage this workflow.

Good and Fast Continuous Integration Setup

Once you’ve got the tools in place to perform the code reviews and manage the information that they generate, you’re in pretty good shape.  But there’s another, more subtle tooling consideration: your CI setup.

At first, these might seem like orthogonal concerns.  After all, I’d imagine many of you reading have done stints in shops where long stretches of development were followed by massive code reviews.  Only after both were complete could you promote the code to the main branch or environment.  And the reviews still happened, right?

They may have happened, but it’s unlikely they were particularly efficient.  In the first place, code reviews go better in shorter, highly focused bursts.  But beyond just the effectiveness of the reviews themselves comes an interesting matter of incentives that manifests itself both upstream and downstream.  I’m talking about the “merge party” that occurs in large batch development.  A merge party is what happens when a development team orders pizza and soda and settles in for a long night of merging the code everyone has written over the last 3 months.

When these mammoth merges are a regular part of the development process, the code that was just reviewed has a tendency to get hacked and mangled at the edges, rendering the review somewhat moot.  And, upstream of that, if you know that a giant merge is coming for the code you’re reviewing anyway, are you really going to give it your all?

Code review is a lot more effective when CI allows you to review code that was just written, in small batches, and knowing that it’s going to be promoted imminently once the review items are resolved.

Cultural Healthiness

So far, I’ve talked about tooling, but perhaps the biggest prerequisite for an effective culture of code review is an effective culture period.  You can have the best tools on Earth, but if your group is wrapped up in office politics or riddled with mutual distrust, the code reviews will be less about the code and more about personal posturing and jockeying.

For the group to coalesce nicely around code review they must trust each other and they must have a healthy working respect for one another.  When these things are true, team members feel comfortable giving and receiving honest feedback directed at improving the collaborative work product.  If you sense that this is not the case with your group, see what you can do to get your house in order before you start aggressively pursuing code review as a matter of course.  It may not be nearly as hard as you think to inspire a bit of trust and goodwill.

Bang for Your Buck

Let me be clear about something:  Even if none of the above prerequisites are met, having code review will be better than not having it.  Code reviews are extremely effective for discovering potential defects before they escape into production.  But if you’re lacking these prerequisites, you won’t be getting nearly as much mileage out of the review process as you could.

Newest Most Voted
Inline Feedbacks
View all comments
Thomas Lassanske
Thomas Lassanske
8 years ago

A few things I believe help in making code reviews successful are: 1. Commit and review (Crucible), or review and commit (SmartBear) frequently, in small chunks of completed functionality. This is perhaps implied when you refer to Continuous Integration. Nothing dismays me more than when I see an impending review with hundreds of files and a change description that would qualify as a short story. The reviews tend to get put off until the reviewers feel they have enough time to complete them, and it is difficult to keep dozens of changes in mind and figure out which applies to… Read more »

Erik Dietrich
8 years ago

Definitely agree with all 3 things you said here, and the separation of cosmetic and core changes, where possible, is a great idea. I’d never thought of that before, but it makes a lot of sense.

Thomas Lassanske
Thomas Lassanske
7 years ago
Reply to  Erik Dietrich

I do this, personally, as much for selfish as altruistic reasons: Committing the cosmetic change or refactor is a way for me to “lock it in”. That way, if the later functional change causes the code to “go up in smoke”, it is easy for me to diff my current changes to see what went wrong, and can simply and easily revert the change altogether without losing the previous work, if need be. It also makes merges into other branches safer and easier.

8 years ago

I usually see commits with hundreds of changes and a commit message that is not even a complete sentence, though 🙂
Also, see: “Ask programmers to review 10 lines of code, they’ll find 10 issues.

Ask them to do 500 lines and they’ll say it looks good”.
on https://www.gitcolony.com/features