Optimizing code review

The pros and cons of code review and how to make it more efficient

We’ve always prioritized code review at SharpestMinds. Even when I was the only developer on the team, we’d still have someone on the team else review and approve my code before merging it into our code-base.

A quick refresh for the non-programmers:

When a developer wants to make changes to a code-base, they create a new branch—a copy of the code-base that they can edit without modifying the original. When they feel their work is done, they submit a pull-request (PR)—a proposal to merge their branch into the original code-base and lock-in their changes. It is at the pull-request stage where code review happens.

Every now and then I’ll see a hot take on HackerNews about some team that has decided to get rid of their code review step in the interest of speed. I’m often tempted by this. Code review can be bottleneck. I talked about this in an early issue of Russell’s Index:

Consider software development. Throughput, in this case, is the rate at which code is deployed to production. At SharpestMinds, we make sure that every update to our code-base goes through a code review step before we merge and deploy. This is a bottleneck. No matter how much code I write on any given day, it still has to be tested and reviewed.

I should clarify that speed of shipping code is a vanity metric. What really matters is the speed at which we add value for our users. The bottleneck there is usually not the speed of development, but the speed of learning. However, this does require deploying code to production at some point, so it’s worth optimizing that step.

Code review can become a significant bottleneck because—unless the schedules of developer and reviewer are perfectly in-sync—it won’t be reviewed immediately. For effective code review, the reviewer should be another developer. And developers are likely to be working on maker’s schedules. Interrupting a maker to do code review as soon as a PR is submitted is not ideal. The maker’s schedule should be protected. From a previous Index:

Makers need big chunks of uninterrupted time to be productive. Switching between tasks has a high cost and it can take a while to build up momentum. Especially when writing software—you have to keep a lot in RAM at once.

So every PR has to sit and wait until a reviewer has the time and energy to get to it. This is how code review slows down throughput. And it gets worse if the reviewer requests changes. Every request from the reviewer results in another back and forth between developer and reviewer, leaving the PR as work-in-process for longer.

In my opinion, the value of code review outweighs the reduced throughput. Code review helps catch bugs, increases code quality, and helps share context with other developers. It’s also a great way for junior developers to get familiar with a new code base (by doing code review) and get guidance from mentors (by receiving code review).

But it’s still worth minimizing the lifetime of a PR. Besides slowing down throughput, long-lived branches are an anti-pattern for other reasons. A long-lived branch and the main code base will diverge over time, increasing the chances of conflicts and bugs. Plus, the developer who originally wrote the code might start forgetting the context that led to their decisions in the first place, making code review discussions more challenging.

To speed up the code review process at SharpestMinds, we do a few things:

1. Keep PRs small

The smaller the change, the quicker it will take to review. We aim to keep PRs as atomic as possible. This mean less lines of code but, perhaps more importantly, this means limiting the scope of each PR to one thing. Reviewing is easier when there are fewer concepts to keep in RAM.

One warning sign that your PR is not atomic is if the title of your PR has an “and” in it. For example, Add archive button to project form and fix bug with location input. That should be two PRs.

Atomic PRs also make the history of your code base easier to parse. This makes it easier, for example, to determine which change introduced a bug in production and revert.

2. Review your own code first

A look over of your changes before requesting review can save some back and forth with the reviewer. Maybe you accidentally left in some print statements that you used to debug (I do this all the time), maybe you missed a small error. Fix up those small things now, and you’ll save another back and forth with the reviewer.

It also helps to leave some preemptive comments on your own PR. There might be aspects of your changes that are hard to understand, or extra context that led to a decision. Try and predict where your reviewer will ask for clarification and get ahead of it. This can also save some back-and-forth.

3. You approve, you merge

For a long time, our process for merging branches was explicit—the owner of the PR should be the one to merge it into the code base. But this introduces another back-and-forth that increases the life of a PR. Once approved, the PR has to sit and wait until the owner can come and merge it. Again, this might be fine if the owner and the reviewer are in-sync, but that maker schedule has to be protected.

We’ve just recently changed our process here. You approve, you merge. No need to wait for the owner. This has had the biggest effect on the code-review bottleneck, since it guarentees one less back-and-forth for every PR.

4. Approve (with conditions)

Typically, the code-review process looks like this:

Reviewer requests a change => developer makes the change => reviewer approves and merges.

This is overkill for small, trivial changes. We can cut out a step by approving with conditions. E.g. “Approved, once you fix this small formatting inconsistency”. The process becomes:

Reviewer approves with conditions => developer makes the changes and merges.

This requires some trust between reviewer and developer. Trust that they will actually make those changes before reviewing. And trust that, if a new review was actually required, that they would request it. To implement this we had to disable a setting on Github that dismisses approvals when new code was pushed.

There is another, even quicker option here, which goes like this:

Reviewer sees a trivial error, fixes it, approves, and merges.

This is great becuase it involves zero back-and-forths between developer and reviewer. But it also requires a lot of trust. And, as my colleage Rose wisely pointed out, we would want to be careful with this approach, esspecially with new developers. It might encourage developers to not take responsability for their mistakes.

On that note, I have a PR to review.

Back to building (and reviewing).

-Russell