Check-in Code Reviews

This is a cross-post from my blog: Game Dev Without a Cause.

There are as many ways to conduct code reviews as there are to skin a cat.  Some methods are more pleasant than feline taxidermy, some less so.  At work, we've found a code-review method that has been relatively pain-free while still providing significant benefits: Check-in Code Reviews.

In a nutshell, before a programmer checks any significant code changes into source control, we have them get a quick (usually less than 5 minute) code review from one of their colleagues.  In the process, they're expected to explain roughly how their code works and why they implemented what they did.  The reviewer than offers comments and advice (if they have any) and up the code goes to server-land.  That's pretty much it.  It's simple to the point of being trivial, but it has worked out well at work.  Here's why:

1. A Second Pair of Eyes Helps to Catch Errors Every programmer has particular problems that they are attuned to finding. Where you may catch logical errors in conditionals easily, another programmer may have a particularly strong instinct for detecting missing error handling. Having another programmer glance over your code gives them a chance to spot the sorts of errors that they are good at finding.
In addition to the scrutiny provided by another programmer, the simple act of explaining your code may give you additional insight into your own code and help you spot problems you may have otherwise overlooked. I've had more than a few cases where I've ended up saying something like: "And this line of code here... Well, that doesn't actually make a whole lot of sense here. I'll re-examine this spot and take it out if we don't need it."
Even before you call another programmer over, planning out how you'll explain your code can give you insight into what it is actually doing.

2. Spreading Domain-specific Knowledge Speaking of knowledge, this is another major benefit of check-in reviews. While it would be nice, it's very rarely the case that a project can afford to have more than one expert in any given system of the code. Check-in reviews don't make the reviewer an expert in a system, of course, but they do aid in the sharing of some of the expertise related to a particular system. Having regular reviews with the implementer of a system can also allow a reviewer to build rapport with the implementer and help them to understand how that person thinks.
In cases where the implementer of a system may become unavailable, the person who did the most check-in reviews for the system becomes a good candidate for taking over the system until the original implementer is able to return. While the reviewer will not have nearly the depth that the original implementer had, they will have a head start on understanding the system which will allow them to get up to speed faster.

3. Explaining APIs to the End-User If you're writing code for other programmers, the programmer who is most likely to use your code becomes a very good candidate for check-in reviews. In this case you can explain how the functions that make up a system should be used while also getting code reviewed. This also gives a chance for the end-user to comment on the usability of the API related to the goals that they need to accomplish. This gives them a chance to make requests that would improve their workflow even before using the code.

4. Spread Task Knowledge Around the Team A corollary benefit of check-in reviews is that more programmers are made aware of what each other is building. Not only does this help each programmer know what tasks are assigned to their counterparts, it allows them to share bits of useful code that may be too granular to be expressed in tasks. For example, a reviewer may find that person who's code they're reviewing has already made a bit flag implementation or error checking solution that they can use instead of writing themselves.

5. Help Push Borderline Decisions Over the Edge As a programmer, sometimes you may find yourself implementing code that is not the best it could be (happens to the best of us!) While the code may not be optimal, you may not be able to afford working on it any longer. Still, you wonder whether or not you should check-in such code. At times like these, having another head can help you make a decision about some sketchy (but potentially necessary) code. Having someone say: "I don't think it'll cause any problems, check it in and let's keep an eye on it," or: "I'm not comfortable with that code. It seems too brittle to be safe" can be just the push you need to make a decision one way or the other about your code.

There are many benefits to short check-in code reviews but, they are by no means cost-free. While I generally try to keep my reviews short, there is a definite cost to potentially breaking another programmer's flow. Also, shy programmers may have a harder time building up the courage to talk to someone else about their work. Still, in most cases, I think the potential loss in speed is more than made up for by bugs prevented by the additional checking and thought this process engenders.