Code reviews are overrated
…but the notion that your code will be reviewed is most certainly not. I argue that the vast majority of the benefit of code reviews comes from the belief that many other programmers will look at this code and judge you for it. There is no doubt that this belief will produce better code. The ensuing review itself, though, adds quite a bit less value to the situation, especially given its cost. This distinction is important.
There are several types of code reviews ranging from desk checks (which taken to its natural extreme becomes pair programming) to full conference room thesis-defense-style interrogations. I am certain that these types of activities catch bugs, improve interoperability, and help programmers stay on the same page… but at what cost? Code reviews are terrifyingly expensive. Compulsory and frequent meetings (even impromptu ones at a desk) destroy the flow of a work day. It crushes the ability to get things done. It will slow down the best programmers for the possibility of improving the worst. I’m not a big fan.
Managers managing
A friend of mine works for a software development house who believes in code reviews. Their managers became convinced (undoubtedly at some management training seminar) that code reviews produce better code. Given that belief, it necessarily follows that more code reviews produce even better code. Managers constantly searching for ways to quantify and measure their employees developed a fun optimization: let’s try to increase the number of eyeballs that looked at every piece of code. This rapidly became a classic case of non-technical management trying to invent ways to improve a process they do not understand. They found something they could measure that they believed correlated with code quality. Optimize it!
At first, there was formal code reviews. My suspicion says this produced a sizable increase in the quality of the code. This initial success and a touch of confirmation bias did the rest. Once a week meetings for code reviews probably isn’t too disruptive. Next, they instituted desk checks before all commits. I suspect this added virtually no benefit. Now, though, throughout the day, someone might drop in in the middle of something you were working on and request your presence at their desk for a commit. If you want to finish what you are working on, they have to wait, if you go, you lose your train of thought. It’s a lose-lose. And then, best of all, they decided that for each commit, each developer must send out an email containing the changes that were made, the files they are made to, and the diffs of the changes. All technical readers will note that this should be trivially automatic. It wasn’t.
I’m certain that the programmers quickly automated the process anyway, but think, for a moment, about what these managers must be thinking. For every commit, you must have someone else come check your code, and then manually compose an email containing a whole bunch of information. The sheer amount of time used up in code reviews, reading emails, sending emails, dealing with distraction, and “re-immersion” for programmers must have been staggering. I’d doubt that anyone could get more than an hour honest work in per day. If they followed the concept in earnest, they would spend most of the time doing meta-work (work that lets you do actual work).
The real goal
If you are a manager and do not understand software, let me give you a piece of advice: interruptions are the devil. Instead of min/maxing whatever other variable you had in mind, try minimizing disruptions. If your developers know that their code will be extensively used, maintained, and extended, you already have most of the benefits of code review. The difference is it will be done organically and without the need for constant interruptions.
Our story has a happy ending. The programmers at said development shop, of course, adapted quickly. The emails were automated and the desk checks were ignored. The formal review was impetus enough for people to write better code. The only negative consequence turned out to be the revelation from manager to employee of the former’s inadequacy.
June 2nd, 2009 at 10:13 am
It still baffles me that software companies hire “managers” that don’t know HOW their employees do what they are paid to do. I’m currently in that situation now. It has it’s advantages thought: I can spend “8 hours” working on something any competent boss would know took me an hour.
June 2nd, 2009 at 1:43 pm
The company I work in have an informal async review process. Chunks of code (bug fixes or new features) are developed in separate branches. Once you’re finished, if the change is non-trivial, you ping (e.g. on irc or gtalk) someone that can review that part of the code, and work on next thing. The pinged person reviews it when they have the time (in an hour, day, depending on the branch priority and what they’re working on).
This minimises (but doesn’t eliminate) the costly interruptions in the flow, and in my experience catches a lot of bugs (most of them trivial logic errors that somehow manage to creep in).
That said, the company is relatively small in size and we don’t have layers of managers, so this probably helps in process not becoming beaurocratic.
June 2nd, 2009 at 3:01 pm
I have the same experience as Senko: a ‘code review’ is nothing more than asking a colleague (by email or in some other ignorable way) to review a specific set of changes you checked into CVS/SVN/… The review is performed at a time of his choosing. I have found these reviews to be very helpful, catching more bugs than I would like to admit to.
June 2nd, 2009 at 6:01 pm
Well, if you really care that the code must work, you just _do_ code review. And even more. And to not be disrupted you use GIT (or something that can give you the same flow). You use emails.
Of course there is no silver bullet and of course an insance amount of just code review is just insane. So what did you proved?
June 3rd, 2009 at 2:00 am
Some thoughts on pairing from a business standpoint..
http://www.prestonlee.com/2009/06/02/prestons-business-razor-a-stakeholder-perspective-on-pair-programming/
June 3rd, 2009 at 1:10 pm
This is the only valid measurement of code quality, http://e1.simplecdn.net/slashw/measurement-of-code-quality.jpg.
http://slashweb.org/programming/25-best-programmer-webcomic-strips.html
June 3rd, 2009 at 1:52 pm
“The emails were automated….”
And that’s the key: automation can remove the downside. In other words, use automation to avoid wasting developer time *and* to avoid interrupting people. Because as you point out, those interrupts are expensive.
You can roll your own automation, or buy a tool like ours (http://smartbear.com/codecollab.php) or one of the other peer code review tools (and there are some that are open source). But the bottom line is the same: if you remove the overhead of meetings and interruptions, etc., code review pays big dividends. For more, see: http://smartbear.com/docs/CaseStudy-Cisco.pdf
June 3rd, 2009 at 7:33 pm
I think you’re right that anything we can do to make programmers more conscious of the fact that others are going to be reading their code (by extending and integrating with it! forget for the purpose of “code reviews”, who cares?) and to therefore take the extra time to make it readable is a Good Thing.
I do, however, think code reviews can be beneficial. The problem is just that we like to add so much damn baggage around them.
Drop the baggage, keep the focus on the end goal – better quality code – and keep it informal, lightweight, and something people can do on their own time (not adding distractions, as you mention).
My favorite code review tip (thanks to Uncle Bob ala Clean Code) is – if you see an issue when reviewing someone else’s code, don’t go right a paragraph about it or publicly out them on it in a conference room. Instead, just think about how it could be cleaner/less ambiguous/more correct and then CHANGE IT. Of course, then you want to let them know what you changed and why – but this is much more efficient, and gives the original coder an opportunity to learn how to make his code more correct and/or understandable to others in the future.
June 4th, 2009 at 9:16 am
IMHO code reviews are essential. Perhaps it might not work with every s/w organization but the next time I fly on the Boeing 777 I want to feel reassured that there was some formal code review process. The bottom line is that code reviews are proven to eliminate bugs and provide better code.
As for the process, well there are automation tools. Start pairing that up with static analysis and you have one hell of a bug finding machine: http://kloctalk.klocwork.com/?p=173