Jake Herringbone

What code reviews ought to be

Google is the first place I’ve worked where code reviews are a formal part of every change. And I’m really sold on them. I will always make code reviews a part of committing code from now on, because it guarantees that someone else understands my code. Sure, it’s also handy if the reviewer notices a bug, or suggests a nicer API I could have used, or has some good refactoring ideas. But the most important result for me is “shared code stewardship” – code I write isn’t “my” code, it’s part of the team’s product and it’s going to need maintenance by other people. The code review is often the first chance for someone else on the team to really read through the code and understand my changes.

But, I think it sucks that no one read the code earlier. Code reviews are like a less effective form of pair programming.

The more you pair program, the more often you find that the code review is already done. Your pair can be your reviewer, they have already signed off on the implementation, and they’re a great co-steward of the code for the future. Or, as one team I’ve worked with does, you can send the review to a third person, just to have some extra familiarity with the changes that the pair has made.

By pairing on the change as you make it, the pair may have some input near the beginning of the coding session. As soon as you start to create a new class, and you give it a name, they might say, “hey, I think there’s this other pattern we might use for this,” and you change course immediately. Contrast that with a typical code review – you’ve got to have some pretty serious objections to someone’s code before you tell them to go back to the beginning and use a different design. You’ll never do it if the improvement is minor – it’s not worth changing it at that point.

So code reviews end up being more about style, minor nits, and reminders to fill in the documentation. That’s great, but it’s a poor substitute for having that person onboard sooner. Here’s another reason: if you want to suggest a design change in a code review, you’ve got to explain it in your comments. If you want to make a small nit like changing the order of import/require/include statements, you add a comment and make work for the author to make your change. In both cases, it would be better to just grab the keyboard and make the change, explaining your reasoning as you go. There’s no separate prose you need to write in a comment explaining the change.

So, here’s my proposal for an awesome code review tool:

• Pick the reviewer before you start making changes. They’re like your pair, but don’t have to be in your timezone.
• You commit to a throw-away branch as you work on your change. The branch is automatically set up for your review, and allows commits by the reviewer(s).
• When you commit on your branch, the reviewer gets notified, and can browse your code, in a read/write mode. Ideally it should be easy for the reviewer to load an IDE project from your review branch.
• The reviewer can add inline comments in the code, which don’t appear in the source file, but in some extra metadata file. They can also change the code directly. They commit their changes, with a commit comment, and the author is notified.
• The author considers the suggested changes, and can accept them or roll them back, with further comments if desired.
• Bonus: the tool facilitates easy screen sharing, so if you IM with the reviewer and don’t agree or understand, you just remotely pair on the spot.
• When there is some consensus and the change is done, the author does a merge back to the dev branch or trunk, and the review tool archives the review branch.

No comments yet.

Leave a comment