The gentle art of code review
The gentle art of code review — Bitfield Consulting

If the programmers like each other, they play a game called “pair programming”. And if not, then the game is called “peer review”.

—Anna Nachesa

“Brutal”. “Minefield”. “Frustrating”. All these words polled higher with people asked to describe their experiences of code review than the one I’ve used in the title of this piece. Why is that, though? Why does code review suck so badly, and what can we do about it?
Code review is a necessary part of software development, but it’s one that can cause a great deal of friction, anxiety, conflict, and wasted time and effort, when it’s done poorly—and it mostly is.
How can we get better at it? Could we give and receive code reviews with kindness, gentleness, humility, and compassion? What kinds of comments add value to code, and build trust and safety with our co-workers? How do we accentuate the positive, cope with wounded vanity, and make a point without making an enemy?
The life and times of a changeset
Let’s start by talking about what code review is and when it happens. We’ll use three friends of mine, Aisha, Bryan, and Chiku, as our example.
Usually the workflow is something like this: Aisha writes some code (or Aisha and Bryan write some code as a pair) and it gets checked into version control, usually on a branch. Aisha can now make a pull request, also known as a merge request, which, when approved by Chiku, would merge all her changes—her changeset—with the main line.
Alternatively, some teams push changes directly to the main line, sometimes also periodically pulling from it into a release branch to build versioned releases, if necessary. Either way, there’s usually a step where the changeset has to be approved, maybe by someone more senior, but often just by someone else on the team.
As part of this approval, Chiku should review the code, where “review” means at least “read”, but most likely also to make comments or to ask questions about it. A conversation then ensues between Aisha, Bryan, and Chiku, and usually a few further changes are made, before the merge is eventually approved.
Pair reviewing
Notice that we haven’t said this conversation needs to happen through the version control software itself: for example, as comments on a GitHub pull request. Actually, the best way to have it is by talking, which is an unbelievably high-bandwidth communication channel compared to those little text boxes.
In other words, code should be pair-reviewed as well as pair-programmed. For example, Aisha and Chiku could get on a screen-share and look at the code together, asking questions, making suggestions, explaining ambiguities, and highlighting potential problems.
It’s far quicker to have an exchange like this synchronously, by voice:

CHIKU: Is this int down-conversion safe? What if the value is bigger than 32 bits?
AISHA: It can’t be, because if you look right here, it comes from…
CHIKU: Oh, I see now. What if we put in a comment here, just to make that clear?
AISHA: Sure… (TYPES)

That took just fifteen seconds by my watch, but if they’d had to go round this loop a few times via GitHub comments, it probably would have pushed the merge back till Tuesday. Plus, Aisha and Chiku are probably still good friends, whereas lengthy code review debates have been known to jeopardise even the best relationships.
Tone of voice and even facial expressions convey a lot of extra information that would otherwise be lost, and somehow, people are always gentler and more respectful to each other in person than they are when typing messages back and forth.
Perhaps for this very reason, you’ll sometimes encounter people who insist on trying to make the discussion asynchronous and text-only. The simplest way to foil this tactic is not to join in. If they send you comments on your code via GitHub or email, just reach out to them in person and ask to have a chat about it.
If they’re still unwilling, then so be it. Evidently they don’t want a conversation; they just want to talk while you listen admiringly. That’s unlikely to be helpful, so you should bypass them altogether and find someone who is interested in a genuine conversation about the code.
Over time, you’ll build up a useful network of relationships with people who can “pair review” code with you, and your code will be far better as a result (theirs, too).

Pair review like this isn’t always possible, though, so when a code
review happens textually, that means we need to take even more
care about what we say and how we say it. Let’s start with the
“what”.
A good way to think about code review is as a process of adding
value to existing code. So any comment you plan to make had better
do exactly that. Here are a few ways to phrase and frame the different
kinds of reactions you may have when reviewing someone else’s code:

Not my style. Everyone has their own style: their
particular favourite way of naming things, arranging things, and
expressing them syntactically. If you didn’t write this code, it won’t
be in your style, but that’s okay. You don’t need to comment about that;
changing the code to match your style wouldn’t add value to it. Just
leave it be.
Don’t understand what this does. If you’re not sure what
the code actually says, that’s your problem. If you don’t know
what a particular piece of language syntax means, or what a certain
function does, look it up. The author is trying to get their work done,
not teach you how to program.
Don’t understand why it does that. On the other hand, if
you can’t work out why the code says what it says, you can ask
a question: “I’m not quite clear what the intent of this is. Is there
something I’m not seeing?” Usually there is, so ask for clarification
rather than flagging it as “wrong”.
Could be better. If the code is basically okay, but you
think there’s a better way to write it that’s not just a style issue,
turn your suggestion into a question. “Would it be clearer to write…? Do
you think X is a more logical name for…? Would it be faster to re-use
this variable, or doesn’t that matter here?”
Something to consider. Sometimes you have an idea that
might be helpful, but you’re not sure. Maybe the author already
thought of that idea and rejected it, or maybe they just didn’t think of
it. But your comment could easily be interpreted as criticism, so make
it tentative and gentle: “It occurred to me that it might be a slight
improvement to use a sync.Pool here, but maybe that’s just
overkill. What do you think?”
Don’t think this is right. If it seems to you like the
code is incorrect, or shouldn’t be there, or there’s some code missing
that should be there, again, make it a question, not a rebuke.
“Wouldn’t we normally want to check this error? Is there some reason why
it’s not necessary here?” If you’re wrong, you’ve left yourself a
graceful way to retreat. If you’re right, you’ve tactfully made a point
without making an enemy.
Missed something out. The code is fine as far as it
goes, but there are cases the author hasn’t considered, or some
important issues they’re overlooking. Use the “yes, and…” technique:
“This looks great for the normal case, but I wonder what would happen if
this input were really large, for example? Would it be a good idea
to…?”
This is definitely wrong. The author has just made a
slip, or there’s something you know that they don’t know. This is your
opportunity to enlighten them, with all due kindness and humility. Don’t
just rattle off what’s wrong; take the time to phrase your response
carefully, gracefully. Again, use questions and suggestions. “It looks
like we log the error here, but continue anyway. Is it really safe to do
that, if the result is nil? What do you think about
returning the error here instead?”

