On Code Review

In which the author makes a strong claim regarding the importance of timely code review, and offers advice to reviewers and submitters.

Glen D Sanford
Aug 10, 2017 · 8 min read

TL;DR

  1. Code review should probably always be your top priority, and you should figure out the best way to work it into your event loop.
  2. Make your code review requests a pleasure to read.

The Premise

I believe that the single most important thing a team can do collectively to improve its throughput is to make code review a (or better yet, the) top priority for all team members. This is the most compelling and concise reason I could come up with, so I’ll emphasize it as strongly as I can:

Pending code reviews represent blocked threads of execution.

In the best case, an engineer blocked on code review is able to make progress on some other change, at the cost of (1) context switching and (2) increased memory allocation. Some engineers are great at this, but the more changes you have in the air, the more total memory is required, and if some of that gets paged out, the context switching gets expensive.

Reviewers

Make sure you know what review requests you’re on the hook for

Have a simple way to find all the pending review requests that someone (or some automated tool) went to the trouble to put your name on. Some code review systems do this better than others; personally I have ended up depending on plain old email. I won’t presume to tell you how to filter your mail or what tools to use, but make sure you have a fast way to find review requests for which you, specifically, owe time.

Shorten your event loop; when possible, be interrupt-driven

Armed with a method to quickly find out what code you need to review, try to integrate invocation of that method into your event loop, with the platonic ideal of being able to handle code review requests as they arrive. Everybody needs focused time to get their work done, and switching from a tight code/compile/test eval loop into a code review cycle is not free; But weigh the cost of these context switches against the larger switch you have to make when you’re blocked on others for code review.

Prioritize code review above other work*

*except for outages, and customer service

Expect to spend time on code review every day, throughout the day

A question that comes up in one-on-ones from time to time is “how much time should I be spending on code review?” The short answer is “as long as it takes.” Minimally, your day should begin and end with clearing your code-review backlog. Ideally you find time to clear the backlog at least once or twice in between. Inbox zero is the goal here, and I don’t think it’s unreasonable to spend one or two total hours a day on getting there. Don’t spend it all at once, though! Just as a single long review can reduce efficacy, so can many short ones in a row. If you’ve reviewed more than 500 lines of code in an given hour, take a break.

From https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/

Be thorough, each and every time

The only thing worse than no code review at all is a single good code review broken into many smaller reviews over days. It’s the submitter’s job to publish code reviews that are well-formed and and carefully crafted. It’s the reviewer’s job to be as thorough as possible in the each pass, rather than metering out their feedback in a series of partial reviews over multiple revisions of a review request. If a review drags out into many revisions, it’s sometimes because it started out sloppy, but it’s often because reviewers didn’t pay enough attention the first, second, or third time and missed things. This is something I continue to work on, and it’s hard to be consistent about. If you catch something late in a review cycle, own up to it, and apologize.

Submitters

Dot your i’s and j’s

In exchange for your reviewers making code review their top priority, make sending out a good code review request yours. Carefully review the diff before publishing (most code review tools support some form of draft mode). Make sure there’s nothing missing from the change, and nothing unnecessary. Make sure you have, to the best of your ability, written code you like and which conforms to the style guidelines (or local examples) of the project you’re working in. Don’t submit a review request and then immediately push multiple revisions as you find problems; this will confuse and bewilder your reviewers. If you don’t like what you see, discard the draft review before submitting, fix your branch, push it, and start over.

Keep review request as small as possible

Bias to small, digestible review requests. When possible, try to break down your large refactor into smaller, easier to reason about changes, which can be reviewed in sequence (or better still, orthogonally). When your review request gets bigger than about 400 lines of code, ask yourself if it can be compartmentalized. If everyone is efficient at reviewing code as it is published, there’s no advantage to batching small changes together, and there are distinct disadvantages. The most dangerous outcome of a large review request is that reviewers are unable to sustain focus over many lines, and the code isn’t reviewed well or at all.

From https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/

Write good titles and descriptions

The title of your review request should distinguish it clearly in a list of other review requests in someone’s inbox. It should not be the most recent commit on the branch. Your description should tell the story of your change. It should not be an automated list of commits. Instead, you should talk about why you’re making the change, what problem you’re solving, what code you changed, what classes you introduced, how you tested it. The description should tell the reviewers what specific pieces of the change they should take extra care in reviewing. Your reviewers will thank you for making their job easier by explaining to them what they should be looking at, and their reviews will be correspondingly more thorough.​

Acknowledge comments quickly

A tight event loop is even more important when your code is out for review; Do your best to acknowledge comments as quickly as you can, and particularly to ask for clarification if you don’t understand the comment or want to push back on it. As soon as a reviewer submits their comments, the action is back on you. If the changes requested are straightforward and small, post a new version quickly, to keep the ball rolling. The more quickly you are able to respond to comments and defects, the more likely the reviewer is to still have them paged in, and the more quickly you’ll arrive at consensus.

In closing

Thanks for getting all the way to the end of this, I really appreciate your reading it. Now get back to work.

Glen D Sanford

Written by

Software Engineer at Slack, 10x pedant, fundamentally OK guy