Jeff Kesselman (a.k.a. gameguy) at Sun posted about why he doesn’t believe in code reviews. I responded taking the contrary view that code reviews can be worthwhile (even though they often aren’t). The result is excerpted over the fold so that my readers as well as his can see it and join the debate.

To understand my perspective you need to understand that most of my programming career has been spent in an industry where efficiency is king– the game industry. Efficiency of your code is important because well tuned game code provides a better user experience.

That’s called special pleading. It doesn’t work unless you can show how the special circumstances make a difference to the claim(s) under debate, and in this case that’s not likely. Gaming is not the only programming subdiscipline in which performance is critical, nor even the most difficult. Kernels, many types of embedded systems, and HPC all come to mind as examples of more challenging specialties that still don’t succumb to that false reasoning. Even if that weren’t the case, you haven’t come close to showing how reasoning based on the supposed special needs of game programmers can be generalized to all of programming.

Code review improves code quality.

This is a pretty general glittering-generality, but lets see if we can’t make it more specific. The idea here is that each coder has individual tricks and techniques they have developed on the micro-level that improve the reliability and/or performance of their code. By looking over each other’s code they share the value of these things.

That’s not the only way in which code reviews improve code quality. In addition to having their own tricks and techniques, each coder also has their own blind spots. If you’ve ever unit-tested your code, chances are that you’ve seen it pass the unit tests and then fail in later tests or in the real world. How can that happen? It’s easy when the person writing the test has the same blind spots as the person writing the code because they’re the same. Everyone has blind spots, and there’s just no substitute for a second pair of eyes to spot bugs that live there. It’s similar to the “many eyes make all bugs shallow” philosophy of open source, except that in this case it actually works because it includes even the bugs that are hidden from the casual novice’s view.

In the forest of code it is way too easy to thus spend inordinate amounts of time on things that don’t really matter and miss things that do. This is not to say that peer opinions aren’t valuable, but in my opinion they aught to be confined to code that really matters, and the best way to do that is to allow the engineer to profile and test his or her code and then ask peers for help where it is needed.

Congratulations, that’s a code review. You’re absolutely right that priority should be given to the most critical pieces of code, and also that reviews should be lightweight, but that’s only an argument against one specific style of doing reviews. Presenting it as a general argument against all code reviews is a bit of the excluded middle.

more code is ruined by “maintainence” that pushes it beyond its originally designed in limits then any thing else in computer science

If the original author writes unnecessarily obfuscated code, or the maintainer is incapable of understanding code that’s exactly as complex as it needs to be, then that’s likely to remain true, but not if both are competent. You yourself point out that understanding another’s code can be more difficult than rewriting it, and that’s true initially until you (or your customers) start finding all of the bugs that were reintroduced in the process. In the end it’s a waste, and it’s a waste characteristic of novice programmers who dream up all sorts of rationalizations for not doing something that’s hard or unpleasant. I believe Joel Spolsky has written quite eloquently about this phenomenon1, but I’ll settle for a simple question. Would you feel more comfortable shipping code to your customers that was rewritten by your laziest programmer, or that was maintained by your most diligent one? That’s often the choice you’ll have.

uniform naming conventions I honestly think are over-rated…
…Something that, for similar reasons, I think is even sillier are formatting conventions

Unfortunately, most people seem to be exposed to this kind of nit-picking early in their career, and it sours their attitude toward a valuable part of the development process. Bitching about naming and indentation and curly braces in a review is a waste of everyone’s time, but not all code reviews are like that. I’ve run many that had a specific rule about needing a clear line between every comment and present or almost-inevitable future functional flaw – and somebody to enforce that rule. Such reviews usually go quite quickly and turn out to be very productive. I’ve seen them yield measurable improvements in code quality, so if you’re the kind of guy who believes in “active test, measurement, and management” of anyone’s code base there you have it.

Usually the people who hate code reviews the most are those who quite justly always feel beaten up in them. Find out how to do code reviews right, and you might find that they’re not such a bad idea after all.

1 Yep, here it is.