Code reviews before every commit?

First, a little cartoon about measuring code quality.

The only valid measurement of code quality: WTFs / minute

How true this is! See the original article here:
http://www.osnews.com/story/19266/WTFs_m

All developers talk about code reviews, but most of the time, it’s one of those things that there is never enough time for, and always takes a back seat to other priorities, which means it’s easily left off.

I work part time at Wikinvest.com, and we have a policy that requires that all svn commits are code reviewed first. When this policy was first introduced, I thought that this policy was Draconian and would slow things down.

I kicked and screamed inside, thinking that it was unnecessary and was going to hamper productivity. I was told that this was common practice everywhere, which I knew wasn’t true. I went along — begrudgingly. Since I’m part time, it’s not my place to try to shape development policies.

However, now that I’ve done this for a few months, I’ve come to feel differently. Here are the advantages I’ve found in having every commit code reviewed.

1. Less bugs — this is obvious. More mistakes are caught. The live site has less bugs. Duh. The evidence: Wikinvest.com has less live site bugs than any place I’ve worked. The time savings for this benefit alone more than make up for the total time spent fixing bugs.

2. Higher initial code quality — Why? Hacks will be reduced. If you know that your code is going to be reviewed by someone else, you’ll take that extra time to write it well. Your name and reputation is on the line, and it will be presented to someone else for review. You know this as you are writing the code, and you’ll be encouraged to treat the project like a high quality craftsman would. Comments will be clearer. Edge cases will be more thought through. Logical flow will be enhanced. In general, developers will take more pride in their work.

3. Enforced cross training — Every new piece of functionality, feature or fix has at least one other person that has seen it, and taken the time to understand it. This insures better code coverage across the team in case someone leaves or goes on vacation. Development managers — this will solve one of your biggest problems! When someone changes something on their own without letting anyone know, and the live site breaks, now you have at least one other person that might say "I think I know what it might be."

4. Developers learn from each other — I can attest that my code quality has improved because of code reviews. I’ve learned better practices, both by seeing others code closely, and by having others critique my code. I’ve been in several code reviews where I’ve told people about a better approach because I knew about some special function in PHP or I had previously solved the problem in a more elegant way. Big thumbs up here.

5. Coding standards and conventions are better enforced — Even with the best intentions, we can miss the way the rest of the group is doing something. Perhaps we shouldn’t have that particular variable hard coded in the script, because someone else has already moved it to a configuration file. Or perhaps there is a new standard way for getting a database connection, but you missed the e-mail that announced the new practice. Code review to the rescue! It’s always good to go along with what the group is doing and adopt standards. Get two sets of eyes on the code to make sure.

When you weigh all the above advantages, the extra time spent code reviewing for every commit is easily offset by all the above benefits, and I’m sold that this is the right approach.

Here are some best practices to consider if you are implementing in your development environment.

1. Set up a process for getting the reviews done and commits to happen quickly.
2. Spread it around! Everyone should review everyone else’s code. Discourage silos. Remember, one of the key benefits is to have developers learn from each other. If two buddies always review each others code, you aren’t getting all the benefits from spreading knowledge across the team.
3. Discourage any exceptions. The only exceptions are "trivial" changes like spacing or comments.
4. The svn commit message should contain the name of the person that did the code review.

Instituting code reviews before every commit won’t be a popular approach for everyone. People will kick and scream, like I did. The chief argument will probably be time. "It will slow us down too much". Yes, commits will be slower. But overall development will be much more efficient, and there will be less bugs. Would you rather spend less time on code reviews now or more later on bugs?

In doubt? Give it a try and see what happens to the code quality and developer engagement.

One Comment software
One Response to “Code reviews before every commit?”

1.

Pete on 25 Mar 2008 at 3:53 pm #

Well put Nick. Though I would argue that *every* commit is not necessarily the right approach in most places. We trust the author to initiate code reviews, this enables trivial changes to go unreviewed but more importantly it doesn’t hamper "commit early commit often". The developer then starts a code review for a logical piece of work, which is usually multiple commits and usually associated with an issue/story.

We think code review is such an underrated process we wrote a tool to make it easier Crucible (atlassian.com/crucible). There is another commercial tool CodeReviewer from SmartBear that is more formal than Crucible and a couple of open source alternatives as well (CodeStriker and Jupiter).