Lately, I’ve been reading articles about best practices for code reviews. I notice that these articles focus on finding bugs to the exclusion of almost every other component of a review. Communicating issues you discover in a constructive and professional way? Irrelevant! Just identify all the bugs, and the rest will take care of itself.
So I had a revelation: if this works for code, why not romance? With that, I’m announcing my new ebook to help developers with their love lives:
My revolutionary ebook teaches you proven techniques for maximizing the number of deficiencies you find in your partner. The ebook does not cover:
Communicating issues to your partner with empathy and understanding.
Helping your partner address their weaknesses.
Based on my reading of code review literature, those parts of a relationship are obvious and not worth discussing.
Does this sound like a good ebook to you? I’m assuming you just yipped “Nonononono!”
So, why is that the way we talk about code reviews?
I can only assume the articles I’ve read are from the future, where all developers are robots. In that world, your teammates welcome thoughtlessly-worded critiques of their code because processing such information warms their cold, robot hearts.
I’m going to make the bold assumption that you want to improve code reviews in the present, where your teammates are humans. I’ll make the even bolder assumption that a positive relationship with your colleagues is an end in itself and not simply a variable you adjust to minimize your cost-per-defect. How would your review practices change under these circumstances?
In this article, I discuss techniques that treat the code review as not only a technical process but a social one as well.
What is a code review?
The term “code review” can refer to a range of activities, from simply reading some code over your teammate’s shoulder to a 20-person meeting where you dissect code line by line. I use the term to refer to a process that’s formal and written, but not so heavyweight as a series of in-person code inspection meetings.
The participants in a code review are the author, who writes the code and sends it for review, and the reviewer, who reads the code and decides when it’s ready to be merged in to the team’s codebase. A review can have multiple reviewers, but I assume for simplicity that you are the sole reviewer.
Before the code review begins, the author must create a changelist. This is a set of changes to source code that the author wants to merge in to the team’s codebase.
A review begins when the author sends their changelist to the reviewer. Code reviews happen in rounds. Each round is one complete round-trip between the author and reviewer: the author sends changes, and the reviewer responds with written feedback on those changes. Every code review has one or more rounds.
The review ends when the reviewer approves the changes. This is commonly referred to as giving LGTM, shorthand for “looks good to me.”
Why is this hard?
If a programmer sends you a changelist that they think is awesome, and you write them an extensive list of reasons why it’s not, that’s a sensitive message to get across.
That’s one reason I don’t miss IT, because programmers are very unlikable people… In aviation, for example, people who greatly overestimate their level of skill are all dead.
-Philip Greenspun, co-founder of ArsDigita, excerpted from Founders at Work
It’s easy for an author to interpret criticism of their code as an implication that they are an incompetent programmer. Code reviews are an opportunity to share knowledge and make informed engineering decisions. But that can’t happen if the author perceives the discussion as a personal attack.
As if this wasn’t difficult enough, you also have the challenge of conveying your thoughts in writing, where the risk of miscommunication is higher. The author can’t hear your tone of voice or see your body language, so it’s even more important to articulate your feedback carefully. To an author who’s feeling defensive, an innocuous note like, “You forgot to close the file handle,” can read as, “I can’t believe you forgot to close the file handle! You’re such an idiot.”
Between interruptions like meetings and emails, the time you have available to focus on code is scarce. Your mental stamina is in even shorter supply. Reading a teammate’s code is cognitively taxing and requires a high level of concentration. Don’t squander these resources on tasks a computer can do, especially when a computer can do them better.
Whitespace errors are an obvious example. Compare how much effort it takes for a human reviewer to find an indenting mistake and work with the author to correct it as opposed to just using an automated formatting tool:
Effort required with a human reviewer
Effort required with a formatting tool
Reviewer searches for whitespace issues and finds incorrect indentation.
Reviewer writes a note calling out the incorrect indentation.
Reviewer rereads their note to make sure that it’s worded in a clear, non-accusatory way.
Author reads the note.
Author corrects the code indentation.
Reviewer verifies that the author addressed their note properly.
Nothing!
The right side is empty because the author uses a code editor that automatically formats the whitespace every time they hit “Save.” At worst, the author sends their code out for review, and the continuous integration solution reports that the whitespace is incorrect. The author fixes the issue without the reviewer ever having to care.
Look for mechanical tasks in your code reviews that you can automate away. Here are the common ones:
Task
Automated solution
Verify the code builds
Continuous integration solution, such as Travis or CircleCI.
Verify automated tests pass
Continuous integration solution, such as Travis or CircleCI.
Verify code whitespace matches team style
Code formatter, such as ClangFormat (C/C++ formatter) or gofmt (Go formatter).
Identify unused imports or unused variables
Code linters, such as pyflakes (Python linter) or JSLint (JavaScript linter).
Automation helps you make more meaningful contributions as a reviewer. When you can ignore a whole class of issues, such as the ordering of imports or naming conventions for source filenames, it allows you to focus on more interesting things like functional errors or weaknesses in readability.
Automation benefits the author as well. It allows them to discover careless mistakes in seconds instead of hours. The instant feedback makes it easier to learn from and cheaper to fix because the author still has the relevant context in their head. Plus, if they have to hear about a dumb mistake they made, it’s much easier on their ego if they hear it from a computer instead of from you.
Work with your team to build these automated checks directly into the code review workflow (e.g., pre-commit hooks in Git or webhooks in Github). If the review process requires the author to run these checks manually, you forfeit most of the benefit. The author will invariably forget on occasion which forces you to continue reviewing for the simple issues that automation is meant to handle instead.
Settle style arguments with a style guide
Arguments about style are a waste of time in reviews. Consistent style is certainly important, but a code review is not the time to bicker about where to put the curly braces. The best way to excise style debates from your reviews is by keeping a style guide.
A good style guide defines not only superficial elements like naming conventions or whitespace rules but also how to use the features of the given programming language. JavaScript and Perl, for example, are packed with functionality — they offer many ways to implement the same logic. A style guide defines The One True Way of doing things so that you don’t end up with half your team using one set of language features while the other half uses a totally different set of features.
Once you have a style guide, you don’t have to waste review cycles arguing with the author about whose naming conventions are best. Just defer to the style guide and move on. If your style guide doesn’t specify a convention about a particular issue, it’s generally not worth arguing about. If you encounter a style issue your guide doesn’t cover and it’s important enough to discuss, hash it out with your team. Then, record the decision in your style guide so you never have to have that discussion again.
Option 1: Adopt an existing style guide
If you search online, you can find published style guides ripe for the taking. Google’s style guides are the most well-known, but you can find others if this style doesn’t suit you. By adopting an existing guide, you inherit the benefits of a style guide without the substantial costs of creating one from scratch.
The downside is that organizations optimize their style guides for their own particular needs. For example, Google’s style guides are conservative about using new language features because they have an enormous codebase with code that has to run on everything from a home router to the latest iPhone. If you’re a four-person startup with a single product, you may choose to be more aggressive in using cutting-edge language features or extensions.
Option 2: Create your own style guide incrementally
If you don’t want to adopt an existing guide, you can create your own. Every time a style argument arises during a code review, raise the question to your whole team to decide what the official convention should be. When you reach agreement, codify that decision in your style guide.
I prefer to keep my team’s style guide as Markdown under source control (e.g., GitHub pages). That way, any changes to the style guide go through the normal review process — someone has to explicitly approve the change, and everyone on the team has a chance to raise concerns. Wikis and Google Docs are acceptable options as well.
Option 3: The hybrid approach
By combining options 1 and 2, you can adopt an existing style guide as your base, and then maintain a local style guide to extend or override the base. A good example of this is the Chromium C++ style guide. It uses Google’s C++ style guide as a base, but makes its own changes and additions on top of it.
Start reviewing immediately
Treat code reviews as a high priority. When you’re actually reading the code and giving feedback, take your time, but start your review immediately — ideally, within minutes.
If a teammate sends you a changelist, it likely means that they are blocked on other work until your review is complete. In theory, source control systems allow the author to branch, continue working, and then forward-merge changes from the review into their new branch. In reality, there are about four developers total who can do that efficiently. It takes everyone else so long to untangle three-way diffs that it can cancel out any progress made waiting for the review to come back.
When you start reviews immediately, you create a virtuous cycle. Your review turnaround becomes purely a function of the size and complexity of the author’s changelist. This incentivizes authors to send small, narrowly-scoped changelists. These are easier and more pleasant for you to review, so you review them faster, and the cycle continues.
Imagine that your teammate implements a new feature that requires 1,000 lines of code changes. If they know you can review a 200-line changelist in about 2 hours, they can break their feature into changelists of about 200 lines each and get the whole feature checked in within a day or two. If, however, you take a day to do all code reviews, regardless of size, now it takes a week to get that feature checked in. Your teammate doesn’t want to sit around for a week, so they’re incentivized to send larger code reviews, like 500-600 lines each. These are more costly to review and yield poorer feedback because it’s more difficult to keep context on a 600-line change than a 200-line change.
The absolute maximum turnaround on a review round should be one business day. If you’re struggling with a higher-priority issue and can’t complete a round of review in under a day, let your teammate know and give them the opportunity to reassign it to someone else. If you’re forced to decline reviews more than about once per month, it likely means that your team needs to reduce its pace so that you can maintain sane development practices.
Start high level and work your way down
The more notes you write in a given review round, the more you risk making the author feel overwhelmed. The exact limit varies by developer, but the danger zone generally begins in the range of 20-50 notes in a single round of review.
If you’re worried about drowning the author in a sea of notes, restrict yourself to high-level feedback in the early rounds. Focus on issues like redesigning a class interface or splitting up complex functions. Wait until those issues are resolved before tackling lower-level issues, such as variable naming or clarity of code comments.
Your low-level notes might become moot once the author integrates your high-level notes. By deferring them to a later round, you save yourself the nontrivial work of writing carefully-worded comments calling out the issues, and you spare the author from processing unnecessary notes. This technique also segments the layers of abstraction you focus on during the review, helping you and the author work through the changelist in a clear, systematic way.
Be generous with code examples
In an ideal world, the code author would be thankful for every review they receive. It’s an opportunity for them to learn, and it protects them from mistakes. In reality, there are a number of external factors that could cause the author to perceive the review negatively and resent you for giving them notes. Maybe they’re under pressure to meet a deadline, so anything other than your instant, rubber-stamp approval feels like obstruction. Maybe you haven’t worked together much, so they don’t trust that your feedback is well-intentioned.
A great way to make an author feel good about the review process is to find opportunities to give them gifts during the review. And what’s the gift all developers love to receive? Code examples, of course.
If you lighten the author’s load by writing out some of the changes you’re suggesting, you demonstrate that you are generous with your time as a reviewer.
For example, imagine that you have a colleague who is not familiar with the list comprehensions feature of Python. They send you a code review that includes these lines:
Responding, “Can we simplify this with a list comprehension?” will annoy them because now they have to spend 20 minutes researching something they’ve never used before.
They will be much happier to receive a note like the following:
Consider simplifying with a list comprehension like this:
urls = ['https://' + domain + path for path in paths]
This technique is not limited to one-liners. I’ll often create my own branch of the code to demonstrate a large proof of concept to the author, such as breaking up a large function or adding a unit test to cover an additional edge case.
Reserve this technique for clear, uncontroversial improvements. In the list comprehension example above, few developers would object to an 83% reduction in lines of code. In contrast, if you write a lengthy example to demonstrate a change that is “better” based on your own personal taste (e.g., style changes), code examples make you look pushy instead of generous.
Limit yourself to two or three code examples per review round. If you start writing the author’s whole changelist for them, it signals that you don’t think they’re capable of writing their own code.
Never say “you”
This one is going to sound weird, but hear me out: never use the word “you” in a code review.
The decisions you reach in a review should be based on what makes the code better rather than who came up with the idea. Your teammate put significant effort into their changelist and is likely proud of the work they did. Their natural reaction to hearing criticism of their work is to feel defensive and protective.
Word your feedback in a way that minimizes the risk of raising your teammate’s defenses. Be clear that you’re critiquing the code, not the coder. When an author sees “you” in a comment, it brings their focus away from the code and back to themselves. This increases the risk that they’ll take your criticism personally.
Consider this harmless comment:
You misspelled ‘successfully.’
The author can interpret that note in two very different ways:
Interpretation 1: Hey, good buddy! You misspelled ‘successfully.’ But I still think you’re smart! It was probably just a typo.
Interpretation 2: You misspelled ‘successfully,’ dumbass.
Contrast this with a note that omits “you”:
sucessfully → successfully
The latter note is a simple correction and not a judgment of the author.
Fortunately, it’s easy to rewrite your feedback to avoid the word “you.”
Option 1: Replace ‘you’ with ‘we’
Can you rename this variable to something more descriptive, like seconds_remaining?
becomes:
Can we rename this variable to something more descriptive, like seconds_remaining?
“We” reinforces the team’s collective responsibility for the code. The author may move on to a different company and so might you, but the team who owns this code will remain in one form or another. It can sound silly to say “we” when it’s clearly something you expect the author to do themselves, but silly is better than accusatory.
Option 2: Remove the subject from the sentence
Another way to avoid using “you” is to use a shorthand that omits the subject from the sentence:
Suggest renaming to something more descriptive, like seconds_remaining.
You can achieve a similar effect with the passive voice. I generally avoid the passive voice like the plague in my technical writing, but it can be a helpful way of writing around “you”:
This variable should be renamed to something more descriptive, like seconds_remaining.
An additional option is to phrase it as a question, beginning with “what about…” or “how about…”:
What about renaming this variable to something more descriptive, like seconds_remaining?
Frame feedback as requests, not commands
Code reviews require more tact and care than usual communication because there’s a high risk of derailing the discussion into a personal argument. You would expect reviewers to dial up their politeness in reviews, but bizarrely I’ve found them to go the opposite direction. Most people never say to a co-worker, “Hand me that stapler, then fetch me a soda.” But I’ve seen numerous reviewers frame feedback with similarly pushy commands, such as, “Move this class to a separate file.”
Err on the side of being annoyingly gentle in your feedback. Frame your notes as requests or suggestions, not commands.
Compare the same note framed in two different ways:
Feedback framed as command
Feedback framed as request
Move the Foo class to a separate file.
Can we move the Foo class to a separate file?
People like to feel in control of their own work. Making a request of the author gives them a sense of autonomy.
Requests also make it easier for the author to push back politely. Maybe they have a good reason for their choice. If you frame your feedback as a command, any pushback from the author comes across as disobedience. If you frame your feedback as a request or a question, the author can simply answer you.
Compare how combative the conversation seems depending on how the reviewer frames their initial note:
Feedback framed as command (Combative)
Feedback framed as request (Cooperative)
Reviewer: Move the Foo class to a separate file. Author: I don’t want to do that because then it’s far away from the Bar class. Clients will almost always use the two together.
Reviewer: Can we move the Foo class to a separate file? Author: We could, but then it’s far away from the Bar class, and clients will generally use these two classes together. What do you think?
See how much more civil the conversation becomes when you construct imaginary dialog to prove your point frame your notes as requests instead of commands?
Tie notes to principles, not opinions
When you give the author a note, explain both your suggested change and the reason for the change. Instead of saying, “We should split this class into two,” it’s better to say, “Right now, this class is responsible for both downloading the file and parsing it. We should split it up into a downloader class and parsing class per the single responsibility principle.”
Grounding your notes in principles frames the discussion in a constructive way. When you cite a specific reason, like, “We should make this function private to minimize the class’ public interface,” the author can’t simply respond, “No, I prefer it my way.” Or rather, they can, but it would look silly because you demonstrated how the change satisfies a goal, and they just stated a preference.
Software development is both an art and science. You can’t always articulate exactly what is wrong with a piece of code in terms of established principles. Sometimes code is just ugly or unintuitive, and it’s hard to pin down why. In these cases, explain what you can, but keep it objective. If you say, “I found this hard to understand,” that’s at least an objective statement, as opposed to, “this is confusing,” which is a value judgment and may not be true for every person.
Provide supporting evidence where possible in the form of links. The relevant section of your team’s style guide is the best link you can provide. You can also link to documentation for the language or library. Highly-upvoted StackOverflow answers can work as well, but the farther you stray from authoritative documentation, the shakier your evidence becomes.
This is the second half of my article about how to communicate well and avoid pitfalls in code reviews. Here, I focus on techniques to bring your code review to a successful close while avoiding ugly conflict.
I laid the groundwork in Part One, so I recommend starting there. If you’re impatient, here’s the short version: a good code reviewer not only finds bugs but provides conscientious feedback to help their teammates improve.
My worst code review
The worst code review of my life was for a former teammate I’ll call Mallory. She started at the company several years before I joined but had only recently transferred to my team.
The review
When Mallory sent me her first changelist for review, the code was a bit rough. She had never written Python before, and she was building on top of a clunky, legacy system that I maintained.
I dutifully recorded all of the issues I spotted, 59 in total. According to the review literature I’d read, I had done a great job. I found SO many mistakes. Therefore, I must be a good reviewer.
A few days later, Mallory sent me the updated changelist and her responses to my notes. She had fixed the simple issues: typos, variable renames, etc. But she refused to address the higher-level problems, such as the fact that her code had undefined behavior for malformed input or that one of her functions nested control-flow structures six layers deep. Instead, she explained dismissively that these issues were not worth the engineering time to fix.
Angry and frustrated, I sent a new round of notes. My tone was professional but meandering into the realm of passive-aggressive. “Can you explain why we want undefined behavior for malformed input?” As you might guess, Mallory’s replies became even more obstinate.
A bitter cycle
It was Tuesday, a week later. Mallory and I were still going back and forth on the same review. I had sent her my latest notes the evening before. I purposely withheld them until she left for the day because I didn’t want to be in the same room when she read them.
All morning, I felt a sinking weight in the pit of my stomach as I dreaded the next round of review. I came back from lunch to see that Mallory was away from her desk but had sent me new changes. I guess she didn’t want to be around to see me read her replies either.
My heart began pounding in my chest as I grew more infuriated by each of her responses. I immediately started hammering my keyboard with rebuttals, pointing out that she had neither made my suggested changes nor offered justification for me to approve.
We repeated this routine every day for three weeks. The code barely changed.
Intervention
Our most senior teammate, Bob, thankfully broke this cycle. He returned from a long vacation, alarmed to find us bitterly flinging code review notes back and forth. He immediately recognized the situation for what it was: a stalemate. He requested to take over the review, and we both agreed.
Bob began his review by asking Mallory to create new changelists, splitting off two small libraries that we had never really fought about, each about 30-50 lines. Once Mallory did that, Bob instantly approved them.
Then, Bob came back to the main changelist, which was trimmed down to about 200 lines of code. He made a few small suggestions, which Mallory addressed. Then, he approved the changelist.
Bob’s entire review was done in two days.
Communication matters
You may have deduced that this conflict wasn’t really about the code. It had legitimate issues, but they were clearly solvable by teammates who could communicate effectively.
It was an unpleasant experience, but one I’m glad for in retrospect. It caused me to reevaluate my approach to reviews and identify areas for improvement.
Below, I share techniques that will reduce your risk of a similarly undesirable outcome. I’ll return to Mallory later and explain why my original approach was backward and why Bob’s was quietly brilliant.
While your teammate might, in theory, want to explore every opportunity to improve their code, their patience is finite. They’ll quickly grow frustrated if you withhold approval round after round because you keep thinking of new and brilliant ways for them to polish their changelist.
I privately think of the code in terms of letter grades, from A to F. When I receive a changelist that starts at a D, I try to help the author bring it to a C or a B-. Not perfect, but good enough.
It’s possible, in theory, to bring a D up to an A+, but it will probably take upwards of eight rounds of review. By the end, the author will hate you and never want to send you code again.
You might be thinking, “If I accept C-grade code, won’t I end up with a C-grade codebase?” Fortunately, no. I find that when I help a teammate go from a D to a C, the next changelist they send me will start at a C. Within a few months, they’re sending me reviews that begin as Bs, which become As by the end of the review.
An F is reserved for code that is either functionally incorrect or so convoluted that you don’t have confidence in its correctness. The only reason you should withhold approval is if the code remains at an F after a few rounds of review. See the section on stalemates, below.
Limit feedback on repeated patterns
When you notice that several of the author’s mistakes fit the same pattern, don’t flag every single instance. You don’t want to spend your time writing the same note 25 times, and the author certainly doesn’t want to read 25 duplicate notes.
It’s fine to call out two or three separate instances of a pattern. For anything more than that, just ask the author to fix the pattern rather than each particular occurrence.
Respect the scope of the review
There’s an anti-pattern I see frequently where the reviewer identifies something near code in the changelist and asks the author to fix it. Once the author complies, the reviewer usually realizes that the code is better but inconsistent, so it needs a few more minor changes. And then a few more. And on and on until a narrowly-scoped changelist has expanded to include lots of unrelated churn.
If a hungry little mouse shows up on your doorstep, you might want to give him a cookie. And if you give him a cookie, he’ll ask for a glass of milk. He’ll want to look in a mirror to make sure he doesn’t have a milk mustache, and then he’ll ask for a pair of scissors to give himself a trim…
The rule of thumb is: if the changelist doesn’t touch the line, it’s out of scope.
Here’s an example:
Even if you’ll be kept awake all night, haunted by the magic number and ridiculous variable name in your codebase, it’s out of scope. Even if the author is the same person who wrote the nearby lines, it’s still out of scope. If it’s egregiously bad, file a bug or submit your own fix, but don’t force it onto the author’s plate in this review.
The exception is when the changelist affects the surrounding code without actually touching it, for example:
In this case, point out that the author needs to rename the function from ValidateAndSerialize to just Serialize. They haven’t touched the line containing the function signature, but they still caused it to become incorrect.
I softly break this rule if I don’t have many notes but notice an easy fix just out of scope. In these cases, I make it clear that the author can ignore the note if they please.
Look for opportunities to split up large reviews
If you receive a changelist that’s more than ~400 lines of code, encourage the author to split it into smaller pieces. Push back proportionally harder the more they go over this limit. I personally refuse to review any changelists that exceed 1,000 lines.
The author may gripe about splitting the changelist because it’s a tedious task. Ease their burden by identifying logical boundaries for the split. The easiest case is when the changelist touches multiple files independently. In that case, they can just split the changelist into smaller sets of files. In harder cases, find the functions or classes at the lowest layer of abstraction. Ask the author to move these to a separate changelist, then circle back to the rest of the code after the first changelist is merged in.
When the code quality is low, emphatically request a split. The difficulty of reviewing bad code grows exponentially with size. You’re much better off auditing a couple of sloppy 300-line changelists than a single 600-line abomination.
Offer sincere praise
Most reviewers focus only on what’s wrong with the code, but reviews are a valuable opportunity to reinforce positive behaviors.
For example, imagine you’re reviewing for an author who struggles to write documentation, and you come across a clear, concise function comment. Let them know they nailed it. They’ll improve faster if you tell them when they got it right instead of just waiting to ding them when they screw up.
You don’t need to have a specific goal in mind to offer praise. Any time I see something in the changelist that delights me, I tell the author about it:
“I wasn’t aware of this API. That’s really useful!”
“This is an elegant solution. I never would have thought of that.”
“Breaking up this function was a great idea. It’s so much simpler now.”
If the author is a junior developer or joined the team recently, they’re likely to feel nervous or defensive during a review. Sincere compliments ease this tension by demonstrating that you are their supportive teammate and not the cruel gatekeeper.
Grant approval when remaining fixes are trivial
Some reviewers have the misconception that they should withhold approval until they witness fixes for every last note. This adds needless code review rounds, wasting time for both author and reviewer.
Grant approval when any of the following are true:
You have no more notes.
Your remaining notes are for trivial issues.
E.g., renaming a variable, fixing a typo
Your remaining notes are optional suggestions.
Explicitly mark these as optional so that your teammate doesn’t assume your approval is contingent on them.
I’ve seen reviewers withhold approval because the author missed a period at the end of a code comment. Please don’t do this. It signals to the author that you think they’re incapable of adding simple punctuation unless supervised.
There is some danger in granting approval when there are still outstanding notes. I estimate that ~5% of the time, the author either misinterprets a final round note or misses it completely. To mitigate this, I simply check the author’s post-approval changes. In the rare case of miscommunication, I either follow up with the author or create my own changelist with a fix. Adding a small amount of work to the 5% case is better than adding unnecessary effort and delay to other 95%.
Handle stalemates proactively
The worst possible outcome of a code review is a stalemate: you refuse to sign off on the changelist without further changes, but the author refuses to make them.
Here are some indicators that you’re headed for a stalemate:
The tone of the discussion is growing tense or hostile.
Your notes per review round are not trending downward.
You’re getting pushback on an unusually high number of your notes.
Talk it out
Meet in person or over video chat. Text communication has a way of causing you to forget there’s a real human at the other end of the conversation. It becomes too easy to imagine your teammate is coming from a place of stubbornness or incompetence. A meeting will break that spell for both you and the author.
Consider a design review
A contentious code review may indicate weaknesses earlier in the process. Are you arguing about things that should have been covered during the design review? Was there a design review?
If the root of the disagreement traces back to a high-level design choice, the broader team should weigh in rather than leave it in the hands of the two people who happen to be in the code review. Talk to the author about opening up the discussion to the rest of your team in the form of a design review.
Concede or Escalate
The longer you and your teammate stew in stalemate, the more damaging it is to your relationship. If alternatives haven’t gotten you unstuck, your options are to either concede or escalate.
Weigh the cost of just approving the changes. You can’t build quality software if you casually accept low-quality code, but you also can’t achieve high quality when you and your teammate fight so bitterly that you can no longer work together. How bad would it really be if you approved the changelist? Is it code that could potentially destroy critical data? Or is it a background process where, at worst, the job will fail and require a developer to debug it? If it’s closer to the latter, consider simply conceding so that you can continue working with your teammate on good terms.
If concession is not an option, talk to the author about escalating the discussion to your team’s manager or tech lead. Offer to reassign to a different reviewer. If the escalation goes against you, accept the decision and move on. Continuing to fight it will drag out a bad situation and make you look unprofessional.
Recovering from a stalemate
Messy review arguments tend to be less about the code and more about the relationship between the people involved. If you reached stalemate or near-stalemate, this pattern will repeat if you don’t address the underlying conflict.
Discuss the situation with your manager.
If there’s conflict on the team, your manager should know about it. Maybe the author is just difficult to work with. Perhaps you’re contributing to the situation in ways you don’t recognize. A good manager will help both of you address these issues.
Take a break from each other.
If possible, avoid sending each other code reviews for a few weeks until things cool down.
Study conflict resolution.
I found the book Crucial Conversations to be helpful. Its advice may sound common-sense, but there’s tremendous value in analyzing your approach to conflict while you’re not in the heat of an argument.
My worst code review: revisited
Remember the code review with Mallory? Why did mine turn into a three-week slog through passive-aggressive muck while Bob’s was a two-day breeze?
What I did wrong
This was Mallory’s first review on the team. I failed to consider that she might feel judged or defensive. I should have started out with only high-level comments so that she didn’t feel ambushed by the large volume of notes.
I should have done more to demonstrate that my job wasn’t to obstruct her work, but rather help it move forward. I could have provided code examples or called out the positives in her changelist.
I allowed my ego to affect the review. I had spent the past year nursing this old system back to health. Suddenly, there was a new person futzing with it, but she couldn’t be bothered to take my concerns seriously? I took it as an affront, but that attitude was counterproductive. I should have maintained the objective mindset I try to bring to all of my reviews.
Finally, I allowed the stalemate to drag on too long. After a few rounds, it should have been clear to me that we weren’t making meaningful progress. I should have made a drastic change, such as meeting in person to address the deeper conflict or escalating to our manager.
What Bob did right
Bob’s first move of splitting up the review was very effective. Recall that the review that had been stalled for three painful weeks. Suddenly, two pieces of code were merged in. This made both Mallory and Bob feel good because it established forward momentum. There were still issues with the remaining chunk, but it became a smaller, easier-to-manage changelist.
Bob didn’t try to strangle the review to perfection. He likely recognized the same issues that I was screaming about but realized Mallory would be on the team awhile. His flexibility in the short-term positioned him to help Mallory improve quality in the long-term.
Conclusion
After I published the first half of this article, several readers took issue with the communication style I recommended. Some found it patronizing. Others worried that it was too indirect and risked miscommunication.
That feedback is reasonable and expected. One person may find a terse review comment to be brusque or rude. Another may judge the same comment as concise and efficient.
In reviewing code, you make many choices: what to focus on, how to frame feedback, when to approve. It’s not important that you choose my options. Just recognize that there are options.
No one can hand you a recipe for a perfect review. The techniques that work best will depend on the code author’s personality, your relationship with them, and your team’s culture. Hone your approach by thinking critically about the outcomes of your code reviews. When you encounter tension, take a step back to evaluate why it happened. Pay attention to the quality of your reviews. If you feel unable to bring code up to your quality standards, think about what aspects of the review process are hindering you and how you can address them.
Good luck, and may your code reviews be human-like.
