One thing that people quickly realize about me is that I hate lots of things. Partly that’s because I tend to be very critical, partly because I am a little too candid for my own good, partly because here in Middle America the scale starts at “it’s okay…” when people talk about the worst thing in the world. Usually this isn’t a huge deal. I warn people, they understand. A few weeks go by and they realize they didn’t really understand, but eventually we settle on some good compromise where we each understand what we mean.
But even within that shared understanding, there are ranges. If I say I hate brain freeze, or cancer everyone nods and says “yeah, they’re the worst!”. If I say I hate leafy vegetables or Tom Hanks movies (except for Apollo 13), people kind of give me the side-eye but go along with it.
The worst is when I say I hate code reviews.
Pretty much everyone in earshot reflexively reaches for the nearest pitchfork. Then they look at each other to wordlessly agree who will be the one to engage. Then comes the usual argument:
“Come on Matt, code reviews are super useful! They catch bugs. They make sure people are writing good, legible code. They help make better code since the reviewer can offer suggestions. They help spread knowledge, both of the codebase and various tips-and-tricks that each individual knows. They can help make sure people write tests and documentation. They make sure nobody is putting backdoors in the code… How can you possibly hate that?”
That is all true.
And I hate it.
The reason I hate your code review process isn’t because of the value it provides, but because of the cost it incurs.
Take the “it catches bugs” argument for example. It’s commonly accepted that the earlier you find a bug, the cheaper it is to fix. Code reviews necessarily occur after the code has already been written. Any bug found is found after the programmer has done a bunch of work to implement the thing. Other techniques like pre-coding design discussions or (*shudder*) pair programming do a better job at identifying the sort of bugs caught in code review earlier in the process.
Beyond that, code review is a labor intensive way of finding bugs. This guide suggests 300–500 LOC/hr as a target. My workplace uses a complexity measure to estimate code review times, and generally suggests about 10 minutes for a run-of-the-mill change in any run-of-the-mill general purpose programming language. Like any other manual QA effort, the benefits are only gained once. If your goal is to find bugs, that time would almost certainly be better spent on automated quality checks like unit tests or static analysis or performance benchmarking which can provide continuous benefits as the codebase evolves.
Another common argument is code review as a learning mechanism. The idea here is that reviewing code helps spread knowledge about how to do things and how the codebase is structured. Yet most code review processes are individual affairs. A single reviewer might learn about a particular trick, or a certain class within the codebase but the knowledge is spread chaotically based on who does the reviews. Group reviews help this problem, but cost even more since everyone gets pulled into the review meeting and are forced to review at a fixed pace. At that point, why not just have dedicated training? Deliberate teaching does a better job at spreading knowledge to the whole team.
This is particularly useful for growing less experienced programmers. Often code reviews are meant to be the primary feedback mechanism for people new to the team or new to the career entirely. Yet this means that the new person needs to stumble across something beneficial (assuming they review code at all), or they need to make the mistake before a code reviewer notices and helps them. It effectively guarantees that they learn something after they need to know it. Not only is that inefficient, it’s a poor experience for someone who should be building confidence.
The biggest problem I have with code review processes is how they impact team dynamics and programmer workflow. Let’s assume that you found a typo in a comment. Being the good citizen that you are, you decide to leave things better than you found them by polishing off that debt. You know that best practices say you should keep that change small and isolated so you split it off from your main work. But your workplace almost certainly has mandatory pre-commit code reviews, meaning you can’t just ship it and get on with your day. You get to write up some nice change documentation, send it up for review, maybe chase someone down to actually review it, wait for it to be approved, rebase your in-flight work on top of it, maybe fix a few merge conflicts… (and heaven help you if your company has a change control board)
All of that is waste. It’s time you’d be better spent working on actual problems rather than placating some inflexible process. Engineers know that. And very many of them will at some point say “fuck it” and either not fix the comment or lump the changes together. It’s not just trivial changes to comments either. As soon as code goes up for review, it breaks flow. Writing up the change requires a different mindset from writing code. Working on a pending change is risky since feedback might change its direction or lead to conflicts with any work stacked on top of it. So changes get bigger to avoid the interruption. They get less isolated. The code review process governs their workflow rather than best practices or an individual’s own ideal workflow.
Some engineers invariably will mitigate the overhead by social means. They will have an “easy” reviewer handle their changes, or they will have a buddy that will sign off on things with minimal oversight. The common code review process encourages these sorts of cliques and gamesmanship. It rewards programmers who build trust through their charisma rather than through their skilled programming. It is yet another small advantage given to people who “fit in” to engineering teams.
These arguments all assume that the process is actually good and working. Reviewers are good at providing feedback, assume positive intent, review code promptly, and focus on substantive problems rather than nitpicking names or styles or not doing things how they would have done it. In my experience, that’s somewhat optimistic.
There’s more likely going to be a few inter-personal problems that come up. Some programmers will feel like they’re not trusted by their peers or their company. Some reviewers will feel like they’re not being listened to, and some reviewees will feel picked on unfairly. Some managers will ask why things haven’t been done yet, and the pressure to deliver will cut corners off even the best review process.
I’m not saying we give up. Code reviews are a useful tool in the box. They are a fantastic tool for verifying vital or squirrelly code because they’re worth the cost. They’re great at providing feedback to beginners when that is your focus, because you can tailor the reviewer and the feedback appropriately and because you can do them before the code is “done”. And they’re great in general for one programmer to ask another “hey, what do you think about this?”. But you don’t need a process for that, you need a culture of safety and collaboration.