Programmers like writing code but few love reviewing it. Although code review is mandatory at many companies, enjoying it is not. Here are some tips I’ve accumulated for getting people to review your code. The underlying idea behind these suggestions is that the person asking for review should spend extra time and effort making the pull request easy to review. In general, you can do this by discussing code changes beforehand, making them small, and describing them clearly.
At this point you may be wondering who died and made me king of code review (spoiler: nobody). This advice is based on my experience doing code review for other engineers at Twitter. I’ve reviewed thousands of pull requests, posted hundreds of my own, and observed what works and doesn’t across several teams. Some of the tips may apply to pull requests to open-source projects, but I don’t have much experience there so no guarantees.
I primarily use Phabricator and ReviewBoard, but I use the term “pull request” because I think that’s a well understood term for code proposed for review.
Plan the change before you make a pull request
If you talk to the people who own code before you make a change, they’ll be more likely to review it. This makes sense purely from a social perspective: they become invested in your change and doing a code review is just the final step in the process. You’ll save time in review because these people will already have some context on what you’re trying to do. You may even save time before review because you can consider different designs before you implement one.
The problem with skipping this step is that it’s important to separate the design of the change from the implementation. Once you post code for review you generally have a strong bias towards the design that you just implemented. It’s hard to hear “start over” and it’s hard for reviewers to say it as well.
Pick reviewers who are relevant to the change
Figure out why are you asking people to review this code.
- Is it something they worked on?
- Is it related to something they are working on?
- Do you think they understand the thing you’re changing?
If the answer to these questions is no, find better people to review your change.
Tell reviewers what is going on
Write a good summary and description of the change. Long is not the same as good; absent is usually not good. Reviewers need to understand the context of the pull request. Explain why you are making this change. Reading through the commits associated with the request usually doesn’t say enough. If there is a bug, issue, or ticket that provides context for the change, link to it.
Ideally you have written clear, readable code with adequate documentation, but that doesn’t necessarily get you off the hook here. How your change does what it says it does may still not be obvious. Give your readers a guide. What parts of the change should they look at first? What part is the most important? For example, “The main change is adding a UTF-8 reader to class XYZ. Everything else is updating callers to use the new method.” This focuses readers’ attention on the meat of the change immediately.
You may find it helpful to write the description of your pull request while tests are running, or code is compiling, or another time where you would otherwise check email. I often keep a running description of the change open while I am writing the code. If I make a decision that I think will strike reviewers as unusual, I add a brief explanation to that doc and use to write the pull request.
Finagle uses a Problem/Solution format for pull requests that I find pleasant. It’s also be fun to misuse on occasion. I don’t recommend that, but I do plenty of things I don’t recommend.
Make the change as small as possible while still being understandable
Sometimes fixing a bug or creating a new feature requires changes to a dozen-odd files. This alone can be tricky to follow before you mix in other refactorings, clean-ups, and changes. Fixing unrelated things makes it harder to understand the pull request as a whole. Correcting a typo here or there is fine; fixing a different bug, or a heavy refactoring is not. (Teams will, of course, have different tolerances for this, but inasmuch as possible it’s nice to separate review of these parts.)
Even if you have a branch where you change a bunch of related things, you may want to extract isolated parts that can be reviewed and merged independently. Aim for a change that has a single, well-scoped answer to the question “What does this change do?”. Note that this is more about the change being conceptually small rather than small in the actual number of files modified. If you change a class and have to update usages in 50 other files, that might still count as small.
Of course there are caveats: having 20 small pull requests, each building on the previous, isn’t ideal either so you have to strike some balance between size and frequency. Sometimes splitting things up makes it harder to understand. Rely on your reviewers for feedback about how they prefer changes.
Send your pull request when it’s ready to review
Is your change actually ready to merge when reviewers OK it? Have you verified that the feature you have added works, or that the bug you fixed is actually fixed? Does the code compile? Do tests and linters pass? If not, you are going to waste reviewers’ time when you have to change things and ask for another review. Some of these checks can be automated—maybe tests are run against your branch; use a checklist for ones that can’t. (One obvious exception to this is an RFC-style pull request where you are seeking input before you implement everything—one way to “Plan the change”).
Once you have enough feedback from reviewers and have addressed the relevant issues, don’t keep updating the request with new changes. Merge it! It’s time for a new branch.
Closing thoughts
Not all changes need to follow these tips. You probably don’t need peer buy-in before you update some documentation, you may not have time to provide a review guide for an emergency fix, and sometimes it’s just really convenient to lump a few changes together. In general, though, I find that discussing changes ahead of time, keeping them small, and connecting the dots for your readers is worthwhile. Going the extra mile to help people reviewing your pull requests will result in faster turnaround, more focused feedback, and happier teammates. No guarantees, but it’s possible they’ll even enjoy it.
Thanks to Goran Peretin and Sarah Brown for reviewing this post and their helpful suggestions. Cross-posted at Medium.
Comments