Why code reviews matter

At Airtasker we are determined to have a powerful engineering culture. A strong culture begins with a shared understanding of and belief in the way good software gets built. To that end, we are busy putting in place norms, structures, and tools around all manner of things: architectural design, experiment lifecycle, tracking, testing, bug management, branching policy, and more.

Taken in sum, all of these declare “this is how we do engineering at Airtasker”. Of course, saying things is easy; the world is littered with neglected values statements, best practices guides, and other well intentioned cultural artefacts. No, for these norms to become bedrock for our culture they must be enforced. And the only scalable, culturally acceptable form of enforcement is peer enforcement. If people have bought in to the engineering culture, they will not accept work that is not up to scratch.

That is why I’m obsessed with pull requests. The code reviews associated with PRs are the perfect tool for engineers to uphold standards amongst themselves. Once it is understood that reviewing a PR is not a rubber-stamping exercise but rather an almost-sacred duty to uphold the best of engineering within the team, there is an avenue for the culture to propagate itself.

If you don’t have a strong culture of code review within a team, few of your other cultural efforts will take root. For example, writing tests is a drag (I know some people love writing tests but that majority — including myself — do not). Under time pressure, and if you think you can get away with it, the temptation to go a bit light on testing can become too strong to resist. Good code review culture means that you can’t get away with it. And if you know you can’t get away with it, you won’t even try. Eventually writing tests becomes a habit and you can’t imagine how you ever thought it was OK not to.

When I started at Airtasker, one of the first things we did was agree on and document the expectations, roles and responsibilities around pull requests. This document is in our wiki and helps create a rigorous, efficient, and harmonious process of review. We’re getting pretty good at it. And the better we get at it, the easier it becomes to get better at other things.

I’ve reproduced the internal pull request guidelines we use at Airtasker below. Feel free to make use of them if they seem useful.

How to read this document

Think of this as a set of expectations that we as engineers have of each other. Some of the points are more like rules, others are more like values. In any case, the most important thing is that these are expectations that we have of each other as colleagues. It only works if we hold ourselves and each other accountable to these expectations.

This is how we do things here.

Committing to master is a privilege, not a right.

As a code author you need to convince your reviewer that the code under review has earned the privilege of being committed to master. If a reviewer is not comfortable with the state of the PR, they are not “blocking” you, they are doing their duty.

Acting as a code reviewer is a duty. Doing your duty is not always convenient or pleasant, but it is important. As a code reviewer, it is your duty to safeguard the quality of the codebase and to perform reviews responsively and promptly.

What makes a good PR

A good PR is easy to understand. That means it is as small and focused as possible, and provides as much context as possible. That allows the reviewer to understand what the change is intended to do, and to evaluate and respond to the PR quickly.

A good PR has these attributes:

• Atomic; it just does one thing, and is as small as possible. If you can’t describe it in a sentence or two, consider whether it could be broken out to multiple PRs. It’s okay (often great) to deliver a ticket in multiple PRs. There is no expectation that a single ticket should result in only a single PR.
• Well documented; always ensure your PR description is meaningful for the reviewer (see atomicity). Link to relevant documentation.
• Make your squash commit message as meaningful as your PR description.
• As a suggestion, copy/paste the PR summary into the commit text.

Expectations of you as a PR author

• Write good PRs (see above)
• Seek feedback early on your approach before you’re committed (with design docs and WIP PRs). If your reviewer requests a change that requires major rework, that’s probably because you waited too long to seek feedback.
• Practice egoless programming: understand that a reviewer is doing their duty as custodian of the codebase, and that comments and feedback address the code under review and not you as a person.
• Don’t put unreasonable pressure on a reviewer based on your deadlines. Part of meeting a deadline is leaving adequate time for the code review process.

Expectations of you as a reviewer

• Treat small PRs as soft interrupts; i.e. review the code at your next context switch. Ideally this should be within half a day.
• Large PRs should be dealt with as quickly as possible. Communicate directly with the PR author to establish a timeframe that is reasonable for both parties.
• What is a large PR? At the time of writing, we will treat 150 LOC delta as the threshold.
• If a PR feels unreasonably large or not atomic enough, you should feel free to push back and ask the author to split it up.
• Performing a review is a duty; you do it because you’re part of a team, not as a favour to the author.
• As a general rule, performing reviews is more important than meeting your sprint goals. Sprint planning should account for the fact that PRs are a part of an engineer’s job (this should happen automatically via velocity estimation).
• You should never feel under pressure to approve a PR because of perceived deadline pressure; in a vast majority of cases, the health and quality of the codebase are more important.
• PR feedback should address the code, not the person. Being firm and assertive with feedback is important, but you should always be respectful to the author.
• If you can’t review within the expected timeframe then it’s OK to find an alternative reviewer. But it’s your responsibility as a reviewer to do so promptly, and to communicate with the author around this.

How to choose a reviewer

• AND semantics — all nominated reviewers must approve before a PR is committed. If you’re requested, you are blocking; act accordingly.
• Don’t add many reviewers to your PR as a “catch all” to get a faster review. AND semantics means you’ll just slow yourself down.

Choose the best reviewer for the job based on:

• Experience
• Workload
• Familiarity with the technology and code that is being touched

Avoid the following ways of choosing a reviewer:

• Always choosing the same person
• Always choosing someone who is on the same team as you
• Choosing someone who you know won’t challenge you
• Choosing someone because they owe you a favour
• Avoiding a particular reviewer because they tend to give your code more scrutiny