Posted 2013-05-06 05:49:00 GMT

In 2001, before I started university, I interned at a company making radio controlled heating valves: why not use code review I asked? Palpably, the quality of technical decisions in open source software like the Linux kernel was much better for discussion around direction — sometimes descending into frankly ad hominem insults but resulting at least in some degree of consideration of alternatives. On the other hand, who wants a layer of bureaucracy? And so we opted not to.

Since coming to Facebook, where code reviews are strongly encouraged and almost enforced, I've done more review than coding — about three to one — which is personally a little frustrating as writing code is more fun. But one reason I do so many reviews is that it is not always easy to get changes in: there are large swathes of the code-base, lying unmaintained, where proposed changes can go unreviewed forever and finding someone who is able to spend the time to consider the ramifications of a modification is often tricky.

What are the duties of a reviewer? There is a school of thought which suggests that these to not extend to verifying the software for correctness. I would disagree — with the exception that if the description of how the change is tested is an outright fabrication, then the reviewer is responsible for independently assessing the correctness of both the implementation and the assumptions underlying it, including a duty to insist on a proper plan for empirically observing the behaviour of the program. Beyond that, the reviewer should consider the consequences in terms of the wider ecosystem of the change (does it increase load on another system or impose technical debt in terms of fragility to subsequent changes), and should consider alternative approaches. The issue of coding style, especially superficial formatting, should not be the main focus of discussion.

The duties of the coder, the reviewee, comprise foremost a duty to ensure a proper review, which means submitting comprehensible (and therefore small) patches to a reviewer who is capable of understanding their consequences — and sometimes this means insisting on additional consideration of some subtlety that the author may have missed.

The question of how strongly opinions should be expressed in the discussion of a patch is largely a personal preference and in some open source communities vitriolic and scathing remarks are not uncommon (Linus Torvalds being infamous for this). My personal opinion is that the delivery of the message is less important than the content, and the reasoning behind it, which should be made clear. And if the reviewer expresses concerns, the onus is on the reviewee, as supplicant, to placate those, or alternatively to find another more convivial reviewer, rather than to try to bully a change through the process. However, civility and a lighthearted sense of humour are most pleasant to work with!

Sadly, in moments of highest pressure the review process is most circumvented: when the change is very large or even beyond a few hundred lines it is most time-consuming to review, so it becomes tempting to skip the process: but this is exactly when consideration of alternatives can have the greatest benefit. Similarly, when there is a very proximate deadline of some sort it is tempting to short-circuit the review, but exactly then are bugs and wrong decisions most damaging, as there is by definition little time to observe and correct them. Reviews here are most essential and I feel that an additional process requirement of a third pair of eyes might actually be beneficial.

At the end of the day, it's almost certainly easier and quicker to rewrite some code than debug it years later. Good code review means better code, better mutual understanding, better systems and therefore better morale.