The gentle art of code review
From Code For Your Life
Your code review comments are making people actively wish you were dead. Please stop.
If the programmers like each other, they play a game called “pair programming”. And if not, then the game is called “peer review”.
“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
intdown-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).
Want to learn more about the people skills that will help you become a great developer? Code For Your Life is a friendly, funny book about succeeding in the tech industry, and building the rewarding career you want.
Using comments to add value
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.Poolhere, 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.
