4  Speed of reviews

4.1 Why should code reviews be fast?

The speed of an individual developer is important, but the speed at which a team of developers can produce high quality code is even more important. When code reviews are slow, several things happen:

  • Authors start to protest the code review process. If a reviewer only responds every few days, but requests major changes to the PR each time, that can be frustrating and difficult for the author. If the reviewer requests the same substantial changes (changes which really do improve code health), but responds quickly every time the author makes an update, the frustration tends to disappear.

  • The velocity of the team as a whole is decreased. Yes, the individual who doesn’t respond quickly to the review gets other work done. However, new features and bug fixes for the rest of the team are delayed by days, weeks, or months as each PR waits for review.

  • Code health can be impacted. When reviews are slow, authors are actually incentivized to submit fewer PRs and to make each one larger. If you think that it is going to take days to get a single small change reviewed, you are going to be pressured to include one more thing in your PR just to avoid another multi-day review process. To ensure that each PR only improves the health of the package, code review must be fast enough that authors don’t feel hamstrung by it.

4.2 What do we mean by “fast”?

Rather than suggesting average turnaround times for a PR review (such as 2-4 hours), we feel that it is more useful to categorize PRs under different patterns of collaboration. This recognizes the fact that each PR comes with its own context - a time sensitive bug fix that blocks another colleague is going to be reviewed and merged faster than a small typo in a dormant package that isn’t actively being worked on.

The most important thing is for the author and reviewer to have similar expectations around the amount of time a PR review may take. If there is confusion around this, the author and reviewer should discuss this with each other, especially if they are both Posit employees, to ensure that they are on the same page about which pattern their style of collaboration falls under (keeping in mind that the patterns can be modified as needed).

If you are feeling overwhelmed by the amount of PRs that you are in charge of, it is worth reviewing your PR process. If you have so many PRs to review that you can’t get to your own work, then you might need to distribute the reviews across your team more evenly. Not speaking up when you are overwhelmed by PRs can in turn slow down an entire team!

4.3 Speed vs interruption

There is one time where the consideration of personal velocity trumps team velocity. If you are in the middle of a focused task, such as writing code, don’t interrupt yourself to do a code review. Research has shown that it can take a long time for a developer to get back into a smooth flow of development after being interrupted. So interrupting yourself while coding is actually more expensive to the team than making a PR author wait a bit for a code review. If you want to read more about this, Paul Graham’s post on Maker vs Manager is very good.

Instead, wait for a break point in your work before you respond to a request for review. This could be when your current coding task is completed, after lunch, returning from a meeting, coming back from the breakroom, etc. Most “deep work” sessions last around 2-3 hours, so batching your code reviews and handling multiple of them at once after a focused session often works well.

4.4 Detailed review

With all of this discussion about speed, it can be tempting to just do a surface level review and respond with “LGTM.” Resist this temptation! The whole purpose of code review is to ensure that the code health of the package is improving, and you are unlikely to catch potential bugs with a surface level review.

PRs vary greatly in their complexity and sufficiently carrying out the review can take a non-trivial amount of time and cognitive effort. In general:

  • Small PRs take anywhere from 5-15 minutes and don’t typically require checking the code out locally. This should be the majority of code reviews that tackle small bugs or documentation changes. Experienced reviewers should be able to “run” the code related to these changes in their heads.

  • Medium PRs take up to 30 minutes and often involve using usethis::pr_fetch() and exploring the code locally. This typically includes PRs that implement new features or small UI changes.

  • Large PRs can take up to 1 hour, and occasionally even longer. These should be very rare, and are reserved for large refactorings, especially if there are unavoidable behavior changes mixed in. Large PRs are typically undesirable because they slow the velocity of the entire team, so typically the reviewer should ask the author if the PR can be split into multiple smaller focused PRs. If you don’t have time to review a large PR and it can’t be made any smaller, provide some broad comments on the design of the PR, and consider requesting another colleague to take a second look.

4.5 Cross time zone reviews

Dealing with time zone differences can be challenging, but when the author and reviewer are working together closely it is reasonable for the reviewer to ensure that they provide feedback before the author starts work the next day. If more rapid turnaround times are needed, then the author should communicate that with the reviewer and attempt to batch multiple PRs in parallel, or set aside a predetermined time during the day when both the author and reviewer are online for review.