How to Give a Good Code Review

Searching the web for “how to give a code review” will net you plenty of resources on why we do reviews (share knowledge, find bugs, improve code quality, …), and what to look at when doing code reviews (design, legibility, correctness, …). But there’s surprisingly little on how to give a good code review. At a mechanical level it seems rather straightforward: i) examine the code, ii) make judgements, iii) send them to the author. In that first step, however, there are so many opportunities for excellence and negligence, it’s anything but straightforward. So I thought I’d write about it.

Most of this advice comes from experience working at Java shop, using a variety of tools to facilitate the code review workflow: Gerrit, Stash, and most recently GitHub. So, I guess YMMV.

The following is what I’ve seen the best code reviewers do.

Understand the Code

OK, this is seemingly the minimum requirement for doing a code review. And yet I’ve seen so many reviews fail at this basic requirement. You know how you can tell? When you get a code review on a complex change, and the only suggestions are superficial: e.g. change identifer names, split up a function, extract a constant, or add comments. How often are code reviews like that at your company? How many code reviews have you done that are like that? These kinds of suggestions are helpful, but they aren’t impacting the design of the software. To impact design you need to grok the code you’re reviewing.

To understand new code you must play with it. Download the change; load it into your IDE. Hop around in the new code — everytime you see function you’re not 100% familiar with jump to its definition, for every function changed in the patch audit all of its calls. Don’t hold back. Your understanding of the code is proportional to the time you spend playing with it; and your impact on its design is proportional to your understanding of it. So get in there.

In other words, if you’re doing code reviews of complex changes from the diff view of your code review tool, your team is probably not getting enough out of code reviews. Don’t get me wrong, the diff view is great for finding where changes are; but it’s terrible for understanding code. (Until GitHub incorporates code navigation right in the code review tool! That would be awesome.)

Don’t Confuse Intent with Correctness

Here’s a tip to help you understand code.

Sometimes when I’m looking at code it’s really easy to grasp the intent of the author, without actually knowing what they did. For example, let’s say the code review is of a Print dialog feature, and you run into five lines of code that looks like it calculates the length of the print out. It’s easy to say, “Ah-ha, this code calculates the length. That makes sense.” But all you’ve done is established the intent of the author! The whole point of a code review is to establish correctness, not intent.

Don’t be lazy. If you find yourself skimming code after establishing intent alone, go back and reread the code with a more careful eye.

Check the Boilerplate

And don’t be lazy about the boilerplate code either. I hate boilerplate, but if you’re coding in Java, it’s an inevitability. Boilerplate code is just as important as the rest, so make sure you review every line of it. Besides, checking boilerplate is usually pretty easy, and the 1 in 50 chance you catch something might make you a hero.

While you’re deep in there, you might even develop ideas to remove the boilerplate all together. #winning

Run the Code

If you’ve followed my advice so far, then you’ve already made the effort of switching branches to this new code, disrupting whatever feature work you were doing. May as well run their code while you’re at it. There are several reasons to do this.

• You might be the only person to run it before it gets merged. I know, I know… the author should have run it before sending it for code review. But there are a lot of times that doesn’t happen. For example, the author runs it before submitting, notices some issue, “fixes” it, and then submits. We’ve all made this mistake before, and we’ve all been bitten by it. Do your friends a favor and run their code.
• Sometimes differences between your development environment and the author’s will show up when you try to run their code. These issues can be very subtle and frustrating to track down after the fact, so early detection is key.
• User interface stuff is easier to evaluate visually, by running the code.

Multithreaded Code & Concurrency

If the code you’re reviewing concerns multithreaded parts of your application, then buckle-the-fuck-in cause it’s time to go deep. Actually, that’s not always true. There are two cases to consider: i) the code adds a new concept to the design (e.g. a new thread, a new consumer, or a new producer), or ii) the code does not, that is to say, it works within existing design concepts.

If you’re in case number one, then you really should buckle up. Maybe even see if the new concept is absolutely necessary, because not adding it might yield a good enough result, and simplify the architecture. When that’s not possible your only choice is to get into that code. Sadly I don’t have any pearls of wisdom to impart on those adventurers embarking on the multithreaded journey other than, good luck.

For the latter case, hopefully you’ve built enough abstractions around the existing design that slotting in new code doesn’t require being aware of the gnarly underpinnings. As a code reviewer just make sure the new code uses those abstractions correctly.

Check for Tests

Most of what I’ve written above could be summarized as, “Really, really, really understand the code you’re reviewing.” Well here’s something for the lazy reviewers among you. Always remember to check that adequate automated testing has been added. It’s an easy thing to check for, and if your developers are like me, you’ll likely find some shortcomings.

That’s It

As I was looking around the internet for relevant articles, I saw a lot of articles that recommended creating a code review checklist; and then zero mention of what belongs in the checklist. Well maybe the above can serve as a start for that checklist:

1. Download the code you’re reviewing locally, and load it into your IDE/editor of choice.
2. Understand it deeply. Check every line.
3. Run it. Run its tests. Run a full build.
4. Double check any areas that deal with multithreading.
5. Check for automated tests.

GL HF.