X

Contact Us

News

How to Reduce Code Review Cycle Time

DanielGallagher
Partner

Code reviews are almost ubiquitous on our projects. We have worked with clients whose review processes provide significant value, and some whose processes are more hindrance than help. Our team meets weekly to learn new things and share best practices, we call them Software Discos. Here are our tips to reduce code review cycle from a recent disco.   

The Why

Before we get into reducing the time it takes to do code reviews, let’s quickly review why we do them in the first place. There are a lot of obvious benefits for doing code reviews. They can find and fix bugs before they are deployed and they are a good way to share knowledge of the codebase and teach methodologies. In some cases, they are also required by clients or government bodies like the FDA. 

However, there can be downsides to code reviews too. They can slow down the process. It can also be frustrating when feedback is more preference-based than requirement-based or when people use a code review to make a developer look bad. Some developers may feel that performing code reviews takes time away from “their development.” Though they are meant to catch bugs, a bad review could actually introduce bugs via bad advice. However, all of these can be avoided with a good process. 

We’ll take it as given here that you want or are required to do code reviews. We strongly encourage you to adopt a review process if you haven’t already! We want to ensure an effective code review process, not scare you away from having one at all.

Set Clear Expectations

Who is required to review code? 

We’ve found that the ideal is between 2-4 people to reduce code review cycle time. One person can cause a bottleneck while more than four people usually result in one of two extremes: either everybody leaves lots of comments or nobody leaves comments, assuming someone else will, the diffusion of responsibility in action.

  • If you want the whole team to become familiar with the code, clearly separate roles for reviewers and people who have access to read the code without commenting or approving. 
  • If you want to train junior reviewers, do a pair review with the author or an experienced reviewer.

Some organizations have code “owners” or “custodians” who are responsible for reviewing every change in a particular section of code. We prefer to work without strict code ownership, where everyone on the team is empowered to make fixes throughout the code base. But, in large codebases, owners can be a last line of defense against bugs creeping into the code. We strongly recommend that you use owner groups if at all possible. That lets authors know who to add to the review, but prevents any single person being a bottleneck. If you do have owners, you need to ensure their domain of responsibility is small enough that they can review everything without holding up other developers.  

When should code reviews be completed? 

One of the best ways to increase the effectiveness and efficiency of code reviews is to minimize the time between writing the code and merging it to the primary branch. If you wait too long, you will forget why you did what you did. As a result, you may accept advice that goes against your original plan. 

Ideally, review code the same day to reduce code review cycle time. If that’s not possible, make sure everyone on the project knows how long reviews will take so authors can plan for the delay by having multiple work streams going at once. Just in case the review takes a while, get into the habit of adding a comment to anything that is not standard practice. This will communicate your thinking to the reviewer and help you remember as well.

What code should be reviewed?

Small patch size is extremely correlated to reduced code review cycle time. As a result, reviews can be done by a small team and within a short timeframe – achieving the who and when tips above. 

Influence of the Patch Size (LOC) over Duration (days), Participation (%), Comment Density (comments per 100 LOC) and Comment Density by Reviewer (comments per 100 LOC per active reviewer). The y-axes representing Comment Density and Comment Density by Reviewer are in a logarithmic scale.

Get Fluent in Git 

Getting comfortable switching between branches locally and saving work for later is helpful both as an author (so you can switch between your current work in progress and a branch that you need to update for reviews) and as a reviewer (if you want to check out code locally for testing). Check out the book Pro Git for more information.

Some common tasks to juggle code are:

  • git stash especially if I’m checking out someone else’s code for review and will get back to my work shortly
  • git stash pop to undo a stash
  • git commit -m "wip" especially if I’m saving my work to switch back to an old branch for PR updates
  • git reset HEAD~ to undo the previous command without losing work
  • git rebase -i to help keep commit history clean, so I don’t have “Addressing PR comments” commits
    • Some code review tools (such as Gerrit) essentially require you to get good at editing multiple commits on a branch.
    • Other review tools may get confused if you rewrite history during a review. In that case, it may be better to have “Addressing PR comments” commits during a review. We still would recommend doing a final pass to clean up the branch prior to merging to the primary branch. If your team uses squash merges, this may not be an issue.
  • git add -p to make multiple commits out of my working changes, and git stash -k to make sure those intermediate commits build and/or work.

Make Testing Easy

Your PR should include automated tests that run alongside the new production code. Tests let reviewers examine the code from an outsider’s perspective in addition to the author’s perspective. It makes it easy to see the intended API, and whether or not it will be ergonomic for callers.

For firmware projects (or software projects that involve a hardware component), making testing easy can be more challenging, especially if hardware availability is limited. This is a priority if code reviewers are tasked with running the code to verify it. It becomes less important as the code review process is easier and faster, since bugs can be fixed more easily than with an arduous code review process.

Make Code “Perfect” 

One way to to reduce code review cycle time is to ensure reviewers have nothing to say. Of course that’s not a practical goal but, in general, lessons can be learned from reviewers past comments and, over time, those comments can be addressed before the code is put up for review.

