Code reviews: what we owe each other

2025-01-11

How do you think code reviews and pull requests should work?

A notification for a new pull request to review pops up. You open it up, you don't really know what it is about, but you see that there is no documentation, and tests are missing. You just know that this is not acceptable for quality code, so you ask for docs and tests in a comment, and make a mental note to come back to the PR once the developer has done it. No need to review closely for now.

Later, you check the reviews for one of your own pull requests. There is a lot of code, so you know it will take some time to review, but this is an important change, so you decided to make it nearly perfect before opening it and asking for reviews. You feel disheartened when you see low engagement on it, and only for a few nitpicks here and there.

You look at the rest of the work in progress. One change is still blocked because the multithreading expert pointed a bug, but is not available to help, due to their own workload. There is a pull request that was approved within 15mn of its opening, you know it's not enough for a good review, but the bugfix is urgently needed, the developer spent 2 weeks on it in isolation, and the release is cut this afternoon, so what can you do? And There is also a new feature that is nearly ready and highly anticipated. It got good feedback from the team at first, but after a few iterations, people stopped engaging and the developer stopped asking for reviews. There's jut a bit of cosmetics to do on it, 30mn tops, and it's been in that state for a month.

As software developers, we have all seen variations of these situations unfold. And I have to ask, why do we do that to ourselves?

I have seen this happening in many different contexts, mall companies, large companies, open source projects, among friends. Even high velocity teams can fall into this trap. It can begin innocently, when teams start requiring reviews as a gatekeeping measure. Sure, they won't call it that way, but maybe there's been that one huge incident that destroyed a lot of trust? Or we are getting hammered by support calls due to a lot of small bugs encountered in production? Or you just feel that overall quality is slipping. It is easy then to mandate reviews before merging code, thinking that it will increase quality and reduce issues. And it could not be more wrong.

Once code reviews are required, it demands intention and effort to avoid the following pitfalls:

• reviewing code prevents bugs, so if there's not been a review, code should not be merged
• reviewing code takes a lot of time, so I should only ask for a review when the work is done
• all code changes need a review, however small
• my pull request got no reviews, it's just a small change, I will ask a friendly colleague for a quick approve, they know I'll do the same for them another time
• if I approve code but miss a bug, then I share the responsibility for the bug, so I should not review code that I do not understand
• I just saw an unrelated bug when implementing a feature. Reviews are costly, so I'll just fix it quickly in the same pull request instead of opening a new one
• reviewing code takes time, I do not have the time to look at all the code produced by my team, I have my own code to write, meetings to attend, documents to edit, etc
• I will wait until a pull request is ready to look at it, it makes no sense to look at unfinished code that could change or get deleted before merging

If any of these thoughts ever crossed your mind, take a moment to ask yourself how you ended up there.

You can have a team full of reasonable and experienced people, and still end up in that situation. When you start replacing trust and collaboration with process, you lose what makes a team. That can come from a lot of different triggers, but code reviews are such an easy one.

So if you ever encountered this, here is a way to look at it differently. Code reviews are not a gatekeeping tool, they are a teamwork platform.

The goals of reviewing pull requests are:

• Understanding what work is happening currently
• Helping the work progress
• Catching problems early
• Teaching each other

What a review should not be:

• A way to block work from advancing. Unless there is a strong disagreement, that should be resolved in sync as soon as possible
• A quick message without deep thought or proposing help to fix it. If somebody reaches out to you for help, why not give your full attention?

If you start to see reviews as a collaboration tool, suddenly your mindset shifts. A code change or pull request is not the work of one person that others look at and judge. Every change is everybody's responsibility. Software development is a team effort.

To that end, you need to change the way you approach reviews.

• Open pull requests as soon as possible, even before you start working on them. Get people involved early, and make sure they are aware of the work in progress. Share your plan ahead of time, get feedback on it before the first line of code is written.
• Review as early as possible. It is better to catch problems before the developer has spent too much time on something. Later refactorings are more expensive. It is also much easier to read the code early when there’s not much of it, and read more as it advances, than look at the finished product, because you already understand it and saw how it evolved. You are part of the process from the beginning, each new change is easy to follow, and once the work is done, approving it is an easy formality, there should be no surprises left.
• Understand what is needed at this stage of the project. Is it a prototype where the implementation idea must be discussed? Are we targeting GA quality? What is the time constraint? What must be done right now or can be pushed later? Adapt the comments to the context.
• Check interactions with your favorite topics. Nobody knows everything, so your teammates will rely on you to help early in the design process, instead of verifying after the fact. Knowledge is meant to be shared.
• Reviewing code is a good way to learn too. Even without knowing the topic at hand, you can learn a lot by reading and asking questions. The more you do that, the easier it gets. And often, asking the dumb questions reveals the hidden assumptions and the edge cases that would have been missed otherwise. You should never say "that file was written by X, I don't know what it does". Take it instead as an opportunity to learn more, because the entire code is the entire team's responsibility, and there will definitely come a time where you need to dig into it.
• Test it manually. It is easy to stay in the review page, but it is not that much harder to check out the branch, compile it and test it. Testing it live is a very good way to understand the intent and the limits of the code.
• Pay attention to the edge cases. This is one of the most effective benefits of an outside look: spotting missing cases (branches not covered, values without bounds, etc). Check out the logic. Spend way more time on this than on discussing the easy nitpicks, because this is where the expensive bugs hide. Do it as early as possible
• Do not hesitate to jump in and help fix problems (ask first, of course). Writing code is a collaborative activity, so why should it happen with only one person writing the code and the others commenting? If you spot a huge issue, offer to help fix it. The result will be more reliable, more people will feel ownership over the code, and it will strengthen team cohesion.
• Keep the work small and flowing. Small pull requests that can be merged early are more useful than a feature omnibus:
  • merging code regularly reduces the risk of conflict (the code can be merged but made inactive)
  • smaller code is easier to review
  • cutting up the work in small chunks makes it easier to adapt the scope and schedule
  • seeing work getting merged quickly is motivating. Pull requests that wait for weeks are a drag on everybody’s mind
  • the quicker we are at reviewing and merging code, the faster we can detect and fix issues before they reach production. When work gets slower, it piles up, there’s more to look at, and there is even less time to review

This takes effort and intent, and will slow things down at first, but the payoff is worth it. You want people to work more closely together, share knowledge, and jump to help each other. Be wary of any policy change that could break this attitude, and instead use every opportunity to build trust and shared understanding.