Accentuate the positive
We often think about code review as a process of pointing out what’s
wrong, but there’s also a great opportunity to praise what’s
right. Indeed, your review should begin with a description of
everything you like about the code. “This is really well organised, and
I like how you’ve used X, Y, and Z to give extra context for readers.
The function names are great, and this seems like it would be a nice API
to use.”
A spoonful of sugar helps the medicine go down, and even when you
have to give someone feedback that’s hard to hear, there’s always
something good you can find to say, too. Lead with the positive, follow
up with your tactful, respectful comments and suggestions, and finish
the review with something encouraging: “Great job! This is really
shaping up nicely.”
If you’re reviewing code from someone who’s senior to you, though,
it’s probably not appropriate to tell them they did a great job. They
might feel, rightly or wrongly, that you’re not yet in a position to
judge that. Instead, you could say something like “I really learned a
lot from this, thanks for giving me the opportunity to look at it.”
When your vanity is wounded
Your self-esteem can take a hit from code reviews, especially if
they’re not done tactfully, and that hurts.
You put your heart and soul into a piece of work, you send it off
with fond expectations that it’ll be hailed as a triumph, and then it
comes back with brutal comments all over it.
You feel wounded, you feel angry and resentful, and you’re tempted to
hit back. “Oh yeah? Well let me tell you, buddy…”
Needless to say, this isn’t a good idea. Your reviewer almost
certainly means well, even if it doesn’t come across that way.
If they don’t mean well, and they intend their remarks to be
dismissive and hurtful, then sure, they’re a jerk.
But the world is full of jerks. It’s not your job to turn them all
into decent human beings, and you’re certainly not getting paid for
it.
Instead, just suck it up and move on. There’s probably more truth in
their feedback than you’re prepared to acknowledge at first, even if
it’s expressed in an unnecessarily rough way.
Not everyone is a master of people skills, and even the kindest of
colleagues can have a bad day for reasons totally unrelated to you or
your pull request. Tout comprendre, as they say, c’est tout
pardonner: if we could truly know the secrets of someone’s heart,
we would never think harshly of them.
“I see what you tried to do
here”
Some people’s code review comments, while not exactly rude, can be
very condescending, which is almost as bad. You know the kind of thing:
“Hey, you probably didn’t realise this, but you should use a pointer
receiver here. Let me show you how that works…”
Perhaps they’re trying to be nice, but actually, what we
hear is “You don’t have a clue what you’re doing. Lucky I was
here to save you from making a total ass of yourself.” Indeed. The funny
thing is that people who act like this usually don’t know very much
themselves, and maybe that’s why they’re so quick to point out to
others what they don’t know.
This happens a lot in the tech industry, and you’ll find that those
who know the most often don’t say much, while those who know nothing
seem to talk all the time.
Maybe that’s because, in tech, people can often get away with being
unaware of their own incompetence. Not all industries are so
forgiving.

In aviation, for example, people who greatly overestimate their
level of skill are all dead.
—Phillip Greenspun, quoted in Jessica Livingston’s “Founders at Work”

Stylistic nitpicks
We saw earlier that “not my style” comments don’t really add value to
code; they just create churn. Every time this particular section of code
is updated, it will also accumulate wasteful and random changes to suit
the “style” of whoever happens to be reviewing it. So, while you won’t
be giving these comments, what should you do when you receive
them?
My suggestion, which may seem a trifle radical, is simply to
ignore them. Just because someone has made a comment, it
doesn’t mean that you automatically have to respond to it. If you don’t,
they’re unlikely to follow up.
If they do, and if they continue to insist on the changes (assuming
they’re in a position to insist), you can have a private conversation
with them to clarify exactly what they think the blocking issue is.
Is it purely a matter of style, or is that masking something
more important that they haven’t adequately explained? You’re entitled
to ask them politely if there’s something they can’t live with.
Setting the example
Over the course of your career, which we
may hope will be a long and successful one, you’ll find that
understanding people and relationships is a more valuable skill than any
technical acronyms on your resume. Collaboration is a tricky business,
and the only thing harder than developing software is developing
software as a team.
When you treat those around you with kindness, consideration, and
respect, you’ll teach them the gentle art of code review in the best
possible way: by example. Looks good to me.
