Adam Jordens

Canadian @Netflix'er. I've had a few failed startup projects over the years but keep coming back for more.

© 2017. All rights reserved.

What makes an effective code review?

18 Feb 2008

As developers, we’re all responsible for writing code in one way or another. We write it ourselves or help others write it better. Either way, code gets written and we all move on.

The rest of this short post will be focused on code reviews and what (I think) makes them efficient and beneficial.

Code Reviews (CRs)

Code review is systematic examination (often as peer review) of computer source code intended to find and fix mistakes overlooked in the initial development phase, improving both the overall quality of software and the developers’ skills. – Wikipedia

It’s not a stretch to realize that there is a strong relationship between the overall quality of software and the skills of the developers building it. Invest in developer education and you should have better software coming out the other end (at the very least you’ll have different and likely more challenging problems – assuming you’re not bottlenecked on process). In my mind, code reviews are an essential part of any development process. Regardless of whether you do them face to face or over the ‘net, the code will always come out better and the reviewers better informed.

What a Code Review isn’t

A code review isn’t an exercise in rubber stamping. It’s an opportunity for both the developer and reviewer to reach consensus while verifying that the proposed implementation or code change under review satisfies both the functional and non-functional requirements of the system. If it isn’t satisfactory, the expectation should be that any required changes will be made and the code re-submitted for review. The reviewer should hold power over what gets committed and what doesn’t.

At the company I work for, code reviews are mandated for every non-trivial commit, regardless of whether you’re a junior or senior developer. In the past our reviews have typically involved two people (developer and reviewer) and been done at the developers desk. They’re scheduled adhoc-ly with the reviewer involved generally being someone with an in-depth knowledge of the area being modified. More recently, we’ve been trying out Crucible in support of offline CRs.

It goes without saying that any code submitted for review should be atomic. Nobody likes reviewing 50+ significantly changed files on a project you spent the past 3 weeks working on. Break that down into three separate commits and everyone involved will be much happier :).

What’s Worked

A lot of things have worked. We’ve gotten progressively better over the past few years with every project making noticeable improvements to our overall development process.

Obviously the good thing for us is that code is getting reviewed before it gets committed to the repository. We’ve taken to documenting common problems and have developed a quick check list to help ensure a more thorough review. We’re generally successful with our attempts to have more senior developers (or domain experts) do reviews for junior developers, although it does vary a bit depending on the stage of development we’re in.

Alongside the code review check list, we’ve also developed a series of lessons we’ve learned the hard way. Unfortunately, software development is complex and you’re going to make mistakes along the way. It’s part of the learning process and the only way you’re going to avoid repeating them is to make sure they’re well documented and made front and center.

What Could Be Better

Scheduling of reviews has been a hot topic for us in the past. People are busy and it hasn’t always been possible to get the most appropriate person involved in a review. Part of the motivation for looking at a tool like Crucible is to enable offline code reviews at the convenience of the most appropriate reviewer. An offline CR is one in which the developer commits his/her change to a private branch and schedules a web-based review (using Crucible). Once the CR is complete, comments are sent back to the developer who then addresses them and either gets a follow-up in person review or schedules another offline one.

There are certainly benefits and drawbacks to both approaches. An offline CR loses a bit of the human touch and the emphasis on education could be diminished. Something that I’ve been experimenting with is doing an initial face-to-face review but committing to a private branch. The reviewer can then take a more in-depth look (at their leisure) at the code in a private branch before it’s actually merged into a team branch.

It’s important to have a process around code reviews. Something we’ve learned the hard way is that large commits (50+ files) don’t go over very well with reviewers (in-person or otherwise). They take a long time and diminishing returns start after you’ve been looking at code for longer than an hour or two. That being said, some large changes are unavoidable and have to be dealt with, but more often than not with a bit more decomposition you’ll have a handful of atomic commits that collectively address the same problem. It’s not surprising that most of the large change sets I’ve seen have come at the end of a particular feature or project. Unfortunately this usually leads to a series of conversations around whether we can afford to slip a completion date in order to ship code that’s up to par. Nobody likes to have these conversations.

The other thing to note is that it’s important to have documented outcomes and follow-up loops. This is more easily accomplished if you’re using a tool like Crucible, but could be as simple as a page of hand-written notes and a completed checklist.

What Makes a Good CR (my top 5)

Related Posts