Humane code review

In the year I’ve been at Mozilla I’ve submitted almost two hundred patches for review. My email archives suggest that I’ve had 26 reviews not granted, and 164 granted.

I’ve also been reviewing patches for Sync since January of 2011. And in the past few months our team — and thus the number of people whose code I review, or see being reviewed — has grown significantly.

This has given me a lot of food for thought on the topic of code review and motivation, so here’s a Saturday afternoon blog post!

From one valid (if extreme) perspective, we don’t own our code, and we have no investment in it. All code is bad, and a positive review brings only grim satisfaction. A negative review is a joyous opportunity to learn, not a personal insult. We toil beneath our hooded robes.

Very few people can be this detached from their output. (I try!)

A “review granted” or “review not granted” email brings most of us a burst of emotion; even having reached a very low-ego place with respect to my code (it’s hard to make it through two hundred code reviews and still derive my self-worth from a patch!), I still feel good when I get r+, and a mild shot of frustration and disappointment when I get r-. Despite the fantastic opportunity to learn inherent in each review, we need to fight through our emotions to get to the point of learning. Marko Kloos writes about a similar process in his discussion of writers’ rejection letters.

If I still sometimes feel these things, I thought, then what about people who aren’t like me? Perhaps they’re volunteer contributors with other demands on their time. Perhaps they’re just less motivated, or have more self-worth riding on each patch. Perhaps they are junior engineers who feel that they have to prove themselves. A blunt, factual, “this is totally wrong” review, as so many of us tend to give, can be a shattering blow to a person’s motivation. Even people with practice dissociating their ego from their work can take a motivation hit; it’s not just the personal involvement, but also the frustration at a setback.

I can already hear the comments: “but they shouldn’t feel like that! It’s just a code review!”. My rebuttal is “but people do, because most people are not like you, and not accounting for that can be damaging in all kinds of ways”.

For a lot of people, how you phrase your code review will dictate whether the patch ever lands at all, or at least how soon it lands.

I’ve experienced a few approaches to code review that I think might help.

The absolute first thing is to provide a useful review. Just like an email, if your review doesn’t tell the author what they need to do to get the patch approved, then they need to double their effort: they not only have to fix the patch, but they have to put in the time to find out how first! The more clearly you can state what you want, and ideally how to get there (“try looking at this test to see how to do it”), the more likely it is that the author will be able to keep on rolling.

The second is an elaboration on the first: engage. Don’t just throw an r-; walk over (or call, or IRC) and talk through it, perhaps pairing to get to a patch that’ll pass review. Rather than just “no”, spend a little time to make sure that the contributor is actually moving forward and motivated. That might mean you spend some time trying to solve someone else’s problem. That’s fine; it’s an investment in the code, just like code review, but it’s also an investment in the author!

The third is to be careful with your wording. Follow the same kinds of tips that you read in magazines about having a constructive argument: avoiding finger-pointing, focusing on solutions or improvements rather than problems, etc. This is a little bit of a hack, but people don’t stop being people when they open Bugzilla. There is a difference between “you forgot to clear session state here, so the following test exercises the completely wrong thing (you idiot)” and “we should make sure that the following test still works when you clear session state at the end of this one, like this…”. It’s small, but a few of those per review, and a few reviews a month, will eventually leave a sour taste in a contributor’s mouth. Do you want people to be apprehensive about asking you for review?

On a related note, be careful with your flagsMarco Bonardo once reviewed one of my patches. I was new to that component, and he gave me very detailed feedback on everything from style to direction. Something that stuck in my mind was that he cleared the review flag and set feedback+, rather than review-. The patch wasn’t anywhere near good enough to land, but rather than coming across as a rejection it was presented as “great start, keep it up!”. This put me in a positive mood and encouraged me… and I’m pretty unfazed about negative reviews! That technique went straight into my reviewing toolbox. (This is the same kind of diplomatic response that normal human beings use all the time in social situations: “do you like my new jacket?” “it really complements your jeans!”.)

Similarly, as a patch author I sometimes find a flaw in my own code and clear my review? flag; as a reviewer one can do the same, which has a smaller ego hit than that big minus sign.

Does anyone else have any good tips?