How I Learned to Like Code Review

2010-05-03

Code review is a development process where each time you write some code someone else is supposed to review the code before you check it in. As far as I know, most teams do not practice code review. Certainly most game development teams? I know that until I came to Google I had zero experience with code review. I had read about it and I mostly thought about how annoying it sounded to have someone have to review at my code. Now that I've experienced it at Google though I'm a fan.

I think I was really lucky to experience code review the "Google" way because I suspect it's a better experience than most other places. I'll point out why as I go over the 5 things I think it took to start liking code review.

1) It's nice when a reviewer saves your reputation.

I write some code, I assume it's going to work. Maybe even wrote some unit tests. Still, I can't think of everything. When someone reviews my code and they find some issues I forgot about I feel much more confident that the code is solid. Maybe they see a memory leak or a potential buffer overflow. Maybe it's an off by 1 bug or maybe it's a simple optimization. Whatever it is I feel the usefulness to have another pair of eyes at least glancing at the code who might see obvious mistakes. If they do find a mistake it also gives me the sense that they actually looked at the code.

2) I love it when I learn something new from a reviewer.

One of the things code review provides is a chance to share knowledge. It could be a better ways to code something or a better pattern that solves the problem more elegantly. It could be pointing out functions or libraries, internal or external, that already solves the issue.

A few examples from my own code getting reviewed: When I first started at Google I added an RTTI system to some code not realizing one already existed. The reviewer asked why I wasn't using the current system and I was like "DOH! I didn't even know we had one" and quickly refactored my code to use it. Those types of comments were great for getting up to speed when I joined the project.

In another case I was writing secure code that required checking for overflow. The reviewer pointed out a quick way to check for integer multiplication overflow is

unsigned int v = a * b;
if (b != 0 && v / b != a) {
  // error, overflow
}

Both little things and big things get passed on through code review.

3) It's great to have the person who originally wrote the code verify you're using it correctly.

If someone writes a class, library or set functions and I write some code using or modifying them it's great when I can easily ask the guy who wrote the original code if I'm using his code as it was intended to be used. He can check that I'm not fighting his code with the wrong set of assumptions.

4) Possibly most important thing is I think specific to Google. Google has AWESOME code review tools.

All I have to do is create a changelist in p4 or svn or git and type a simple command in the form of

gcl upload changelist -r person_to_review@mycompany.com

The system uploads the files to a web server and sends email to the reviewers as well as to me with a link to the specific review webpage. This system is open source and there is also a free running service you can use to try it out.

It's pretty damn slick. If you go to the URL sent in the mail you'll see your changelist description along with the list of files changed or added. You can click on any file and view a side by side diff of the files with all the changes highlighted. If you see any issues you just double click on a line and start adding comments. When you're finished you publish the comments and they're sent to everyone involved. The email includes all the comments along with direct links into the review system to the place in the code they reference. You can click "done" as in "I dealt with the issue you mentioned on your comment as you suggested" or you can reply, adding comments of your own about why the code is the way it is or discussions about what the reviewer was getting at.

When everything is good the reviewer will reply "aokay". Here at Google we use "LGTM" which stands for "Looks Good To Me" to signal that it's okay to commit the code.

If you'd like to try it out and you have subversion around, assuming you have python installed, download upload.py, edit some files in your project (or create a new one) and then run upload.py.

python upload.py -r your@email.com

It should upload your files to the public review system and give you a URL to visit them. It will also send an email to the person email address you listed on the command line. Check out the URL to see what would be sent to the reviewer.

The green lines have been added.

As a reviewer you double−click any lines you have a comment for and enter your remarks.

Hopefully some useful comments.

You then publish the comments

Publish them back to the programmer.

Upon receiving the comments, mark them as done or add comments back.

Reply with your updates.

When you agree the code is good, commit it. It's that simple.

The system is called Rietveld. It's open source. The version checked in uses Google App Engine as the backend but there are public patches that will let you run in on your own servers if you don't want to use App Engine.

5) I guess the last one is prompt reviewers.

Most of the time a co−worker will review the code within a hour or two. I find most of the time I have plenty of other things to work while I wait for that review. I go to another client and start on something else.

There are somethings that can make code review suck. Having to wait too long is one. Another is code nazis who either want the code written their way, which is not actually better, just different. Or, they harp on things like style or grammar. For example I had one reviewer complain about a comment that was something like "This part of the code does a, b and c". His comment was there needs to be a comma after b. Sorry dude, that's not a useful review comment IMO.

In those cases it helps to have a thick skin, to not take it personally and if it really doesn't matter to the code then just give in and make the change. When giving comments always try to phrase things as a question. It's easier to read "could this be a std::vector?" rather than "bare pointers suck! Use an std::vector!" etc.

All 5 of those things above are what helped me enjoy code reviews. I'll definitely code reviews it if I ever go back to a company that doesn't use them .... as long as their process is as easy as the one above 😉

Comments
Apple to Ban another 100+ existing games from iPhone
Using a Mock Library to make Unit Testing Easier in C++