1,895 reads

Code Review for Real People

by Steve KonvesJuly 7th, 2017

Too Long; Didn't Read

Code review is one of those software disciplines that incurs a small short-term cost in order to yield a large long-term gain. But the positive effects of code review are not only deferred, they only manifest themselves when the review is done well. The negative effects, however, are immediate and guaranteed: it consumes precious developer time right now.

Companies Mentioned

featured image - Code Review for Real People

“Peer code reviews are the single biggest thing you can do to improve your code” — Jeff Attwood

Rationale

The flip side of this coin is poor code quality. Allowing quality to slip yields a small short-term gain while incurring large long-term costs. By cashing in on short-term savings, the desire to “get this out the door” increases the cost to get the next thing out the door. Code is less readable, bugs are more expensive to fix, and the team as a whole gradually becomes less equipped to work in the codebase.

With this tension between short- and long-term tradeoffs, it’s no surprise that while code review is good, it isn’t done more often. The solution is to understand what makes code review work well and then adopt a practical approach that minimizes short-term cost while maximizing long-term benefit. Industry research and my personal experience have validated Atwood’s sentiment that Peer Code Review is the best way to achieve this cost/benefit balance.

Peer Code Review is the process wherein a developer’s peers individually review no more than 400 lines of code in no more than 1 hour expecting to leave about 15 comments per hour.

That is my current working definition of the term “Peer Code Review” which is what I mean hereafter when I say “code review” or just “review.” I’ll go into more detail in just a bit, but the main benefits of reviewing code in this fashion is that it helps to:

Ensure code is readable (unreadable code is unmaintainable code)
Catch bugs when it is cheapest to do so
Spread knowledge of the code base throughout the team
Expose everyone to different approaches

How to do it

General Process

Start by submitting a Pull Request. As much as possible, try to keep the number of changes to 400 lines or less. Ensure that the PR has good description of what has been changed and/or a list of specific things for the reviewer to look at. Github has a fantastic blog post on how to write the perfect Pull Request.

Next, let one or more of your peers know that the PR is ready for review. Within a few hours at least one of them should be able to find time to individually read through the changes. (Avoid the temptation to review code as a group. A study by SmartBear demonstrates that it’s a waste of time and yields practically zero results.) For a small change of about 100 lines, expect them to take about 15 minutes and leave 4–5 comments. Never spend more than 1 hour reviewing in one sitting. For larger PRs, break up the review across multiple sessions.

Comments represent anything from questions about functionality all the way up to actual logic bugs discovered in the code. Using a checklist of common defects helps to know what to look for. Questions don’t necessarily mean that changes need to be made, but they could indicate readability issues that the author needs to address.

When all questions have been answered and when any required changes have been made, the PR can be merged.

Expectations

The following expectations come from SmartBear’s of about 2500 code review sessions. These are by no means hard and fast rules, but should definitely heavily influence the review process.

LOC under review should be under 200, not to exceed 400. Anything larger overwhelms reviewers and defects are not uncovered.
Inspection rates less than 300 LOC/hour result in best defect detection. Rates under 500 are still good; expect to miss significant percentage of defects if faster than that.
Authors who prepare the review with annotations and explanations have far fewer defects than those that do not. We presume the cause to be that authors are forced to self-review the code.
Total review time should be less than 60 minutes, not to exceed 90. Defect detection rates plummet after that time.
Expect defect rates around 15 per hour. Can be higher only with less than 175 LOC under review.
Left to their own devices, reviewers’ inspection rate will vary widely, even with similar authors, reviewers, files, and size of the review.

How to actually do it

Ok, so you know why you need it and how to do it. But how to you ensure that you actually review code?

Allocate Time

Every developer from interns up through architects should be allocated 3–5 hours per week for code review. If you don’t intentionally set this time aside, you fall back into the pattern of “we know we should be reviewing code, but no one ever has time.”

This time is likely going to be distributed over multiple days but doesn’t necessarily need to be evenly distributed. Flexibility is key. Ideally, review should happen within a few hours of being requested. A rigid schedule (eg. 2–3pm, Monday, Wednesday, and Friday) is predictable, but also delays review. If the full allocation isn’t needed in a given week, that’s fine and developers can use that time to work on their other initiatives. But the time is available and those outside the team shouldn’t be surprised by how it is being spent.

It is the team lead’s responsibility to not just manage a project, but also the expectations of project stakeholders. Time allocations for code review should be clearly communicated and accounted for during planning sessions. As much as it is tempting to do so, this time should not be cannibalized to increase project velocity. Seriously, don’t do this! Writing larger volumes of bad code (as opposed to smaller volumes of good code) might offer a short-term boost, but is always done at the cost of decreasing velocity down the road.

Review Pull Requests

