The risks of code review

Code (or peer) review is supposed to be a process, during development, that helps producing better software. It sounds simple enough, except it’s not.

Coding is not an exact science. Not even close! It requires a lot of subjective things of the person producing it. Things like: empathy, ability to communicate, imagination, past experiences, capacity for understanding different layers of a problem etc. These things are also the ones being employed during code review. They constitute the differences between us and let’s face it, as much as we would like to think that our differences create a better world, in reality we are hardwired mostly for war and these differences are triggers.

Of course, we do have mechanisms to regulate fighting impulses, but most of them are activated as responses to direct stimuli: voice tone, hand movement, body language in general. During code review, all these things are missing, so guess which way the “conversation” will go. Without a real conversation with the other person, all the important cues and hints are missed, making for an extremely poor conversation. This is one of the reasons internet discussions are almost always going from “Here’s my nice kitten” to “I hate your race!” in just a few replies.

This is one major reason why pair programming makes more sense.

The subjective things matter a lot for: naming (functions, variables, constants, classes…), choices for code composition, choices between imperative vs. declarative styles, choices for frameworks. This is the space where most code reviewing is taking place, unfortunately. There is a high risk of overseeing a real problem (e.g. we read way too much data from DB into memory) while getting lost in debates over the name for a local variable (e.g. “k” vs. “counter”).

We then tend to create the so called “code conventions” within teams (which btw, will always piss off a subset of the team members), that will no doubt focus on those small things. And so will the developers from then on, trying to stick to the conventions and losing the overview (can’t see the forest from the trees). These “code conventions” (I’ve seen wiki pages with dozens of conventions for a single team) are usually not a positive, common understanding of “how we should do things in this team”, but rather a list compiled by some people that “won” the fight. This can clearly be seen as subjective, as one encounters many such lists, different from one another, when switching teams.

It’s fun to see how the author’s review responses are also extremely selfish. More often then not, they try to justify mistakes that they would have clearly accepted and corrected would they have been in direct contact with the reviewer. In a best case scenario, the discussion would be taken “offline” and resolved through direct communication. In a worst case scenario, the comments thread would span for miles. Now multiply this with just the lack of context for the reviewer and the number of reviewers. So delayed delivery is another, very real risk. What I’ve seen happening a lot is a third party (usually some form of management) stepping in and forcing a compromise that will, most of the time, send local variable “k” to production and the overlooked memory leak along with it.

I don’t want to make a case against code review here, but in my experience people engaged in it need to be really good managers of other aspects of their lives too, in order to leverage its full potential. Otherwise it’s like giving the car keys to a bunch of 12-year-olds. Also, when I say pair programming, I don’t mean focusing specifically on the “driver/navigator” paradigm, but simply 2 people sitting and working together on the same code.

Leave a Reply