Code reviews are bad
Deliver better software, faster.
Yes. This is a clickbait-y title. Let me expand on it a little bit: how most development teams I know do code reviews is detrimental to delivering quality software.
A lot of questions need answering here. What’s a code review? What is quality in software? What’s “delivering”? And if code reviews are so bad, should there be an alternative? If so, what is it? Let’s explore.
What’s a code review?
A code review starts with a notification from the local version control tool that there’s a “pull request” or “merge request” that someone wants you to review. It can also be that you get a message from a colleague asking for a review, or pick up one of the work items that have ended up on your [insert agile flavour here] board’s “To Review” column.
It doesn’t matter which of these flavours kicks you off. The common thread here is that another developer has worked on something and then, before the work gets taken into operation (or “to production”), stops their work on the item and hands it over to a colleague. This can be a specific (set of) colleague(s) or it can be as general as “I’m putting it up for review from anyone that has time for it”.
Then, once a reviewer finds time, they find the piece of code the colleague made and they review it. The reviewer leaves comments for the creator of the code, to process (or reject) later. This can go back and forth a couple of times before everyone is happy. When all developers involved are happy, the change is integrated into the main branch of the application’s source code and you’re done. Happy days.
This is what a code review looks like in most teams I have worked with. I’ll call this type of code review “asynchronous and blocking”.
What is software quality?
In every environment and line of work, quality means something different. If you’re building a website to support a television show for one season and after that, the website will be thrown away, you don’t care about adding loads of new features easily. If you’re creating the software that orchestrates the money flow for a bank, you might want to make sure everything has double or triple fail-safes. I’m not going to pin you down to one of these scenarios.
The quality I’m thinking of is on a different level. On that level, a more generic definition of software quality can be found in the number of production problems, mean recovery time of production problems, time to market, and even developer morale (which in turn speeds up development and reduces developers falling ill and/or leaving the team).
What is delivering software?
This is probably the definition that’s going to incite the most debate: software is only delivered when it is in production. If software is “on acceptance, waiting for the product owner to approve”, it’s not delivered. So in my definition, if you put your code up for review from other developers, it is not delivered yet.
And as the only measure of progress is software actually delivered, this is kind of a big deal.
Code reviews are bad
The asynchronous and blocking code review described above is bad.
The asynchronous and blocking code review is a phase gate. That is to say, it blocks the flow of the feature or improvement you’re working on until an outside event, the code review, has happened. You have to stop working, wait for someone else, and then they have to wait for you again. This looks, to me, an awful lot like reintroducing a waterfall way of working. And we just got the hang of this whole agile thing, right? Right? We also just defined quality, partly, as the time to market: the time between the idea and the delivered software needed for that idea to flourish. A gate that needs to be kept, in this case the blocking code review, lengthens that time.
Let’s take a closer look at the asynchronous review process. When you finish the coding work and offer the code up for code review, you cannot work on that anymore. What do you do in the meantime? Start work on something else? Do some preliminary research into a bug report? How long will it take? An hour? A day? In teams I’ve seen over the years, it takes a colleague somewhere in between an hour and two days to find the time in their own schedule to review your code. This is a focus switch for you. For them too. Then, when your colleague is done reviewing, you have to switch back to the code you submitted. Another focus switch.
I don’t know about you but for me, a focus switch takes time. Even if it’s a 1-minute interruption, it takes me multitudes of that to return my full focus and understanding to the previous issue at hand. Going back and forth, asynchronously, about code open for review eats up time and leaves us open to miss context when we get back to the code that is in review. Your reviewer does not know why you solved the problem like that. And maybe, in a day or two, you don’t either. Your reviewer might not find the real difficult parts, or the parts that were especially tricky and thus worthy of a thorough review.
Of course, you can fix all this by leaving lots and lots of comments and instructions for review. But I believe there’s a better way that makes the asynchronous and blocking code review obsolete.
The better option: pair programming
We’re looking to reduce gates, to reduce focus switching, and we’re looking to increase understanding (and thereby quality) of code.
I acknowledge the need for a four-eyes kind of policy. I don’t think it’s a good idea to take code to production when only one pair of eyes has looked at it. That would increase the number of production problems. It also decreases the number of people understanding the code which makes it more time-consuming to fix production problems and recover from them: that’s a reduction in code quality. So skipping collaboration on code is not the solution we’re looking for.
Over the years, I’ve introduced pair programming and used it as an alternative to asynchronous and blocking code reviews. When you build code together, you use the power of two minds building the code. One on the (literal) hands-on level operating the computer, the other one keeping an eye out on the higher level and maintaining course toward the solution you’re building. You can try out and reject ideas right away, together. That sounds like a built-in code review right there. And you get automatic knowledge sharing for free.
Yes. Pair programming is hard and exhausting. It takes practice to do well. And yes. It might be that your team agrees that for some minor changes, you don’t want to bother with the four-eyes policy. That’s a valid choice for less risky changes, if a defect in your software does not mean that people die or the company loses bucket loads of money. The problem, of course, is deciding up front what a “less risky change” is. If you choose to make this distinction, it’s your professional responsibility and that of your team to actively refine your radar for “less risky changes”. Discuss it when planning the work. Don’t go looking for a silver bullet here, or an algorithm that can detect “less risky changes” for you. This is what makes you valuable as a human being in tech, and makes sure I don’t think software developers will be out of jobs anywhere in the near future.
When code reviews do make sense
There are cases where code reviews make sense, or are compulsory, or both. If you’re contributing to open source, with its inherently asynchronous nature of cooperation as most people involved will have different time zones, schedules, and productivity peaks to take into account, then pull request-based asynchronous and blocking code reviews make total sense. They fit there. If you need to pass a certain kind of security certification which requires explicit and blocking code review, there’s no alternative. That doesn’t mean it’s improving your ability to deliver quality software, though.
Additionally, you can still do non-blocking code reviews. I often take planned time with a colleague to showcase the changes we took to production already. Out of curiosity, out of care and a feeling of responsibility over the software, and out of professional interest to learn new ways of solving problems, I want to know how my colleague solved a problem. This planned code review is non-blocking: all changes are already delivered, and we explicitly picked a moment that does not interfere with our focus. It is also synchronous: it allows us to think about code together simultaneously, instead of going back and forth asynchronously. During a code review like this, we also regularly identify improvements and carry them out. That’s a big part of our responsibility as developers: to keep software quality high.
Lastly, I continually try to review all the code I see and touch. “The boy scout rule” tells me to leave the campsite (or in this case, the code) cleaner than I found it. One way a blocking code review can end up taking ages is the feeling that “if we don’t fix all the problems now, they will never be fixed later”. Instead of doubling down on your blocking code review, try and clean up code you encounter during your work, even if it’s not directly related to what you are working on. When you nurture the trust that code will be touched regularly, and improved continuously, you don’t need to bother with blocking reviews.
In summary
Choose actively working on solutions together over a pull-request-based blocking approach of ensuring code and software quality. Pair programming and a practice of continuous improvement on all code you touch will improve your software quality and delivery routine.
