Code Review Review
Mel Chua writes about code review tools in a recent blog post, pondering whether a software code review tool could benefit OLPC (where she’s now employed, doing QA and, knowing her, a zillion other things). I was about to just make a comment there, but I realized I have a fair bit to say, so full-fledged blog post from me it is. The only potential benefit she directly mentions is that
there’s this constant loop of feedback and revision happening with code – imagine something going around and around in a positively improving cycle – and when it’s ripe and ready, someone with privs can pluck it from that cycle and make it a commit (as opposed to a linear “go forward… get stuck here… if it doesn’t work discard it out of the waterfall completely” system).
She also links to a video of a Google Tech Talk by Guido van Rossum (of Python fame) about Google’s code review process, along with a tool he wrote to make it easier. (The open-source version can be found here.) There, he enumerates several benefits of code review:
- You can catch bugs before they make it into revision control
- Senior developers can impart knowledge to n00bs
- Senior developers can verify that said n00bs can be trusted with commit privileges (this is a big one in the open-source world)
- Team members can familiarize themselves with each other’s strengths and weaknesses
- In general, you get the benefits of pair programming without the scheduling constraints.
A while back we installed a trial version of Atlassian’s Crucible code review tool at OpenGeo to use for GeoServer development. I personally ended up using it for only two reviews: a (vast) speed improvement on the code that builds regionated hierarchies in GeoServer, and to review a patch submitted by Wayne Fang of Refractions Research. Maybe this means that constant code review is not well suited to use by smallish groups (I interact with about 4 developers who actually work on GeoServer on a regular basis, and most of the time we work pretty independently on separate parts of the project. We are geographically dispersed, so often Important Stuff happens while I am asleep.) Anyway, I’d say that of the benefits Guido mentioned, only the first couple really came into play. Interestingly enough, the KML regionating work ended up being a more-or-less complete rewrite, including a new algorithm, so familiarity with the code wasn’t that important in the end. Much of the discussion around the patch from Wayne was related to style and design concerns (does that class name really signify what it’s actually doing?) rather than behavior, so may have actually been better served by the mailing list.
A couple of other random thoughts:
- Code review is a process. Including more process means more training needed by people coming onto a project, though this can be mitigated by restricting the complicated bits to senior members, or, ideally, an automated system. I don’t think it’s a foregone conclusion that adding code review to a project’s policy will make it a better project; it should be considered against the weight in developer ‘activation energy.’ For something like GeoServer, where development is fairly process-heavy anyway, this is pretty marginal (and I’d be interested in seeing where a more serious trial of code review could get us.) For something with less dependencies, less API to get acquainted with, a quicker test suite, etc, it might be overly burdensome.
- Guido also mentioned that code review will happen whether you set aside time for it or not since any bugs that make it into the codebase will have to be fixed. It may be a better strategy to just fix the bugs that are caught in QA, directing developer attention to the most egregious bugs first. Catching bugs by inspection is not exactly bulletproof. Of course, if you’re relying on code review as a means of maintaining some minimal level of API design quality, going back after the fact is a bit tougher. But then, you can review API design without going through a line-by-line audit of the code too.
- As I mentioned earlier, I work on a pretty small team. Patches to GeoServer from developers who don’t already have commit privileges are fairly rare. I might appreciate code review tools more if I had more occasion to assess whether or not a particular change was up to par for the project I’m working on.