Some Tips on Good Code Review

Posted by Matt Farmer on December 31, 2011 · 7 mins read

So, for those who may not know already, about a month ago I joined the team at OpenStudy. So far, it’s been a pretty excellent ride, and I’m excited for what is ahead. Over the past several weeks I’ve gotten to dive into learning Scala, developing Lift-powered applications, using Comet for real-time interactions, and a whole host of other excellent topics. One of the big things that was instituted shortly after my arrival was code review.

Essentially, whenever we’re working on a change that is going to be more than one or two lines we create a new branch on our git repository. We do any number of commits to our new branch, and then push our changes. Since we use Github for our repository hosting, we can then open a Pull Request to let everyone else know the code is ready to be reviewed. Then someone else will review the code, and check the code out to test it for any bugs.

Admittedly, I was apprehensive about this process at first. Mostly because I had never participated in a team where code review was done well before I came to OpenStudy. With most of my recent employers I had a lot of autonomy from other team members or supervisors. As long as my clients were happy, everyone else was happy. Usually code review only happened if someone wasn’t happy. Of course, I didn’t expect that autonomy to remain the same in the transition to a product-oriented company. It’s just not realistic, and it shouldn’t be. So I braced myself for some transitional pains when we started this workflow.

I found that the transition to the code review process at OpenStudy wasn’t nearly as painful as I expected. In fact, our code review process has been invaluable over the past month in my learning process, finding bugs in the code before the changes got merged into master, and otherwise making sure that the code we eventually published to our server was relatively polished. Nothing is worse than preparing a release, getting excited to push it to the server, and then realizing something is horribly wrong as soon as you get it live. Of course, I’ve never actually had that happen (cough cough).

So, the process of code review has been throughly beneficial to me. Although I can’t speak for Antonio and Matt, I suspect they have similar sentiments. I encourage you to strongly consider implementing a similar process in your development team as well. If you do decide to do so, I have a few ideas that you may find helpful. These are some things that I believe we’ve really done right at OpenStudy, and that have made the process more valuable for everyone involved.

First, everyone participates. Anyone who is actively coding needs to participate in the code review process, regardless of their position within the team. The truth is that being around longer, or having a more senior title, doesn’t make you immune to human error or mean your ideas will always be the best, yet I’ve heard many a horror story from friends who work under a managing programmer that doesn’t understand that. Unfortunately, my friends end up cleaning up the mess left behind by their boss more often than not.

Additionally, code review is one of those things that works best when everyone buys into it. The more buy-in that your team members have in the process, the more beneficial it will be for them, the product, and the whole company. The best way to get a team to buy into code review is for everyone to participate. So, even if you are that guy who is capable of taming two lions while writing perfectly formed assembly, you should still allow your code to be reviewed by your colleagues.

Second, we use GitHub. I was one of those people who honestly didn’t see a whole lot of value in GitHub over hosting my own private Git server. I’ve never been in a position to use their pull requests before, but it didn’t really sell me on them based on my initial assessment. However, at OpenStudy we make extensive use of what I’ve come to believe is GitHub’s golden egg: the line commenting feature. It’s possible for any one of us to single out an individual line in a file and ask a specific question about it, or make a specific suggestion. The benefit is twofold: the topic of conversation is immediately visible for anyone reading the pull request (no fumbling through a file looking for the contents of the specific line), and conversations about distinct issues or lines stay separated.

Overall, this reduces the amount of time we spend trying to communicate a specific problem or question and increases the amount of time we actually spend addressing the issue.

Finally, we encourage open communication. The idea behind open communication during the code review process is that all participants in the review converse back and forth on the suggestions that are made. This is what sets “code review” apart from “code approval” and enables the developers to learn from each other, instead of blindly issuing a list of corrections. Developing a good product is a team effort, and no line of code stands alone. Of course if your team ends up at an impasse in the conversation someone needs to be able to pull rank to keep the forward momentum going, but that should always be a last resort.

Code review may not work for every team, but it works pretty well for us. So, leave me some comment love below. I’m sorry it’s been so long since I’ve posted on here – but it’s been a really crazy month. I hope you and your family have a safe and excellent New Years, and here’s to hoping that I get back into the rhythm of regular posting in 2012.