Effective Code Review

Maybe you practice code review, either as a part of your open source project or as a part of your team at work, maybe you don’t yet. But if you’re working on a software project with more than one person it is, in my view, a necessary piece of a healthy workflow. The purpose of this piece is to try to convince you its valuable, and show you how to do it effectively.

This is based on my experience doing code review both as a part of my job at several different companies, as well as in various open source projects.

What

It seems only seems fair that before I try to convince you to make code review an integral part of your workflow, I precisely define what it is.

Code review is the process of having another human being read over a diff. It’s exactly like what you might do to review someone’s blog post or essay, except it’s applied to code. It’s important to note that code review is about code. Code review doesn’t mean an architecture review, a system design review, or anything like that.

Why

Why should you do code review? It’s got a few benefits:

How

So now that I’ve, hopefully, convinced you to make code review a part of your workflow how do you put it into practice?

First, a few ground rules:

So how do you start? First, get yourself a system. Phabricator, Github’s pull requests, and Gerrit are the three systems I’ve used, any of them will work fine. The major benefit of having a tool (over just mailing patches around) is that it’ll keep track of the history of reviews, and will let you easily do commenting on a line-by-line basis.

You can either have patch authors land their changes once they’re approved, or you can have the reviewer merge a change once it’s approved. Either system works fine.

As a patch author

Patch authors only have a few responsibilities (besides writing the patch itself!).

First, they need to express what the patch does, and why, clearly.

Second, they need to keep their changes small. Studies have shown that beyond 200-400 lines of diff, patch review efficacy trails off 1. You want to keep your patches small so they can be effectively reviewed.

It’s also important to remember that code review is a collaborative feedback process if you disagree with a review note you should start a conversation about it, don’t just ignore it, or implement it even though you disagree.

As a review

As a patch reviewer, you’re going to be looking for a few things, I recommend reviewing for these attributes in this order:

You’re going to want to start at intent and work your way down. The reason for this is that if you start giving feedback on variable names, and other small details (which are the easiest to notice), you’re going to be less likely to notice that the entire patch is in the wrong place! Or that you didn’t want the patch in the first place!

Doing reviews on concepts and architecture is harder than reviewing individual lines of code, that’s why it’s important to force yourself to start there.

There are three different types of review elements:

It’s important to note which type of feedback each comment you leave is (if it’s not already obvious).

Conclusion

Code review is an important part of a healthy engineering culture and workflow. Hopefully, this post has given you an idea of either how to implement it for your team, or how to improve your existing workflow.


  1. http://www.ibm.com/developerworks/rational/library/11-proven-practices-for-peer-review/ ↩︎