Social Code Review

Everyone does code review, right? Right? If you’re shaking your head “no” right now, this article is for you. This article is not really about the technical content of a code review, but rather about the social aspects of it. And no, that doesn’t mean doing code reviews via Twitter...(that would be Sad!)

Ready for Launch?

If Peer Code Review (PCR) isn’t already part of your workflow, introducing it requires some care. Simply dropping a decree that Hencef’rth all code shalt beest subject to Code Revieweth will likely encounter some resistance, at least mentally. And why wouldn’t it? Giving and receiving criticism are learned skills many people lack. And doing that face-to-face can be even trickier.

For this reason I prefer asynchronous PCR via a tool, most of the time. There are a number of benefits to this:

●    The author needs to prepare the request-for-review with enough context for the reviewer. This means a description of the issue/feature and a test plan. These two things in of themselves can be quite useful for the author. Much like describing a problem helps solve a problem (aka Rubber Duck Debugging), writing up the requirements and test plan sometimes points out a flaw to the author.

●    Both reviewer and author are granted a little space and time to think things through.

●    Reviewers can schedule it into their day as convenient. Since there is likely to be some iterations of back-and-forth

We use Phabricator (an open source software development support suite, used by the likes of Facebook, Dropbox, and Pinterest to namedrop a few), but there are other tools that do the same thing.

Simply providing the technical infrastructure and workflow for PCR isn’t enough though. We need to establish how to do code reviews.

 The Playground Rules

You’ll need to set some ground rules about how to interact: every reviewer will need to provide their feedback in a non-judgemental way. This means avoiding emotionally laden words like “crappy”, “stupid”, and “incompetent”. Gosh, that sounds pretty simple, doesn’t it? Just skip the value judgements and stick to the facts? How about “when I see code like this, it turns my stomach”...? (not a joke, I saw that in a code review). Perhaps this is a fact to the reviewer, but it still doesn’t feel very good to the author.

You might be tempted to simply say “Don’t be a jerk” (or words that are more/less colourful, depending on your company culture). But everyone has a different idea of what that means. And of course, there are also cultural influences to consider: some cultures are highly conflict-averse and loathe to say anything that might be considered critical, while others are typically eschew what North Americans consider good manners.

The long and the short of it is that your team will need to agree on how they will work together. This will be different from shop to shop, and you’ll almost certainly need a few iterations of fine tuning the playground rules. You’ll need to keep a close eye on the reviews for the first while to make sure things don’t get out of hand.

A Personal Question

I believe that in the right environment, everyone truly wants to do their best work. There may be lots of reasons why the particular piece of code they’ve submitted doesn’t pass muster. Regardless of how it came to be that this code has been returned to them, authors have some responsiblities. They must dispassionately assess the comments made, and make a decision about each: is this important? Do I need to make a change here? Should I make a change here, even if it may not be necessary?

Such clear-eyed assessment might be easier said than done, of course. It’s easy to get defensive about code you’ve painstakingly crafted, and it may take a willful effort to bring the right mindset to it.

The key thing to remember is that it isn’t personal. We’ve all got the same goals, and we’re all working towards them together.

But Let’s Not Kid Ourselves...

It IS personal...nobody wants to have their code savaged by another, no matter how well regarded the reviewer is or how technically correct the reviewer may be. Some reviewers take a perverse pride in their caustic rants, claiming that they’re “telling it like it is”. This is smugly self-righteous balderdash: software development is a team sport, and part of being on a team is the ability to work well with others. Failure to work well with others is anti-social, the opposite of what we should be striving for.

So in the interest of Social Code Review, here are some attributes to cultivate:

Humility

Believe it or not, you may not know everything. It may be that you don’t understand what you’re reading. Perhaps there is some context you’re not familiar with. So seek first to understand, which may mean asking a bunch of questions.

Curiosity

Asking the why questions with curiosity (ie, "I wonder why she did it that way?") leads to an open mind, not only for examination of what was done, but also for what was not done. And this can lead you to exercising the limits of the code. Sure, you can test what it is supposed to do, but you can assume that most of the time that will work. After all, that’s what the author tested too. What about what the boundaries? Or the out-of-left-field inputs? What happens when something unexpectedly fails? This kind of questioning can shine a light on our unnoticed assumptions.

Desire to Improve

Code reviews are a two-way learning street. Both authors and reviewers can learn something. I recall looking at a completed code review that spanned many hundreds of lines of code. The reviewer had sent it back with dozens and dozens of comments. The author (a well respected technical leader) responded with “Thanks for the great review/feedback! Really helpful!”.

If your team can cultivate these attitudes and bring an open mind to the process of code review, not only will you see an improvement in the quality of your product, you’ll also see an improvement in the quality of your team and your work environment.