For the past year I have had the privilege of working closely with BridgeCare.
During this time they ran an experiment where they disabled the requirement for review on PRs before merging. It was a huge success and it has led me to question our industry's established norms of required code review.
Why do we do code review?
Let's ask a fundamental question: What is the value of code review?
This is by no means a definitive list, but here's what comes to mind for me:
- It's a form of async pair programming and seeing other's code is one way we level up
- It builds redundancy in the team as more than one person will be familiar with the code
- It catches bugs
- It catches malicious code
- It ensures code quality
- Consistency and design
- Readability and maintainability
So, yeah, code review is valuable, but it comes at a pretty substantial cost. It introduces delays into our feature development pipelines and leads to additional context-switching and juggling of tasks.
Why do we require code review before deploying?
Mandatory code review is the norm in our industry. There are exceptions of course but generally speaking when it comes to core application code the repo is going to require reviews before merging. Why is that? What are we afraid of?
As an industry we've collectively decided that the correct (and only) time to review code is right before merging/deploying. While this makes complete sense in the context of open source, it's a bit odd that we apply this same rigor to closed source. We should be able to trust internal contributors aren't going to purposefully do bad stuff. That would be career suicide.
Like I get that we want to catch mistakes and/or bugs in code before it goes out. We can't expect people to write perfect code. But does code need to be perfect before it gets deployed? Can it simply meet the requirements set by tests and linters and then get fixed up later if needed?
Consider this: code deployment doesn't make it permanent. We can go back and change things. We do it all of the time with bug fixes, hotfixes, refactors, etc. Why can't code review suggestions and revisions also happen downstream?
Non-blocking reviews
Requiring reviews on PRs can be problematic. The entire development flow comes to a screeching halt for hours (and days) because "we can't allow unreviewed code to go out". Do the benefits of requiring code review prior to deployment outweigh faster delivery, tighter feedback loops, less context switching, and fewer distractions?
Have you considered reviewing code after it's been deployed? Or have you considered pairing and reviewing code before it's in a PR? Have you considered only requiring review for non-trivial code?
As it turns out, I'm not the only person asking these questions. The idea of non-blocking code review is gaining traction:
- Non-Blocking, Continuous Code Reviews - a case study
- Optimizing the Software development process for continuous integration and flow of work
- I’ve Found Something BETTER Than Pull Requests...
- Code Reviews Should Be Optional
- No code reviews by default
- How to Perform Continuous, Non-Blocking Code Reviews
Non-blocking reviews enables the ability to keep reviews “open for comments”. And let the reviewer provide these when it best fits. - Martin Mortensen - Optimizing the Software development process for continuous integration and flow of work
But what about bugs?!? 🐞
You may be concerned that if you don't enforce code review that your code will become littered with bugs. So ask yourself this: does code review catch bugs?
Code review can catch obvious bugs and mistakes. I would argue however that tests and QA do a significantly better job of catching the real bugs.
Pull requests don't prevent bugs. Reviewing code is hard! You can catch obvious mistakes, but you won't have consistent in-depth scrutiny. Many issues reveal themselves only after extensive use, which doesn't happen during code review— Thomas Paul Mann - No code reviews by default
Will code quality suffer?
So that leaves code quality. While your linters can catch many code quality issues they aren’t going to catch bad coding patterns or provide mentorship. You still need code review for this.
But again, this doesn't mean that code review has to block merging. Code review can happen at other points in the process.
We can argue we run the risk of having bad-quality code in production. Yes, you are right. That is possible and that will happen. I do not see a problem over here. First, bad quality does not mean a bug. Code reviews are not there to catch bugs. For this, we have our automated tests and exploratory tests. — Thierry de Pauw
Moreover, from what I have observed, developers naturally opt in to waiting for review. They will know when it's appropriate to wait for a review before merging.
Look at yourself. You have a pretty good idea when your code should be reviewed. I believe you should be able to trust other developers to have this same sense also. If you can't trust your fellow developers to make the right call here... you’ve got bigger problems than code quality.
Can this work for your organization?
I believe non-blocking code review is viable given:
- You can trust your devs to wait for review when it matters
- You have decent tests and test coverage (i.e. your devs add tests for all new features)
- You have safeguards: e.g. Tests + linters must pass before merging
- You are deploying to QA environments first and not straight to production
- You actually do QA on said QA environments
- Your team is motivated to continuously improve and address tech-debt
But, honestly, these are all things you should have regardless of code review processes.
The fact that non-reviewed code can be tested or deployed in production is surprisingly the most significant benefit. We do not ever block the flow of work through the value stream. We can already obtain valuable feedback from testing, and production. We do not have to wait for a reviewer, or worse the ping-pong between reviewer and reviewee, for the feature to be available for testing or deploying into production. — Thierry de Pauw - Non-Blocking, Continuous Code Reviews - a case study
Summary of non-blocking code review
Non-blocking code review comes with a ton of benefits:
- Move faster!
- Developers feel trusted
- Less context switching
- Reviewees won’t always need to pick up another task after creating a PR
- Fewer distractions
- Reviewers have fewer PRs to review
- Features get in to QA earlier
- Bugs can be caught earlier
- Shorter feedback cycle means it’s easier for devs to get their head back into the code when issues are found
Furthermore it allows you to skip all of the virtually useless "LGTM" reviews. Save that energy for the real reviews.
If code reviews are mandatory, a lot of them have to be done. As time is limited, this will incentivize developers to do shallow code reviews which add very little value. It is better to use this time for fewer, but more valuable, deep code reviews. — Felix Roth - Code Reviews Should Be Optional
I know it's a bit of a wild idea, non-blocking reviews, but just think about it. Maybe it's worth trying 🤷.
See our case study of Bridgecare, which experimented with making code reviews optional.