This section includes some tips from Curtis Einsmann via his Medium article:

  • Read the existing code. Make sure that your code fits the patterns of the existing code. This goes beyond the style guidelines (which should be automated as much as possible), and is more about making sure that you are doing things the way the existing code does them. Look for code that you can reuse, and either reuse it or refactor the code so that it is reusable, then reuse it.
  • Plan your implementation. Especially if there is some refactoring involved, it can be helpful to write down the steps you plan to take, and where you expect your code to end up (classes, functions, etc). Plans change, but having a plan can make the implementation more review-friendly if you can make your commits tell the same story as your plan.
  • Refactor for readability. First, write the code any way you want, then refactor to ensure it is all readable. Micro-commits are helpful here. I’ll often have a series of commits where I’ve made (for example) 5 somewhat related commits and a number of cleanup commits. Before making the PR, I’ll do a git rebase -i and change from

 

pick a6c8bd0c Look up the I2C busses from the config file
pick 7a6680cf Store score in hardware state
pick de9024f6 Remove now-unnecessary read/write delay
pick 6ad50740 Force set points to be integers
pick 12cacb45 Unconditionally write current limits to PMM
pick c9c946a1 Fixup with PMM limits
pick 2f6128dc Fixup with set points
pick 1c1be89b Fixup with PMM limits
pick 81b28ce8 Fixup with score

to 

pick a6c8bd0c Look up the I2C busses from the config file
pick 7a6680cf Store score in hardware state
fixup 81b28ce8 Fixup with score
pick de9024f6 Remove now-unnecessary read/write delay
pick 6ad50740 Force set points to be integers
fixup 2f6128dc Fixup with set points
pick 12cacb45 Unconditionally write current limits to PMM
fixup c9c946a1 Fixup with PMM limits
fixup 1c1be89b Fixup with PMM limits

which will then give reviewers 5 clean commits to review, rather than 5 potentially messy commits followed by cleanup commits.

Accept Imperfection

As a reviewer, the flip side of this observation is to accept that code is not perfect or written exactly the way you would write it. Decide if a comment is going to be worth the delay (and potential frustration) it will cause. If you leave a “nit” comment, it will probably still get addressed, but will likely cause another cycle of implementation and review. If you only have nits, it’s probably just not worth it. Alternatively, make your nits optional and approve the PR (if the tool allows). 

Instead of leaving nits, you can always clean them up in your own PR later. This is also one of those things that is easier the simpler your code reviews are. If your reviews are difficult and time-consuming, you’ll be inclined to include nits in your review comments, since it isn’t worth it to address them with a new PR. But if code reviews are a breeze, then there’s no reason to hold up a PR for nits; just submit them yourself later. Be careful with this tactic, since it can come across as passive-aggressive. Make sure you have good communication with all parties and make sure they are OK with it ahead of time.

Automate the Boring Stuff

Use tools to automate as much as you can and reduce code review cycle time. These include:

  • Pre-commit and pre-push hooks to check for style and to ensure tests pass (if required by the project)
  • Using auto-formatters
  • Using static analysis
  • Spell checking in comments

For example, one client has the following requirements for their Python code:

  • Format using yapf
  • Sort imports into three blocks: Standard Library, third-party, first-party
  • Use double-quote characters (Python lets you use either single or double-quote, or three of either) to delimit strings
  • Include type hints for all arguments and return types
  • Document all arguments and return types

Even though only one tool is defined by the team, I’ve found others that automate all of this, except for ensuring all arguments are documented (using the correct format):

  • yapf for formatting
  • isort for sorting (including a custom .isort.cfg to identify first-party imports)
  • unify for using the correct quote character
  • mypy for finding missing type hints (and also doing type checking, which is not required, but extremely helpful)
  • pylint for other static analysis checks, which they have a tool for, but do not require.

Explore Alternatives to Code Reviews

So you’ve tried all of those tips and still can’t deal with code reviews? Try these alternatives to achieve the same benefits.

After-the-fact Reviews

This is the “innocent until proven guilty” doctrine applied to code reviews. There’s no need to review code until it has started causing a problem or if there are code smells. In that case, use git blame to find the commits that last touched that code, then git show to review that commit (or git log -p to also review commits preceding it).

Pair Programming / Mob Programming

Pair programming is where no production code is written by a single person, but rather by a “pair” (which could change day-to-day, or task-to-task). The idea is that one person has the role of driver, who actually types, compiles, runs, etc., and the other is the navigator, who is thinking about higher-level issues, such as how the code fits into the code base, did they cover all the edge cases, etc. The people swap roles frequently. Mob programming is pair programming upsized so that there is one driver and the rest of the team navigates together.

Better Automated Test Suite

If the test suite catches “all” your bugs, you may not need a review – assuming the tests pass.

The Bottom Line

The bottom line? The less friction you have in your code review process, the easier they are to do and the more you can reduce code review cycle. So, do everything you can to lower the friction – make it easier to submit small patches, check your tendency to nit, and get to reviews sooner. Minimizing all of these leads to a virtuous cycle where reviews are easy and painless.

As always, let us know if we can help. You can reach us by clicking “contact us” below or at [email protected].

 

Date published: 02/13/2022

Categories: Insights, Uncategorized