Reviewing code: in-person versus written

It has become a fairly widespread belief (although somewhat less often practiced), that software benefits from having other developers look at it. Such code review is designed to find flaws, seek out forgotten corner cases, improve code style, and any number of other improvements.

One of the key questions is whether to do this in writing or face to face. Most open source projects rely on written feedback, with the most two common forms being (a) send a patch to a mailing list, and get a description of what could be improved (the mozilla code review system is particularly formalized), or (b) check the code in, and other developers look at it there (if not as a deliberate effort to review it, then in the course of their own work). By contrast, a face to face review can be anything from a formal event which sits a bunch of developers in a conference room with the code on a projector, or the back-and-forth of pair programming, or anything in between ("hey, could you look at this?").

As it turns out, this morning I got two unrelated examples of how the written method can be frustrating.

The first story starts a week and a half ago. A new MIFOS developer had checked in some code. Not surprisingly for a new developer, it had a variety of problems both large and small. I sent an email describing some of the ones which seemed most important and/or easiest to find (oh, and reverted the checkin because it broke the build, but fortunately that's much easier with subversion than it would have been with CVS). Today the developer checked in a fixed version. It certainly is improved (I'm pleased to say the unit tests pass). I noticed a variety of smaller things. Now I have to figure out which ones to fix myself and which ones to complain about. In the second case am I going to wait another week and a half to get a reaction? And how do I try to calibrate how good is "good enough" and what we should worry about later? Now, those are valid questions no matter how the suggestions are delivered. And a one-week feedback loop is better than waiting many months, until the developers thought they had something which was "finished". But it is certainly harder to come to an understanding, or build up a team set of practices, if each little point is going to require a number of back-and-forth emails or phone calls.

My second example is a much smaller one, which actually makes it easier to see the point without getting hung up in larger issues of how you manage a project. A well-known columnist wrote an online article which had a typo in one of the examples. I wrote in and said "you have an extra curly brace on the last line; perhaps you copy-pasted it from a few lines up". He wrote back and said "no, the curly should be there" (thinking of the instance a few lines up, which was similar in syntax). This caused me to pause a bit. "Well, I'm pretty sure I'm right, but is it worth worrying about?" and even "how can I express this most clearly, because obviously what I said at first needed too much re-reading for a busy author to do", and so on. Well, I did write the second email ("no, the one at the end" or however I said it) and the author wrote back that I was right and fixed it (no doubt feeling at least a touch sheepish).

Not an especially big deal. But I contrast it with what would have happened if this had happened while pair programming. I would have said "oh, there's an extra curly". Perhaps I would have just taken the keyboard and removed it. But the worst case would be closer to "no, it matches the one up here" "not that curly, this curly (pointing with finger/mouse/etc)" "oh yeah, you're right (reaches for keyboard and deletes it)". Not only would it have gotten fixed faster, but with a minimum of tension (not that pair programming is immune from tension, but that's a subject for another article).