Lunch is over and I’m sitting in Code Reviews for Fun and Profit with Alex Martelli. I really wanted to go to the Perl 6 talk, but I always end up going home disappointed, because I don’t yet have Perl 6. It’s maddening, so here I am, sitting in something that may be useful. And we’re off.
Nearly everyone agrees that code reviews are a good idea, so why aren’t they done more often? In fact, this is the very same problem we’ve had at work. We’ve been talking about code reviews for two years, but we’ve never had one.
There are some barriers to entry to doing code reviews. If revision control is not in use or automated tests aren’t being run, tackle those problems first. Also, the need for a team process is necessary, from ticket tracking to release plans.
Pair programming, that tenet of XP, is a poor substitute for code reviews. Two people working together will not magically turn one or the other into what is essentially a disinterested third party, who may catch bugs simply because they weren’t there when it was written.
Test-driven development is also a great way of coding, but not a substitute for reviews. Often for the same reasons. Tests are often just more code and the code tested is only when someone thinks to test it.
Even during a code review, a reverence for authority can get in the way of getting things done. A poor, intimidated programmer may not have the courage to criticize a more senior programmer. Instead, this can be turned around with something I use a lot myself. I like to call it, “playing dumb.” Instead of saying, “this won’t work,” ask what will happen for a suspicious case.
Socially, the only way for code reviews to work is universal buy-in. Everyone is subjected to code reviews by everyone else. No exceptions. Make them a habit, a regularly-scheduled meeting. At work, I’ve even suggested bi-weekly, or perhaps monthly, catered, lunch time code reviews. Just to get us into the habit of doing it.
Code review time should not be wasted on things such as code formatting, best practices, or test coverage. This is stupid. These are objective tasks that can be automated.
Instead, look for subjective things, which can’t be automatically found. Such as code readability, algorithmic clarity, and consistent identifier naming. Other targets for code reviews are the usual things we here over and over again as development best practices: consistent documentation that follows the internal standard, that kind of thing.
The remainder of the talk is essentially an enumeration of all the things to look for in code reviews. All of them are, at least to me, common sense. So I’m not going to spend any time writing them down. If you don’t already know them, well go find some common sense.
One thing that he recommends that I like is code reviews by e-mail. It’s an old, well-understood, and (usually) reliable tool. So why not combine e-mail with a version control system—particularly one of the newer distributed version control systems—to perform out-of-band code reviews. It actually sounds like a good idea to me, and I’ve done it at work a couple of times with code written by an intern.
What I’m starting to notice is that many of the later the recommendations for reviewing code are personal opinions of the presenter. I think the way in which code reviews are performed are highly dependent on what works best for the group reviewing code. It’s like so many things, from cameras to backup solutions: the best one is not the shiniest or the one with the most bells and whistles, it’s the one that’s actually used.
[tags]oscon, oscon08, oscon2008, programming[/tags]