We all have to do them, so do them well
Code review is just as important as writing code
If code/pull request (PR) reviews aren’t timely, it’s costly to team productivity. Also, the longer a PR languishes, the more it gets out of sync with the target branch and thus harder to merge. And then everyone’s motivation to review/merge diminishes.
The clear lesson: stay on top of your code reviews. My personal rule of thumb is that all PRs should get an initial review within 24 hours of being created.
Make smaller PRs
Smaller PRs are inherently virtuous for a number of reasons:
- They’re less taxing on the reviewer
- They’re more likely to get reviewed faster
- They’re more likely to be reviewed thoroughly
Creating smaller PRs often requires some discipline to think about how to break up the work into easily reviewable chunks, especially while developing a large feature. However, this is itself also a good thing because it provides regular checkpoints to assess your progress and approach instead of finding out that your approach was wrong once the all the work is done.
For the sake of simplicity, let’s say that a small PR only contains changes in about 10 files.
Establish a review cadence
We all know that context switching is bad and having to stop what you’re doing to review a PR can be very disruptive. The solution: establish a regular cadence for reviewing PRs so that you’re not interrupting your focused time.
Personally I do code reviews at the beginning and end of the day. I find that reviewing other people’s work at the start of the day gets me ready for my own and finishing the day with code review is a nice way to unwind.
Use a PR template
This might only be applicable for Github, but a PR template encourages consistency in PR contents, which facilitates easier review.
Take a look at this PR template from my project, Cumulus.
Review PR status and assign reviews in scrums
Reviewing PR statuses in scrums creates visibility so that PRs don’t get forgotten about. However, by itself this practice doesn’t guarantee that reviews happen.
Thus, I suggest that during scrum team members should be assigned to review open PRs so that they don’t just remain “someone else’s problem”. Ideally, team members self-assign to do the reviews.
Use tags to identify PR status
This also may only be applicable for Github, but tags make it really easy to get an “at a glance” summary of which PRs need attention and which don’t.
This is how it looks on the Cumulus project:
Embrace pair/team reviews
This insight has been a revelation to me. I previously thought that doing code review with multiple people was just wasting other people’s time. Boy was I wrong.
The old adage that “two heads are better than one” applies here. Other people will always see things that you don’t, so pair reviews at least increase the chance of review thoroughness. And it can be useful to talk through feedback that you’re not sure about giving/receiving.
Team reviews have all the benefits of pair reviews, but the biggest additional benefit is that they can be a great venue for knowledge sharing. This helps avoid information silos where each team member only understands the parts of the system that they’ve worked on. However, there is a clear cost of development time for team reviews, so I don’t necessarily advocate doing them for every PR (though some teams do).
Focus on what matters
Most important:
- Does the PR address the feature request/bug?
- Are the changes well tested (unit, integration, etc)? Code coverage is somewhat helpful here, but not definitive
- Performance concerns (but not micro-optimizations)
- Security
- Documentation
Less important
- Code style
I realize that code style is a controversial topic and I do think that projects should have strict code formatting/linting standards. But I don’t think that PR review is the right time and place to debate them. Code standards should be established at the beginning of the project. And if a team member feels strongly that they need to change, then that suggestion should be pulled out to a team level discussion. Otherwise code style debates during PR review often devolve into pointless bikeshedding.
Give clear feedback
Consider these two pieces of feedback:
I feel like the parallelism here could be improved
vs
Have you considered using Promise.all to run these operations in parallel?
The latter is definitively more clear and actionable to the code author. Whenever possible, be explicit about the changes that you want the code author to make.
