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.
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.
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.
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.
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.
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.
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
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.
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.
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:
git rebase -i and change from
which will then give reviewers 5 clean commits to review, rather than 5 potentially messy commits followed by cleanup commits.
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:
For example, one client has the following requirements for their Python code:
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):
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.
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