Code review is great way to maintain and improve your team’s codebase, and like a lot of team process having an intentional, positive culture around it makes it better. One of my earliest patches at Twitter was adding a short-lived token-based system for logged out requests (think login, signup, etc.). The patch itself wasn’t so large — a few hundred lines of Objective-C plus tests and integration, but it was important enough to require a fair bit of scrutiny in code review. This change was risky, and it wasn’t something we could easily test incrementally in production so we needed to get it right.

Like all our changes, my patch went through code review and it ultimately took many revisions for me to get it right. Code review can feel intimidating, but in this case I felt supported as my reviewer displayed empathy and patience while helping me find mistakes. That review helped set the tone for how I’d work with others in the future so I thought I’d share a bit of what I’ve learned about approaching code review on large teams.

My job as a reviewer

My job as a reviewer is to help make my coworker’s contributions better by helping them catch mistakes as well as helping them improve their technical solution. My job is not to gate keep to get my way. One natural consequence of this is it enables authors to express creativity in their work, which leads to more engaged teammates. Of course, it’s really important to teach others when there’s a better way to solve problems and doing so gracefully is part of providing effective code review. That’s a core part of code review, but it’s not what this post is about.

Sometimes there are established team norms one needs to follow for the sake of consistency in a codebase — e.g. stylistic things. We’re not here to talk about tabs vs. spaces, sorting imports, or whether braces belong on a new line either though (real talk: spaces, yes, and absolutely not 😇). I’ve found it’s helpful to move these sorts of things to automated linters, where possible, as it removes the need to raise them in code review altogether. This maintains consistency, simplifies and speeds up code review, and removes the potential for animosity over minor stylistic change requests.

Speaking of norms, your team might consider setting one for the upper bound on the size of a typical patch. Exceptions will be made and that’s okay, but keeping patches small means reviewers don’t need to spend as much time on each code review pass. This is not only considerate of teammates’ time — it also helps make space for their other work. Sticking to reasonably sized patches also makes it easier for multiple people to work on the same project as it reduces the times which one person might be blocked waiting on another. I’ve seen larger patches work out fine on other teams — it depends on your team’s needs.

Framing feedback

Framing code review feedback in a way that’s clear and non-confrontational helps avoid the misunderstandings that sometimes result from written communication. Written communication can be devoid of the tone that smooths over mistakes in verbal communication. I ask myself how I might receive a message to help refine the wording so it’s more likely to be understood as I intend. Importantly, the wording I choose critiques the work I’m reviewing and not the person.

Working relationships can also affect how you communicate in code review. Getting to know your teammates helps you understand their communication style, which helps you give each other the benefit of a doubt when reading, for instance, a terse reply. However, you won’t always have the luxury of knowing how others work and communicate. Whether you’re working with a new teammate or your team is so large that it’s impractical to know everyone, it’s important to consider how framing your feedback might affect how it’s received.

Another way to approach communication, in code review and beyond, is leading with sincere curiosity. Curiosity helps disarm negative emotion when another person receives your commentary. For instance, something like “did you know we have a utility function for this?” along with a function name is better than sharing only the function name or a link with no additional context because it doesn’t unintentionally imply the author has done something wrong, is lacking expertise, etc.

Context matters

When reviewing code I try to keep in mind the author may have context I don’t and vice versa. They may have spent more time on the patch than the number of lines changed indicates, and they might have constraints I’m not aware of like deadlines or other urgent work to get to. I ask questions to gather the context I need to provide helpful code review up front. Do I clearly understand what the patch is supposed to do? Should it have automated tests? Does it have manual testing instructions so I can verify it works? A proper commit message? Documentation where applicable?

I then leave comments where I think something can be improved. I like to prepend “optional: ” or “nit:” where I have minor suggestions that shouldn’t block merging the patch so the author knows they are non-blocking. The author makes the call on whether to address those comments. The patch is their contribution, after all.

If a patch requires substantial rework — maybe I feel I’m leaving too many comments, or sense we’re talking past each other in code review, then I take the discussion to a richer communication medium. For instance, if we’re having a discussion on a code review tool, then I’ll send a private message on Slack to ask if we can chat there. If we’re talking on Slack then I’ll ask if we can have an audio or video call. There’s a bit more to this though: Slack is richer than a typical code review tool because it moves the conversation from a public space to a private one, and it enables live communication. The former can help dial down emotions, but it doesn’t necessarily help with tone as that requires intentional word choice, especially in written communication. It’s worth keeping in mind the culture one grew up with may also affect how they interpret your words.

Earlier I mentioned tone helps bridge understanding. Moving to a richer communication medium is one way to add tone to the discussion. I’d be remiss if I didn’t mention it’s important to note any potential accessibility considerations and working hours for colleagues when switching communication mediums.

Avoiding wording that make assumptions about the other person’s knowledge is part of understanding teammates may have different context. I try to avoid words like “obviously” or “just” because what’s obvious or simple to me may not be so obvious or simple for someone else. Perhaps they’re new to some part of the codebase, the team, or are earlier in their career.

Wrapping up

I’ve certainly done all these things wrong in the past, and I’ve been on the receiving end of code reviews from colleagues who have done the same. It wasn’t always clear to me when I made a mistake as a reviewer until later, but it was usually clear to me when I felt let down by the way someone delivered feedback on my work. Being cognizant of your job as a reviewer, framing, and context you have (and don’t) can help you avoid unnecessary strife with your teammates.

There are also things you can do as an author to make code review easier on yourself and your colleagues, as well as ways you can defend your time and approach where needed. We’ll talk about those things in a future post.

Follow me on Twitter at @amdev if you like puns or pizza.