Code Reviews with a Smile

Google Seattle has a weekly event called "Gathering" - a team meeting for all engineers in the office where anyone can talk about anything of interest to Googlers. Topics range from overviews of various projects people are working on, to discussions of the new technologies, to presentations by various researchers from academia.

I took the last couple of slots to talk about the human aspects of the code reviews.

I was doing a lot of these lately. Being a readability reviewer for JavaScript guarantees you at least one code review per week, typically consisting of 3 or 4 iterations. Plus, I have a bunch of readabilities in other languages, and often volunteer to review code for teams who have nobody with readability (more on readability at Google here: http://1-800-magic.blogspot.com/2008/01/after-8-months-no-longer-noogler.html).

The talk turned out a success, and so I decided to blogify it :-).

When I was at Microsoft, I was no fan of mandatory code reviews. I thought it was impractical to have every line of code read by someone else, and I also was afraid that people would use code reviews as a substitute for testing (to an extent, this does happen at Google, which has much smaller test organization than Microsoft).

Well, Google does require code reviews for every check-in, so I had to convert - and I did. As it often happens, new coverts become zealots :-). So not only I ended up enthusiastically supporting the system, but joined Google Readability team as well.

Why did I convert? Basically, I looked at the code base, and was impressed. Microsoft does not have a unified style, and a lot of the code looks jagging to the eyes of a person who did not write it.

It's like an accent - small groups of people that communicate in relative separation from the main body of the language carriers develop it. In case of developers whose only communication is the compiler, a very strong "personal" accent develops, and the resulting code takes an effort to understand by an outsider.

The code reviews smooth the accent by expanding the group of people who speak the same code. Having a single style guide expands the accent to the entire company, so any part of the code looks like you wrote it yesterday. For me at least it improves the productivity dramatically.

Also, I noticed that the developers tend to write better code upfront if they expect that someone else will be looking at it - out of the sheer embarrassment :-).

But beyond the company benefits, there are two reasons why I personally love doing the code reviews.

First is because I learn a lot from them myself. Being exposed to code written by people in different corners of the company is the best way to keep abreast of the many projects you would otherwise have never heard about. Right now I am in the process of reviewing code that I am pretty sure I will use myself - and if not for the code review, I would never knew it existed.

Second is because it gives me an opportunity to teach. After more than 20 years of programming computers, there's a bunch of stuff I know, and recalling it during code reviews keeps it alive :-).

However, once we take the code out of the purely machine environment of compile-run-test-check in, it becomes a human communication. Human communications have the emotional aspect which is missing entirely from the computer-bound interaction of reviewless programming.

Thus code reviews add an entirely new dimension - above and beyond technical aspects, they are an instance of a human communication, and a very sensitive one at that - because in the process of a code review, one developer render opinion about the work of another engineer. Tread carefully!

Studies upon studies have shown that people use the emotions first, and the logic next. So the emotional rapport between a reviewer and the reviewee can have a bigger effect than the information that trades hands in the process. "How" can - and often does - become more important than "what".

I think about most human interactions as the bank transactions involving trust. You make a deposit into your account when you do something that the other person likes. You make a withdrawal when you have the other person do something that you like (but the other person might not).

In terms of code reviews, what is important for a reviewee? How could a reviewer increase his or her balance?

I think that the most important things for a reviewee is the latency - the sooner the review (or at least an iteration of the review) is done, the better.

While a code review is outstanding, a reviewee is often barred from continuing the work with the same set of files, because the newer changes could involve the same code that is not yet checked in pending the review.

If the latency is low, even big changes requested by the reviewer are easier to make - but if the reviewer sits on the code for two weeks, then requests a total rewrite, and then sits on it for two weeks again before declaring that the first version was better after all - it's entirely a different matter. The tension builds.

Another case where the stress tends to build up if when the reviewee works against a hard deadline. But the reviewer often does not live on the same shipping cycle, and is not taking the rush into account. This is good, of course - it helps ensure that the bad code does not get checked in purely to satisfy the timing constraints. But it is very important that the reviewer realizes that the pressure is building - and acts in ways that help relieve it.

What else is important for a reviewee? I'd say the style with which the reviewer acts, the civility of the communication. People generally do not tell their office mate - "Hey, you! Fetch me a chair!". But I've had many, many times seen the review comments say "Rename this variable to foo."

If you and your reviewer has worked together for a while, and built up considerable deposit in their trust account, it is OK to spend a little bit of it to conserve typing. But do not save on civility when reviewing the code for a stranger!

Here are my recommendations for the reviewer:

• Respect the reviewee's time
  
  
  
  • Maintain quick turnaround
  
  
  • Do not ask for small tweaks that do not matter. Does this variable really need renaming? If it were your code, would you care to check out, change, and retest 10 files just to rename this function or not?


• Order not, suggest
  
  
  
  • “Consider naming this foo because this is what it’s named everywhere else in the codebase.”
  
  
  • “If you use bar instead, it could save you some code.”
  
  
  • “Moving baz here could shave a few clocks from the execution path”


• Respect reviewee's opinion
  
  
  
  • (S)he has spent days thinking about this. You have spent less than an hour...
  
  
  • In case of conflicting approaches (AKA religious disagreements), it’s the reviewee who ultimately owns the code – (s)he should have the priority


• Praise! Nothing else creates good will more effectively than this.
  
  
  
  • "Great CL, thank you for doing this!"


• Golden rule: review others’ code as you want your code to be reviewed

Now, let's look at it from the reviewer perspective. What does one want when he or she reviews someone else's code?

I personally like my efforts to be recognized. There is a reason I am doing this, right? I want my opinion to be respected :-).

As a reviewer, I want my time to be used effectively.

And, it goes without saying, I want the product's code base to be as good as possible.

What does this mean for a reviewee?

• If it’s not too hard, it is often easier to just do it
  
  
  
  • If you find yourself pushing back on almost everything your reviewer suggest, one of you may be unreasonable
  
  
  • If you do it often with different people, the unreasonable person is you…


• Respect reviewer's time
  
  
  
  • Smaller change lists
  
  
  • More comments, both in code and in change description


• Recognize stellar reviewers at the performance review time
  
  
  
  • A note dropped to the person's manager endorsing a stellar reviewer will make his or her day


• And, of course, the Golden rule again!

Have your own story of a code review that went badly because of lack of rapport between the people involved? Did I miss an important point? Write about it here!