Thoughts on PRs and Code Review

By anders pearson 31 Jul 2023

Multiple times over my career, I’ve ended up writing a post similar to this on various internal blogs/wikis. I was about to write one yet again, so this time I thought I’d share it up here for a wider audience and maybe avoid having to rewrite this again in the future.

My first caveat is that this is all just my opinions. Take them for what they’re worth, which probably isn’t much. They are formed from a few decades of writing software on teams of various sizes and experience level in various domains and contexts and with outcomes across the whole range of success and failure. I will also include links to the thoughts of others who are probably smarter than me, have wider experience, and whose thoughts on these topics have helped shape my own.

My second caveat is that this is about code review and PRs in a team context. This isn’t about how those things are done on a public open source project. This is about reviewing your colleagues’ and coworkers’ code, not contributions from strangers, which requires an entirely different mindset. This is also an important point to keep in mind. Github’s PR concept and workflow was fundamentally designed around that open source contribution model and only later adapted to the more team-oriented context where it’s probably much more commonly used now.

Here’s my TL;DR: the closer you can get to true continuous delivery and trunk based development with non-blocking reviews and fast feedback cycles, the better. The value of code review lies more in communication and knowledge sharing than gatekeeping or quality control.

If you’ve read my recipe for reliability post, that’s probably not too surprising.

Now, I want to start with a few articles from others that I largely agree with:

If you only read my TL;DR and skimmed those, you might be thinking that I’m advocating against PRs and code review and for just merging everything to main and immediately deploying it to production. To some degree… I am…. Mostly, I just don’t believe that code review and PRs are particularly valuable or important for the purpose of maintaining quality compared to other factors.

If you compare Team A, with a high trust culture that empowers developers to own the production environment, do pair programming and TDD, and invest heavily in tooling for testing, integration, deployment, monitoring, and production observability but does zero code review and Team B, who does none of those things but has extremely thorough and exacting code reviews, I’ll bet on Team A to run circles around Team B, producing better quality, more reliable, more secure software faster and cheaper. They’ll beat them every day of the week on any reasonable engineering or business metric.

Of course that’s a false dichotomy. Zero code review might not be ideal even when you’re doing all the other things right. And doing thorough code review doesn’t necessarily imply that you can’t do the other things. Full trunk-based development and continuous deployment to production might not be fully compatible with your regulatory compliance requirements or some practical constraints. The point is more that at some point, a PR workflow or code review process that impedes on the feedback cycle starts to become a net negative on quality.

If what you want is high quality, safety, catching security issues early, etc. Code review will do some of that, but you will get much more value from improving the team culture, testing and deployment pipelines, static analysis tools, observability, etc. Basically, automation and tooling are going to get you a better return on your investment.

I also feel like I should make it clear that I actually really like reviewing code. I’ve long argued that reading code is a core skill that is largely ignored in our education and something that most developers should put effort into improving. You should get in the habit of reading as much of the code that you’re using as possible. Libraries, frameworks, and of course, your coworkers’ code. Building and maintaining this habit is, IMO, one of the main upsides of code review. Of course, that’s a benefit for the reviewer rather than the reviewee or the codebase.

Since PRs and code reviews are a necessary evil for many of us, the question changes from “should we use PRs and do code review?” to “how should we use PRs and do code review and get the most value out of it?”.

For practical advice, I’d start with this page: Code Review Developer Guide from Google’s Engineering Best Practices documentation. One nice thing about that resource is that it splits it up into a guide for reviewers and a guide for authors (the developer submitting the PR or (or CL in Google’s terminology)).

The most important part of that resource (IMHO), in the Standard of Code Review section is:

In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn’t perfect. … A key point here is that there is no such thing as “perfect” code—there is only better code. Reviewers should not require the author to polish every tiny piece of a CL before granting approval. Rather, the reviewer should balance out the need to make forward progress compared to the importance of the changes they are suggesting. Instead of seeking perfection, what a reviewer should seek is continuous improvement. A CL that, as a whole, improves the maintainability, readability, and understandability of the system shouldn’t be delayed for days or weeks because it isn’t “perfect.”

Most everything else in the guide is either justification for why that is the standard, explanations of edge cases where it may not apply, and advice for how to apply that standard to your own code reviews and development process. I’d even take that standard and generalize it a bit further as my own core principle, emphasizing the importance of frequent improvements and short feedback cycles as being preferable to longer feedback cycles and the pursuit of perfection.

That drives both how I create PRs and how I review them.

Things I try to do when creating a PR:

  • Make it as small and narrowly scoped as I possibly can. Ideally, only adding one feature, fixing one bug, etc. The whole thing should really be able to be summarized in one sentence. If I find myself writing “and” in the summary, I consider splitting it up. Google agrees and summarizes many of the benefits. A small narrowly focused PR is more likely to be correct, it’s easier to review, it’s easier to review properly (rather than a rubber stamp because the reviewer’s eyes have glazed over), and it can be either accepted or rejected atomically without adding extra work to split it up and rework.
  • I try to write a good description and summary that explains what it is for, any context around the work that a reviewer might need to be aware of, and highlight any areas that I am unsure of myself or that I think might look odd to a reviewer. Basically attempting to identify any criticism that they’re likely to have and preempt them.
  • I “self-review” the PR. Ie, I first go over the code and try to look at it as if I’m reviewing someone else’s code. Just seeing the code in a different context (eg, Github’s diff view rather than my usual editor) has been incredibly valuable. I’d honestly say that I’ve caught orders of magnitude more issues in my own code during this self-review step than reviewers have caught. This may be the strongest argument I can make for PRs and mandatory code review in terms of improving quality.
  • If there is any kind of automated test suite or linting available, I make use of it. Ideally, it should already be integrated into the PR automation.
  • If the reviewer sees that test cases have been added for the new functionality and that the test suite has run as part of the PR and passed, that preempts a lot of potential questions they might have about the correctness of the code. They can look for themselves at the tests rather have to go back and forth asking me about various edge cases.
  • If there’s anything potentially controversial about the PR, eg, architectural changes, I try to have a conversation achieve some consensus with the project/codebase’s stakeholders prior to the PR. That can be a formal decision doc or just a chat in Slack. I just don’t want to spring a surprise on them in the form of a PR. PRs and reviews should be about the implementation, not whether a feature or architectural decision is one that we want. Sometimes though, I have a hard time visualizing what the ramifications are of a change without actually just implementing it, so I might make a PR that does that. In those cases, I will make it clear in the description that it’s an experiment and often I’ll keep it as a draft so we can discuss the change elsewhere, just using the PR as example code to keep the discussion grounded.
  • I make heavy use of Github’s “draft” status for PRs so I can make a PR, run the automated tests, look at the diffs to do my own “self review”, clean things up (often using git amend or squashing some history) and write a proper description and summary. Only once I’ve got things how I want them do I remove the draft status and tag reviewers. That should minimize the amount of noise they get.

