Quick fixes to your code review workflow

Code review is often done in a very ad hoc manner, and this tends to result in pain points for a lot of software development teams. Here are some quick fixes for common problems I often end up suggesting, where slightly improving the structure of how you do code review can significantly improve the code review experience.

None of these are super complicated techniques, and none of these will definitely work 100% of the time. Instead they are a collection of easy things that are usually worth trying that will, ideally, solve some common problems. They also tend to work well together.

Most of these will assume you’re using GitHub, as that’s the system I’m most familiar with, but GitLab is basically identical in the relevant ways, and you can easily adapt most of these to other systems where I’ve used GitHub specific terms.

Simplify the decision of when to review

The quickest fix for code review is to actually do code review on time. Here’s how:

1. When people submit a pull request for review, they should always assign a reviewer.
2. If you have any pull requests assigned to you, you should start your work day by doing code review before you get on to any of your other work. You can do code review at other times if you happen to be free and see it needs doing, but you must do your code review first thing.
3. If, for whatever reason, you need something reviewed sooner than tomorrow morning, drop a message on slack or your local equivalent explaining why it is urgent and requesting review from anyone available.

This is a nice easy intervention that solves the biggest problem most people have with code review, which is long latency between submitting code for review and it being actually reviewed. This is frustrating for everyone involved, and results in people having forgotten much of the context from when it was first reviewed.

There are a bunch of variations on this you can try depending on your preferences. This leaves code to wait until the next morning, which isn’t the tightest feedback loop but minimises interruption. Another workflow you can try is the following:

