Code Review Like You Mean It

open source, github, code, code review 0 comments suggest edit

If I had to pick just one feature that embodies GitHub (besides emoji support of course , I’d easily choose the Pull Request (aka PR). According to GitHub’s help docs (emphasis mine),

Pull requests let you tell others about changes you’ve pushed to a GitHub repository. Once a pull request is sent, interested parties can review the set of changes, discuss potential modifications, and even push follow-up commits if necessary.

Some folks are confused by the name “pull request.” Just think of it as a request for the maintainer of the project to “pull” your changes into their repository.

Here’s a screenshot of a pull request for GitHub for Windows where Paul Betts patiently explains why my code might result in the total economic collapse of the world economy.

sample code review

A co-worker code review is a good way to avoid the Danger Zone (slightly NSFW).

Code review is at the heart of the GitHub collaboration model. And for good reason! There’s a rich set of research about the efficacy of code reviews.

In one of my favorite software books, Facts and Fallacies of Software Engineering by Robert Glass, Fact 37 points out,

Rigorous inspections can remove up to 90 percent of errors from a software product before the first test case is run.

And the best part is, that reviews are cost effective!

Furthermore, the same studies show that the cost of inspections is less than the cost of the testing that would be necessary to find the same errors.

One of my other favorite software books, Code Complete by Steve McConnell, points out that,

the average defect detection rate is only 25 percent for unit testing, 35 percent for function testing, and 45 percent for integration testing. In contrast, the average effectiveness of design and code inspections are 55 and 60 percent.

Note that McConnell is referring to evidence for the average effectiveness while Glass refers to evidence for the peak effectiveness.

The best part though, is that Code Review isn’t just useful for finding defects. It’s a great way to spread information about coding standards and conventions to others as well as a great teaching tool. I learn a lot when my peers review my code and I use it as an opportunity to teach others who submit PRs to my projects.

Effective Code Review

You’ll notice that Glass and McConnell use the term “code inspection” and not review. A lot of time, when we think of code review, we think of simply looking the code up and down a bit, making a few terse comments about obvious glaring errors, and then calling it a day.

I know I’ve been guilty of this “drive-by” code review approach. It’s especially easy to do with pull requests.

But what these gentlemen refer to is a much more thorough and rigorous approach to reviewing code. I’ve found that when I do it well, a proper code review is just as intense and mentally taxing as writing code, if not more so. I usually like to take a nap afterwards.

Here are a few tips I’ve learned over the years for doing code reviews well.

Review a reasonable amount of code at a time

This is one of the hardest tips for me to follow. When I start a review of a pull request, I am so tempted to finish it in one sitting because I’m impatient and want to get back to my own work. Also, I know that others are waiting on the review and I don’t want to hold them up.

But I try and remind myself that code review is my work! Also, a poorly done review is not much better than no review at all. When you realize that code reviews are important, you understand that it’s worth the extra time to do it well.

So I usually stop when I reach that point of review exhaustion and catch myself skipping over code. I just take a break, move onto something else, and return to it later. What better time to catch up on Archer episodes?!

Focus on the code and not the author

This has more to do with the social aspect of code review than defect finding. I try to do my best to focus my comments on the code and not the ability or the mental state of the author. For example, instead of asking “What the hell were you thinking when you wrote this?!” I’ll say, “I’m unclear about what this section of code does. Would you explain it?”.

See? Instead of attacking the author, I’m focusing on the code and my understanding of it.

Of course, it’s possible to follow this advice and still be insulting, “This code makes me want to gouge my eyes out in between my fits of vomiting.” While this sentence focuses on the code and how it makes me feel, it’s still implicitly insulting to the author. Try to avoid that.

Keep a code review checklist

A code review checklist is a really great tool for conducting an effective code review. The checklist should be a gentle reminder of common issues in code you want to review. It shouldn’t represent the only things you review, but a minimal set. You should always be engaging your brain during a review looking for things that might not be on your checklist.

I’ll be honest, as I started writing this post, I only had a mental checklist I ran through. In an effort to avoid being a hypocrite and leveling up my code review, I created a checklist gist.

My checklist includes things like:

  1. Ensure there are unit tests and review those first looking for test gaps. Unit tests are a fantastic way to grasp how code is meant to be used by others and to learn what the expected behavior is.
  2. Review arguments to methods. Make sure arguments to methods make sense and are validated. Consider what happens with boundary conditions.
  3. Look for null reference exceptions. Null references are a bitch and it’s worth looking out for them specifically.
  4. Make sure naming, formatting, etc. follow our conventions and are consistent.I like a codebase that’s fairly consistent so you know what to expect.
  5. Disposable things are disposed. Look for usages of resources that should be disposed but are not.
  6. Security.There is a whole threat and mitigation review process that falls under this bucket. I won’t go into that in this post. But do ask yourself how the code can be exploited.

I also have separate checklists for different platform specific items. For example, if I’m reviewing a WPF application, I’m looking out for cases where we might update the UI on a non-UI thread. Things like that.

Step Through The Code

You’ll note that I don’t mention making sure the code compiles and that the tests pass. I already know this through the magic of the commit status API which is displayed on our pull requests.


However, for more involved or more risky code changes, I do think it’s worthwhile to actually try the code and step through it in the debugger. Here, GitHub has your back with a relatively new feature that makes it easy to get the code for a specific pull request down to your machine.

If you have GitHub for Windows or GitHub for Mac installed and you scroll down to the bottom of any pull request, you’ll see a curious new button.


Click on that button and we’ll clone the pull request code to your local machine so you can quickly and easily try it out.

Note that in Git parlance, this is not the original pull request branch, but reference (usually named something like pr/42 where 42 is the pull request number) so you should treat it as a read-only branch. But you can always create a branch from that reference and push it to GitHub if you need to.

I often like to do this and run Resharper analysis on the code to highlight things like places where I might want to convert code to use a LINQ expression and things like that.

Sign Off On It

After a few rounds of review, when the code looks good, make sure you let the author know! Praise where praise is due is an important part of code reviews.

At GitHub, when a team is satisfied with a pull request, we tend to comment it and include the ship it squirrel emoji (:shipit:) . That indicates the review is complete, everything looks good, and you are free to ship the changes and merge it to master.

Every team is different, but on the GitHub for Windows team we tend to let the author merge the code into master after someone else signs off on the pull request.

This works well when dealing with pull requests from people who also have commit access. On my open source projects, I tend to post a thumbs up reaction gif to show my immense appreciation for their contribution. I then merge it for them.

Here’s one of my favorites for a very good contributions.

Bruce Lee gives a thumbs up

Be Good To Each Other

Many of my favorite discussions happen around code. There’s something about having working code to focus a discuss in a way that hypothetical discussions do not.

Of course, even this can break down on occasion. But for the most part, if you go into a code review with the idea of both being taught as well as teaching, good things result.

Found a typo or error? Suggest an edit! If accepted, your contribution is listed automatically here.



11 responses

  1. Avatar for bcs
    bcs October 28th, 2013

    When reviewing code, I often bring up the before and after in a diff tool and try to convert the one to the other. This works very well with re-factor heavy changes, as it allows me to deposes of "semantically null" changes first and then look at only the "real" changes later.

  2. Avatar for Matt Honeycutt
    Matt Honeycutt October 29th, 2013

    Good stuff! I'm a huge fan of code reviews.

    For my projects that aren't using Github (some of which aren't even using Git!), I'll just throw //PEER: comments in the code for any issues or questions, then commit them up to the repo. It's low-tech, but it works well.

  3. Avatar for Bruno Yudi
    Bruno Yudi October 29th, 2013

    Great post! It's very important to realize that code review is part of our job too. We tend to think that we are doing an extra job doing this. Thank you!

  4. Avatar for Alex Dancho
    Alex Dancho October 29th, 2013

    Totally agree with the vast majority of everything in this. Brilliant as always.

    Slightly off topic, but is there a reason GitHub doesn't display a commit's status on the actual commit page (eg. I feel like it'd be an incredibly helpful thing to have.

  5. Avatar for Stephen Hammond
    Stephen Hammond October 29th, 2013

    Concerning tools for code reviews you mention ReSharper. A lot of the devs I'm working with have been opposed to using certain code features (such as lambdas or async keywords). I think it's important to note that ReSharper is a good best practices tool but not necessarily a followed practice tool.

    While I believe strongly in the methods that ReSharper uses are great and it teaches me to define good programming practices, FxCop and StyleCop are the tools I fall back on for code reviews (so to avoid friction). BUT tools like NRefactory and Project Roslyn are making in roads. How do you think these tools will impact Code Review and peoples adoption of "what is a feature I should learn" and "a feature that's a nice suggestion"?

  6. Avatar for Mathias Verraes
    Mathias Verraes October 30th, 2013

    Coincidentally I blogged about patterns and anti-patterns of code reviews two days earlier
    Great post, I especially like the idea of a checklist.

  7. Avatar for Rob H
    Rob H October 30th, 2013

    "a total economic collapse of the world economy"? I think this expression needs some refactoring. ;)

  8. Avatar for Arnab Roy Chowdhury
    Arnab Roy Chowdhury October 30th, 2013

    At first I thought unit testing is best for finding errors in code. But now I found it is the code review that is more usable before unit testing. Also code review makes you learn new things by reading the code of some one else.

  9. Avatar for CaffGeek
    CaffGeek November 1st, 2013

    Did you not see the Danger Zone music video by Archer?

  10. Avatar for gfgfgfg
    gfgfgfg December 8th, 2013


  11. Avatar for ndream
    ndream June 26th, 2014

    We use for GitHub which helps with the actual reviewing of code and tracking of unresolved comments.