If you’ve ever asked a friend to check your teeth for spinach before a big meeting, you can understand the importance of code review. If you’ve never asked a friend to do that because you hate spinach and would never eat it, you can still probably understand the importance of code review.
Code review might not technically be a formal testing or QA step, but it’s still key. And it can't hurt to add one extra step to help ensure you're not going to end up having to roll something back 30 minutes after you push it out.
Of course, you may not need to be convinced to review code. But you might need to be convinced to have a checklist when you do so. I mean, that is the title of this post.
To that, I say: If you were going to that big meeting, would you only prepare by asking your friend to check your teeth for spinach (or to check your teeth for not-spinach if you hate spinach)? Even in this new world of remote work, you’d at least make sure to put on a clean shirt. Or, at least, a shirt that appears to be clean. Or a shirt with a stain that is only barely visible on a laptop webcam.
So, should you use a code review checklist? The answer is both yes and no. And if you think we can’t have it both ways, read on and we’ll prove you wrong (or right).
In surveying a bunch of developers (this is an official unit of measurement, like a bushel of oranges), we learned that many companies don’t have a formal code review process. This is usually because individual teams want the flexibility to do code review in a way that works for them.
Lyft gets around this by adopting different approaches for their different teams. Their core product teams operate with a rigid, formal code review process, while other more peripheral product teams have more freedom in how they approach code review.
While this does work for some companies, it more often than not leads to inadequate code review due to a lack of standardization.
Your code review process doesn’t have to be formalized, but it should have some level of standardization. What’s the difference? A formal process has a rigid structure that must be followed, along with several rules and procedures that may or may not make sense for every situation.
A standardized process just means your whole team is on the same page as far as what constitutes a good review, when they should ask for a code review, who they should ask, and how code review should work. Even the world’s most [bleep]-it, ship-it oriented team shouldn’t ignore the benefits of this kind of code review checklist.
A good code review checklist . . .
Customize your code review as much as you need to in order to express your creativity (Lyft uses emojis as commands). But please, for the love of users and QA testers everywhere, create some kind of code review checklist that your company, department, or project team can agree on. Let’s walk through the must haves, and then you can ignore them and do whatever you want.
You don’t need a rigid hierarchy to decide who should review code, but you also don’t want to just pass it off to whoever is available. As a team, you need to decide who is allowed to approve and merge code. You also need to set some guidelines for who gets tapped to review code. These guidelines allow your whole team to feel confident asking for—and performing—code reviews. It also ensures no one feels singled out if they rarely get asked to review. They’ll know the guidelines in advance and understand if they meet them.
The first answer to the question of who should do code review is simple: everyone should do code review. It’s essential to create an environment where code review is seen as a team effort and a meaningful activity. PullRequest has some great advice on how to establish team values around communication, documentation, and prioritization to create an effective code review culture.
But just because everyone should be involved in code review doesn’t mean you should ask just anyone to review your code. In fact, it’s actually important to limit the number of reviewers that you tag in any given pull request. When you tag too many reviewers, it’s possible to trigger a bystander effect, where no one steps up to help because they all assume someone else has it covered.
Instead, choose one or two qualified reviewers you can trust to give you a helpful review. James Cunningham, Infrastructure Engineering Manager at Sentry, gives a very simple guideline to help you decide whether or not someone is qualified:
“They need to have the personal confidence to say ‘I approve these changes as if they were my own.’” This means they’re familiar with the context of the code, the purpose of the code, and the nature of the related code branches. James also recommends leveraging a CODEOWNERS file in Github to define ownership roles for various code branches and quickly tag relevant reviewers.
When deciding who’s qualified, however, it’s important not to overlook one of your greatest resources: your junior developers. It can be tempting to leave code review to your senior developers for the sake of speed, but not only do junior developers need the experience and camaraderie that code review provides. they also bring a fresher perspective to the code base. They might have some ideas to deal with some of the
Start your junior devs on smaller pull requests, but don’t leave them out of the process. The more you can involve them, the faster they’ll be able to drop the word “junior” from their title.
“Before you merge” isn’t a good enough answer to this question. You need to decide, as a team, when you’re going to request code reviews, and when you’re going to complete code reviews. Without a clear “when,” it’s easy to accidentally push code review to the back burner or to end up scrambling and begging for reviews every time you’re ready to push something out.
A good rule is to apply the “Goldilocks principle” when it comes to the size of your pull request. If you pull hundreds of lines of code for a review, chances are they won’t be reviewed thoroughly. If you pull too little code, it can be difficult for reviewers to understand the context. Aim for “just right” by thinking in terms of what the code actually does, how it relates to other parts of the code, and what other branches your specific branch depends on to function.
Have regular time set aside for code review. Most teams don’t have the luxury of their own dedicated code reviewers. Everyone has their own code to ship, and it’s easy to prioritize shipping over reviewing. This often results in a backlog of code review requests sitting in various inboxes, while delays slowly creep up on the project.
Set the expectation with your team that code review is important—this isn’t an “add-on” or a “when you have the time.” Code review is a core part of shipping excellent code, and it’s part of everyone’s job description, no matter how junior or senior. Build time for code review into your regular work cadence, either as part of your sprints or as a regular part of your week.
We’ve firmly established that there is no one right way to do code review—although you’ll find plenty of tools, checklist templates, and advice out there if you really want to seek a One Right Way. But there are three critical elements that must be part of your code review checklist, no matter how informal:
#1: Give constructive feedback. Yes, you have a lot of other work to do. But code review is worse than pointless if the feedback given is simply “FIX” or “That’s a bad way to do that.” Make it actionable, brief, and context-rich.
If you're worried you'll forget this, just remember these as your ABCs of code review.
#2: Adhere to unified style guidelines. A programming style guide makes every part of your life easier, from onboarding new developers to shipping code on time. One of the main purposes of code review is to keep your code base maintained and readable. The problem is, people sometimes have a lot of different definitions when it comes to what makes a code base “readable” or “maintained.” Base your code review around a style guide to keep this core purpose intact. It’ll give you something to debate and argue over, but will move the debating and arguing out of the review itself.
If you're worried you'll forget this, just remember these as your AUSGs of code review. Or just, like, write it down.
#3: Establish contingency plans. It’s important to plan ahead for the inevitable situation of being slammed against a deadline. You don’t want to throw your code review checklist in the garbage when a deadline hits, but you also want to stay flexible. Build contingency plans into your checklist—plans that work for your team but still place proper emphasis on code review. Consider the way that James’s team at Sentry handles this: approve overall changes, acknowledge individual items that require later revision, and put those items into a team’s backlog.
If you’re doing code review but don’t have a checklist of some kind—however informal—you’re essentially playing darts with a blindfold on. It’s basically just pin-the-tail-on-the-donkey, shoot-the-basketball-over-your-head-from-the-other-side-of-the-court, roll-a-skill-check-when-the-dc-is-really-high-and-you're-not-that-good-at-the-skill, and other mixed sporting / gaming metaphors. It’s pretty risky, even when it works out just fine.
To reap the benefits of code review, you have to have some standardized practices and principles to unite teams and projects. Call it a code review checklist, framework, anti-checklist, or principles—just make sure you’re answering who?,” “when?,” and “how?,” and you’ll be good to go.
Nothing to see here...