1. When you submit a pull request for review, post a message in a dedicated slack channel announcing it, with one sentence explaining what it is, and its urgency and difficulty level (e.g. “I fixed Issue #732 which was preventing some users to log in. This is pretty urgent, but it’s also a one line fix. Can someone please review ASAP?” or “I refactored the asset deletion workflow. It’s kinda big. Non-urgent except that it needs to be merged before anyone can work on Issue #801”). Anyone who is notices and is free to do code review can respond saying “I’ll review it” and review it immediately.
2. In morning standup, go through all the unreviewed pull requests and assign them, preferably to people who haven’t done much review recently.
3. Those people should do their code review before going on to any other work.

This will tend to result in a less even distribution of code review because it still relies on people paying attention, and it’s potentially more disruptive to the reviewer’s workday, but it will shorten the time it takes for code to get reviewed.

Which you prefer is a matter of taste and priorities. Sometimes one will be better, sometimes the other, sometimes a third way will fit into your workflow better. The point is not that there is a single correct system, but that any system like this is better than the ad hoc ones that are common.

Implementing simple rules like this speed the movement of code through the review pipeline, and make everything better as a result. Rather than relying on people getting around to it and making it easy to procrastinate, making these decisions up front helps people coordinate in a way that makes everyone’s life better.

Distinguish comments from requests

When you’re reviewing you’re going to notice things that are not important or don’t need changes. e.g. design decisions where the way they’ve done it is fine but you would have done it differently, or minor improvements in how code could be structured.

It’s natural, and indeed helpful, to comment on these. The problem is what you fail to distinguish these comments from requests for changes, you create a great deal of unnecessary work for everyone, because people either have to justify to you why they’re not going to make the change, or end up making the change, and either is slow and effortful.

The solution is to be very clear about the difference and preface all of your comments with “This is not a request for changes, but FYI…” or words to that effect. People will sometimes change the thing anyway if they go “Oh yeah that is a better idea”, people will also sometimes respond explaining why they didn’t do it that way and that’s usually actively good, but most of the time they should just feel free to mark the comment as resolved without further discussion or action.

Use pair reviews

Regardless of what you think of pair programming, pair reviews are great. Rather than having one person do the code review, have two people do it and discuss the code together. You will get a much greater shared understanding of the code this way, and will often catch things you would otherwise have missed.

There are two ways to use pair reviews that I’d expect to work particularly well:

1. Pairing between the reviewer and the original author of the code, so that you can have review discussions in real time. “Why did you do it this way?” is so much better as a spoken question than a written one.
2. Pairing between a junior reviewer and a more senior one, so that the former can learn from the latter, and the latter can take advantage of the former’s fresh eyes to better understand what’s confusing (everything makes more sense once you’re used to it).

I don’t recommend pairing on everything - it’s high overhead, so for trivial code reviews it’s not worth it, but for complicated code reviews it’s usually worth it. In particular if you’re having a lot of back and forth between the reviewer and the submitter, it’s almost certainly worth hopping on a call or going over to the submitter’s desk and doing a synchronous code review rather than.

Use more and more junior reviewers

If you’re currently using a system of people choosing when to pick up code to review themselves, chances are a disproportionate amount of the review burden falls on a small number of people. Even if you’re not, typically a few senior developers will do most of the review because they “know the system best”. Either way, more junior developers end up not doing review. This does them a disservice, by depriving them of opportunities to learn the code better, and also does the more senior developers a disservice - because they take on a disproportionate share of the code review work.

If you find yourself in this situation, try assigning code review at random - literally just pick a random available developer to do your review.

This will absolutely result in some people reviewing things they’re not ready for, which you can solve with the pair review technique. When someone ends up reviewing something they don’t feel ready to review, they can request help from someone who does understand it, and use that as an opportunity to learn.

Doing it this way will teach more junior developers much more about the code, and about how to review, than they would otherwise learn, and will share the burden more evenly among your team.

Teach people how to do code review

Almost nobody is ever taught how to do code review, which is part of what causes people to want to put off doing them.

Here is a very basic guide to how to do code review:

1. First, read the diff from start to finish, without writing any comments.
2. Now, starting again from the beginning, go through and comment with anything you don’t understand.
3. Finally, do a third pass and add any comments of things you’d do differently, or make any requests for changes.

If at this point you’re not confident enough in your understanding of the code that you’d have caught anything you needed to catch, neither approve nor reject it - instead either wait for responses to your questions or if you think it’s useful, request a second review from someone more familiar with the code.

Otherwise, accept it if there are no things that need changing, or if the things that need changing are so trivial that you don’t think it needs a second review afterwards (if you’re doing that, signal it clearly by saying e.g. “I’ve made some requests for changes. Please make those but feel free to merge it when you’re done”), or reject it with requested changes if you’ll need to take a look at any work that is done in response to your comments.

You don’t have to (and, eventually, shouldn’t) stick to exactly this routine, but it’s worth practicing doing it for a bit this way if you’ve not done it before, and it’s a good starting point if you’re not sure how to review code.

If you want to learn more about how to do code review, I’d recommend the book “Best Kept Secrets of Peer Code Review”, which is very much written for a different era but still very good, but most importantly I’d recommend doing pair review and learning from your coworkers.

Designated second reviewers

If you’ve got a problem where knowledge has become too concentrated and only a few people can do review, you can combine all of the above to start to push back against that by designating certain people as second review only. This can be across the board, or specific to a particular area. e.g. say Kim wrote the asset management code and as such is the most competent person to do review on it. You can implement a system where Kim is banned from being assigned review for the asset management code.

Instead, the review is assigned to anyone but Kim. Kim’s job in this space is only to teach other people to review that code.

When someone reviews the code, they have several options:

1. They can decide that they are not competent to review it on their own, and they can request Kim’s aid on a pair review.
2. They can do the review and then ping Kim to ask for a rubber stamp on the review. Kim then looks over the code and the review and may not comment on it, but may request to do further pair reviewing with the original reviewer to point out anything they might have missed.
3. They can declare a review so fully within their competencies (either because it’s trivial or because they’ve now learned enough about the system) that Kim doesn’t even need to look it over.

This will substantially increase collective understanding about important parts of your code, and reduce the burden on your most senior developers, allowing them to focus on other things.

Decide what review is actually for

This is probably the hardest of the quick fixes, in that it requires actually talking to each other and coming to something of a consensus, but it’s important to have a conversation as a team in which you decide what code review is actually for, and what it’s not for.

A bare minimum review should probably:

1. Ensure that the reviewer has understood the code well enough that they could explain what it’s doing and make changes to it if necessary.
2. Catch any really obvious errors or design problems in the code.

Most reviews probably should not include extensive requests for changes to code style, etc. I’m personally a huge fan of autoformatting, and if your language has good autoformatting (e.g. black for Python is great, rubocop for Ruby is bad enough that every time autoformatting comes up someone mentions how much they hate it), and adopting it is not going to cause you endless fights, you should probably just implement autoformatting for your code and never argue about style again, but this isn’t always a popular move.

Other reasonable priorities might include e.g.

1. Checking that any user-facing APIs make sense and are not going to cause backwards compatibility problems.
2. Checking that code is adequately tested.

Exactly how much you want a review to check for correctness is going to depend a lot on what it is you do. If you’re implementing a social network for dogs, you probably don’t need to be that careful. If you’re a finance company, an extensive review that looks for errors with a fine toothed comb. Most people are somewhere in between.

One way to think about this is in terms of the two error rates model. You can reject a pull request that would have been fine to merge, or you can merge a pull request with errors that reach production. The only way to avoid the latter is to never merge a pull request, the only way to avoid the former is to merge every pull request without review.

This comes down to a curve where the more effort you put in, the fewer bugs reach production. If bugs are relatively cheap, the cost of the effort rapidly dominates. If bugs are very expensive (either because they are hard to fix, or they have very expensive consequences), more cost is warranted.

I can’t give you a simple rule that tells you how much you need your code review needs to do, the point is just that you should think about what this is and all be on the same page as to what that is.

Whatever you decide, I recommend writing it down somewhere in a code review guide, that can serve as a living document for how code review is to be done. Here’s the one we use on Hypothesis for reference.

Adopt a collaborative attitude

This is maybe not a “quick” fix in that changing attitudes is hard, but it’s worth making sure everyone is aware of this: People should go into reviews with a fundamentally collaborative attitude. Your job as a reviewer is not to say no to anything that might be a problem, your job is to work together with the original submitter to produce a better product together. You are not acting as a gatekeeper, you are acting as an assistant and teacher to help the original person produce better code.

In general, when doing review it’s worth thinking “Does this actually matter enough to block merging?” when you request a change and if not, downgrade it to a comment.

This is also an area where pair reviews help a lot, as it’s easy for written reviews to get very combative due to the absence of tone from written text. If discussion around something is getting heated, take a deep breath, remember you’re both on the same side, and try switching to doing it as a pair review with the submitter.

Actually figure out what’s wrong with your code review

These are just some quick fixes to common problems. You likely have other problems, most places do.

You can just fix those too though. Code review is such a self-contained problem that it’s not actually very hard to improve on (except when your code review problems are actually just broader interpersonal problems showing up in code review), but for some reason it’s particularly prone to people just treating its problems as an immutable fact of life.

If you’d like help figuring out what those problems are and some things to try to fix them, I’d love to talk to you about it. You can drop me an email at david@drmaciver.com or book a free consulting intro call.