With all of those, I’m largely trying to prioritize the reviewers’ time over my own. Reviewing PRs is part of our jobs and the reviewers are getting paid for it, but when I request a review from a coworker, it still feels to me like I’m asking them to do a favor and generally, if I’m asking someone to do me a favor, I want to make it as easy as possible for them.

On the other side, when I’m the one reviewing code, my goals are:

  • Fast turnaround. As mentioned multiple times previously, the speed of the overall cycle is really important. The faster code gets reviewed, the faster it can be deployed and we can begin getting value from its improvements and learning whether it was an adequate solution or needs additional work. I try very hard to complete any reviews assigned to me within a working day. Unless I’m dealing with a flat out emergency, reviewing code is a higher priority for me than pretty much any other work. I know from my own experience that if there are delays of days or weeks on a PR that I’ve submitted, I will often have forgotten a lot of the context for the PR. Most of us jump around a lot between codebases, projects, and features. You have to reorient yourself when you come back to something that you haven’t been working on recently. A PR that’s delayed by days or weeks is far more likely to have conflicts with other changes that have happened in the meantime and require re-working. Other times, it can be that the issue that the PR was fixing is no longer relevant, having been obviated by some other change. Of course, fast turnaround is far easier to achieve when the PRs are small, narrowly focused, well described, etc.
  • For large, complex PRs that will take a significant amount of time for me to review or do additional research before I can review it, or if I’m particularly busy with something else and won’t be able to get to it within a day, I try to make a note to let the submitter know that (and maybe encourage them to think about splitting it into smaller PRs).
  • I do not try to identify every possible issue with the PR and block it until it’s perfect. I’m generally asking myself whether the codebase as a whole will be in a better place with the PR merged than without. The goal of a code review isn’t to obstruct work, but to help it move forward. I do place extra scrutiny and higher standards on certain areas like security and data integrity though. An edge-case bug in a feature is something that we can fix in future PRs incrementally. Opening up a security hole or corrupting data is far more expensive to deal with later and worth slowing down development velocity to handle properly the first time. I’m also going to look more closely at changes that are in customer facing critical parts of the code than, eg, internal tools. When I’m reviewing a PR, I’ll generally only block it if I think that it will actually break production or introduce some problem that has a high cost of reversing (eg, lost customer data, potential security breach, etc). If it’s just a matter of “this code doesn’t meet some arbitrary high standard for quality”, I’m much more likely to approve the PR and trust my teammates to address that in a follow up. This is as close to the “non-blocking” approach to code review as I can usually get.
  • I will still look for “low hanging fruit” though. If there are small changes that could be made, I’ll usually approve the PR but still suggest those changes. Github has a really nice bit of functionality for inline code suggestions that I use wherever possible; that makes it so the submitter only has to click a button to accept the changes. For other suggestions, I’ll usually leave it up to them whether they want to incorporate them into the current PR or merge it and address them in follow up PRs.

Again, no strict rules or formal checklists that I go through. My goal is to help improve the code and move things forward.

I have a fairly simple daily routine that I stick to:

  • One of the first things I do every morning to start my workday is pull up https://github.com/pulls/review-requested. That URL shows a list of any pending review requests. If you are reading this, look at your list. How many do you have? How old are they? What’s stopping you from reviewing them?
  • I usually open each of those in a new tab so I can go through them one at a time. Quick ones I deal with right away. Ones that look like they’ll take a bit longer, I save and then figure out when I’ll have time during my day to look at them properly or to let the submitter know that it might take me even longer.
  • Once I’m done with that first pass processing review requests, I click the bell icon in the top right, which should pull this up: https://github.com /notifications?query=is%3Aunread. If you’re not in the habit of keeping your Github notifications at “inbox zero”, you might have thousands there. In that case, I recommend just accepting that old notifications are useless and clearing them all out. I clear mine out on a regular basis so the notifications are actually useful to me. I use it to keep track of whether there are new comments on reviews, whether my PRs have been approved, etc. I visit the notification page many times per day; basically any time I’m in between other tasks. It usually just takes a few seconds to acknowledge and clear them out.
  • Usually, but not always, before I leave for the day, I pull up the review requests list again. This time though, I’m only looking for ones that I can review very quickly. I’ve worked on remote teams distributed across multiple time-zones for almost the last decade so taking a couple minutes before I leave to unblock someone who might be just starting their workday can reduce their total latency by 8-12 hours and seems worth a little extra effort.

So that’s my approach. I hope it’s been helpful. Happy code reviewing!