I’ve seen a lot of questions recently about code review, so I thought I would write about my experiences working across a few development teams. But first, it’s important to remember that code review is but one small part of a good development process. It’s hard to give advice in general about code review without considering the whole process.
For example, some people argue that code review is a waste of time because by the time you review code, it’s already too late to change the design. The author and reviewer will argue for a bit, but by the sunk-cost fallacy the author will only commit to minor changes and move on. Some complain that code review mostly devolves into petty bike-shedding over code style and formatting. Some complain that waiting for review becomes a huge bottleneck in the development process.
I joked a few weeks ago on Twitter about reviewing large PRs, but honestly if a pull request gets this large there isn’t really a feasible way to review it all. The choice is to ask the author to break it down into smaller changes, or let it through blindly.
It’s hard to address these points in isolation, because these issues aren’t caused by bad code review, they are caused by a break-down earlier in the development process. But enough of the complaints, let’s talk about what works. Or at least, what has worked for me in the past on my most productive teams. And let’s start by talking about the broader development process.
A good development process is one that allows the team to collaborate effectively to rapidly develop and deploy code into production, and that brings all developers on the team along for that journey.
Let’s start with “collaborate effectively”. Code review isn’t inherently
Another trick we use is to submit what we call “skeleton PRs”, where we add
only the interfaces, or an API with placeholder/
We also make good use of “draft PRs”, where we submit changes as early as possible and the author actively asks for feedback on specific parts (especially any changes to the design laid out in skeleton PR). This creates a feedback loop, rather than just feedback. Once the implementation is finished, the author will clean up their commit history and then "publish" the draft, letting reviewers know to give it another look so that it can be merged.
“That sounds like a lot of PRs, doesn’t that slow you down?” I hear you ask. That brings us to our next point, “rapidly”. There’s nothing more frustrating than being blocked waiting for code review, I’ll grant you that.
The trick to getting speedy code reviews is to make the job of the reviewer
as easy as possible. If I see a pull request with dozens or hundreds of
files, I’m gonna put it off unless I know it’s an urgent urgent blocker.
The use of skeleton and draft/
It’s important also to keep the PRs as small as possible, and to make good use of a clean Git history and meaningful commit messages to make reviewing the code a pleasant experience. Thankfully my teammates are skilled with Git and use interactive rebase liberally to tidy up their PRs (especially draft PRs, which usually start off a mess) into a few atomic commits with clear messages explaining why the code is necessary and how it contributes to the PR, letting me review PRs one commit at a time.
And this leads smoothly into third point, “brings the team along for the
journey”. A good PR is educational for everybody involved. The author gets
feedback from the reviewer, and the reviewer learns from the approach of
the author. But the reviewer should also learn more about the problems that
the PR is attempting to solve by the description of the PR and the commit
messages. This gives the reviewer a better understanding not just of the
code, but of the product/
I leave you with a few of my favourite videos related to code review. A Branch in Time by Tekin Süleyman is a great talk about how to craft your Git history into something that’s easy for your collaborators to understand. Beyond PEP8 by Raymond Hettinger is a great talk about how to write code that is easy to review, without getting distracted by petty issues. And finally, Constructive Code Review by Erik Rose is a great talk about good code review etiquette.