Code Reviews Are Greatly Overrated

Peer-reviewing code is a de facto standard in today’s software development practice. Code reviews are considered crucial by many teams. In my opinion, based on my experience, they are greatly overrated.

Note however that I am only pointing my finger at code reviews as a formal process and the various ways this process is implemented in most organizations; I think the benefits normally ascribed to code reviews can, and must, be achieved in a different way. I will explain how.

The reason behind code reviews is simple: you want a fresh set of eyes to look at your code, in search of potential mistakes, to validate your implementation and ensure that you are building the right thing right.

All good in theory. Very flawed (and dangerous) in practice.

In some teams, code reviews are done by sitting next to each other and “rubber-ducking” the changes. Other teams rely on pull request mechanisms offered by source code hosting systems such as GitHub, GitLab or BitBucket. Others more may use specific tools like Crucible or Gerritt.

I’ve worked in teams where all those approaches were adopted.

I believe that code reviews are overrated because people tend to assign the wrong responsibilities to them, therefore expecting the wrong outcomes from them.

According to Wikipedia, some of the benefits of code reviews supposedly are:

• Better code quality — improve internal code quality and maintainability (readability, uniformity, understandability, …)

My objection: if you work in a team where code quality (e.g. readability, modularity, etc..) is guaranteed via code reviews, then you have a big problem. Code quality must be enforced through the application of principles like SOLID, GRASP, etc…, as well as methodologies like TDD/ATDD. That has to be done during development, not as a post-development phase.

• Finding defects — improve quality regarding external aspects, especially correctness, but also find performance problems, security vulnerabilities, injected malware, …

My objection: this is probably the worst usage of code reviews. You are not supposed to use code reviews for finding defects. Forget it. I’ve seen countless discussions among developers along the lines of “why didn’t we spot this bug during that code review??”. I’ve seen issues raised in Agile retrospectives to ensure that code reviews became more effective in that sense. Useless. Defects must be staved off by tests (written before the code). Correctness must be assessed via adherence to acceptance criteria. Performance problems must be discovered by performance tests. Security vulnerabilities must be discovered by static code analysis and penetration testing tools. Trusting the ability of human eyes to spot bugs (especially related to security) is chimerical and dangerous. Humans often miss the most obvious blunders, nevermind deep, subtle intricacies.

• Learning/Knowledge transfer — helps in transferring knowledge about the codebase, solution approaches, expectations regarding quality, etc; both to the reviewers as well as to the author

My objection: transferring knowledge via code reviews is too difficult, when not impossible. Code reviews can only reveal the hows, not the whys. Explaining the reason behind a set of changes requires complex human interaction… and time. That interaction should not happen after development but during development. Solution approaches should be discussed before/during development, quality should be guaranteed by tests, whilst knowledge should be transferred in dedicated meetings.

• Finding better solutions — generate ideas for new and better solutions and ideas that transcend the specific code at hand.

My objection: code reviews happen too late for this principle to be effective. Coding the worse solution first and then spending time discussing a better one looks like a waste of time even to a 3-year-old. Again, solutions must be discussed before/during development, not after.

• Complying to QA guidelines — Code reviews are mandatory in some contexts, e.g., air traffic software

My objection: code reviews are mandatory in some contexts due to the assumption that all before-mentioned benefits stand, which, in my opinion, is not the case.

The Problem With Code Reviews

• They are a Continuous Integration anti-pattern and do not encourage the right code rewrite process
  Today’s teams that want to be high-performing should be applying trunk-based development. If you want to stay competitive, Continuous Integration is a must. Code reviews put gates in the middle of a continuous stream of work, stopping the flow for too long. When pull requests get stuck for days, the risk of merge conflicts increases exponentially, developers start getting frantic, they rush others to review, and they beg(!) approvals. Reviewers, urged to unblock them, will then hasten, not focus on the code, and be reluctant to raise legitimate points that might cause code rewrite and delays. The best thing that can happen is that the better solution presented by a reviewer is parked on a tech debt ticket (does “let’s leave it like that for now, we’ll fix it later” ring a bell?), stored “in the bottom of a locked filing cabinet stuck in a disused lavatory with a sign on the door saying ‘Beware of the Leopard.” and (like most tech debt) forever forgotten.
• They are often too long and confusing
  Developers are often not disciplined enough to segregate sets of changes depending on contexts and importance. For example, they tend to put minor refactorings and code clean-ups as well as major functionality changes into the same pull request. The result is that PRs often touch too many files, pissing approvers off, who will not have the time nor will to review that gigantic chunk of work. They will most likely skim through the list of files, paying little attention to the relevant changes — also because difficult to identify within the mass.
• They generate interminable debates (flames?) about code quality
  Some developers, especially experienced ones, are so obnoxiously fastidious about code quality to cause a lot of frustration in others, who will therefore be reluctant to choose them as reviewers again. Obsessive fixation on code quality is detrimental, it may generate long, brutal, useless flames and damage the atmosphere in the team. No code can be 100% pure. Splitting hairs whether a developer should be using a ternary operator or the latest cool method in the Apache Commons library is not important.
• They are created for every set of changes
  Source code hosting systems are often configured so that no code change can be merged into the main trunk of development if not coming from a pull request. That means that every set of changes needs to be reviewed. That should not be the case. Some sets of changes are definitely less important than others. For example, there are minor refactorings or code clean-up tasks that, in a mature team, should not require approvals. They could (and should) go straight into master.

How To Get The Same Benefits?

How can we then achieve the same benefits we want from code reviews without a formal peer-review process?

My suggestions:

• Embrace pair programming or, even better, mob programming, as a standard development practice. Two or more developers working on the same functionality don’t need a further set of eyes to validate changes.
• Trust developers: if you hire competent developers, then you have to trust their competence. Avoid mandatory validation steps, remove PRs as necessary conditions to merge into master. Allow them to go straight into the main trunk of development, as long as they feel confident that their code is in good shape and acceptance-criteria-driven tests pass.
• Enforce test-driven development and acceptance test-driven development. Now.
• Embrace trunk-based development. Now.
• Lead by example. If you are one of the most experienced developers in the team, invest some time disseminating your knowledge about the best development practices. Show your colleagues why a specific technique is better with concrete examples.
• Automate, automate, automate: avoid manual processes, automate everything. Automate all quality gates, security scans, performance tests, etc… and shift-left them in your CI pipeline. Being able to capture flaws as early as possible is crucial to being truly Agile and high-performing, as a team.

Update (12/11/2020)
This article has received quite a few reactions after I published it on LinkedIn. I admit it’s rather opinionated, so I don’t expect everyone to agree with me.
It’s been however interesting to see how split the development world is on this matter: typically it’s the most experienced devs who agree with me on this subject, whilst the less experienced still see a lot of value in code reviews. That’s because they learn a lot from them, of course. The sad reality of software development however is that many teams do not really collaborate (they think they do). Often, they act more like groups of individuals working independently and occasionally touching base, so code reviews are the only opportunity for junior devs to have real technical exchanges. No wonder they find them useful. Anyone looking at the big picture, though, would spot the fact that they never really sit together and talk. Once you fix those collaboration issues, code reviews lose importance. My opinion is that most of the comments in code reviews would be resolved by a 5 min chat at the start of the work, maybe with 1–2 hours of pairing.