Review code at the point that changes get integrated back into mainline code. In general, I prefer to recommend processes in a tool-agnostic fashion, but Github’s Pull Request and Code Review features are too powerful to not mention by name.

For those who haven’t used them, Github’s Pull Request is a feature that allows developers to view and discuss changes on a branch before it gets merged back into mainline code. Most git-based platforms provide this type of functionality even though they may go by different names. For simplicity, I will just refer to them as “Pull Requests” or just “PRs.”

You don’t have to wait until code is “done” before creating a Pull Request. If you want review code in mid-development, create a PR and then add a “do not merge” label, tag, or note in the title. This is a fantastic way to get early feedback on larger architectural changes before you invest too much on the implementation. Just be clear in this case exactly what feedback you are looking for. If the code isn’t complete, your peers’ eyes will gravitate to the unfinished bits unless specifically directed to the parts you want their opinion on.

Peer Code Review should happen individually. Studies overwhelmingly show that the most efficient way to review code is by looking at it by yourself as opposed to a larger multi-reviewer context. The study by SmartBear shows that individuals will find up to 85% of defects in reviewed code, while reviewing as a group will find only 4%. I personally find that to be crazy! That being said, it is totally reasonable to request a review from multiple people.

Anyone should be invited to review anyone else’s code. Senior developers reviewing junior developers’ code is the obvious case, but junior developers will learn from senior developers by reviewing their code. Reading everyone’s code is one of the best ways to spread project knowledge across the team. You miss out on that if you only read more junior developers’ code for the purpose of finding bugs. Reading code written by more senior developers is a great way to learn new things. Reading code in general is a great way to ensure that code is readable. Even if you don’t have the authority to approve or integrate the changes, your feedback is useful.

Lastly, try to keep Pull Requests to a reasonable size. Studies show that the ability of a reviewer to find defects drops off significantly after about and hour. As a rule of thumb, expect to get through about 400 of lines of code per hour. That means that you can review in one sitting any PRs that is under 400 lines of changes. One way to keep PRs that small is to submit and merge code even before the feature is fully complete. If you do end up with a Pull Request can you can’t keep under 400 lines, review it over the course of multiple review sessions.

You don’t have to review everything

Code review does more than just find defects after the code is written. Studies show that when code reviews happen regularly, developers write better code. This happens for a couple of reasons.

First, the more code you review, you more you naturally review your own code before sending it to others. This is compounded secondly by the “Big Brother” effect. When you think that someone might review your code, you write better code, even if the review never even happens. This effect happens even when as few as one in four PRs are reviewed.

Use a checklist

I don’t want to post a checklist here. There are a ton of really good ones to be found by simply searching the internet. Additionally, your team’s checklist should evolve over time as projects and people come and go.

Start with a reasonable checklist targeted at your project’s language and architecture. As time goes on, periodically look through your code review comments and find the most common types of defects found. Add an check for those items to the list. If one already exists, move that item up to a more prominent position. Look through your checklist in the same way. If a checklist item never results in any discovered defects, remove it.

It’s perfectly reasonable for each team or project to maintain their own checklist. Don’t try save time on a nice DRY document that is so full of irrelevant items that isn’t useful enough for anyone to want to use. Also, don’t discourage individuals from using their own pet checklists. If you have something that helps you find defects, use it. The more tools, the merrier!

Using a checklist helps prevent “LGTM 👍” from being the only comment left on a Pull Request before merging. One way to use a checklist to review code is to start with one item on the list and then look for only those defects in the code. Then move to the next item and repeat the process.

Keep some things off limits

Don’t waste time looking for things that could be checked by a machine. Use a linter, StyleCop, or anything else that can automatically ding you for whitespace, nested ternaries, long lines, capitalization rules, 💩 used as a variable name … you get the picture. Add those checks to your CI process so that your team is notified instantly via a failed build when style violations enter the codebase. Let the humans find things that the machines can’t.

Also, don’t require changes that don’t cite some sort of industry standard or predefined team guidelines. Don’t use code review to argue tabs vs. spaces, use it to require tabs if your guidelines require tabs. If you don’t agree with the guidelines, definitely start the conversation to make changes, but do that outside the code review process.

Any other changes you request are going to be based on personal preference. These are perfectly valid to bring up, but be clear about them in your comments. If you think a switch would work better than a chained if/else, say something like “Consider using a switch here. I personally think that it would make this more readable.” If the original author doesn’t think a change should be made, let it go.

Final Thoughts

The ultimate purpose of investing time in code review is to save money by making better code and better coders. This article is the product of my personal research and experience, but if it doesn’t help you remove the barriers to reviewing code, then please skip the dumb parts and use what you can. If you want to read the content that has most influenced my philosophy of code review, please check out the following resources.