How We Code Review

The Theory

“…large numbers of defects are often more attributable to the complexity and risk of a piece of code than to the author’s abilities.” – Jason Cohen

Code review is like test-driven development, version control, and brushing your teeth. Everyone knows they’re good ideas and yet not everyone does them. At Unbounce we review all code changes before they’re allowed anywhere near the master branch, our implicit assumption being that reviews result in better code quality. While an individual alone might improve the codebase, working together we most certainly will.

Code Review Fundamentals

At it’s core a good code review strives for a few fundamental objectives:

Reduce Complexity

One of the most useful comments a reviewer can make is “This code seems too complicated”. It takes a brave reviewer to push back against complexity, as complexity is often mistaken for cleverness and effort. It feels good to write complex code but unfortunately it usually feels bad to read it, and in complexity hide bugs. Simpler is always better.

Share Techniques

The second most useful comment a reviewer can make is “I know a better way…”. In that phrase there is sharing, perhaps debate, ultimately education. This is especially powerful when the “better way” reduces line-count. If you aim only for one improvement from code reviews, I’d suggest always aim for smaller, simpler code.

Sanity-check Decisions

Did the developer really mean to delete that file? Did they know there’s already an interface that contains that function? Is a 400-line class file really a good idea? A good code review also asks the high-level questions.

Generate Discussions

Sometimes a change just doesn’t feel right. Perhaps the reviewer thinks a new chunk of code is in the wrong location, or new tests are stressing aspects they shouldn’t be. In that case “I don’t think this…” can be a powerful way to generate discussion about best-practices or processes, and clear up potential misunderstandings. If something seems strange, push back for clarification.

Validate Best-practices

Of course you have a coding styleguide, don’t you? Make adherence to it part of your code reviews. It can seem pedantic, trivial even, to push back a change because someone uses tabs over spaces, or uses explicit return in their Ruby methods, but at the end of the day it’s all about the code quality. Predictable, familiar code is comprehensible code.

But What About the Bugs?

“We’ll review the code and we’ll kill all the bugs”, right?

Probably not.

A code review is not necessarily about finding bugs. In fact I’d argue that finding bugs is almost a side-effect, like finding $5 in the street because you went outside today. Sure it might happen sometimes but I wouldn’t wake up every morning expecting it.

That said, if your codebase is really crap then maybe that’ll happen often (finding bugs, not finding $5; a money-finding codebase would be awesome), but if your design is good, your functions small, your classes well-intentioned, your test coverage complete, if your developers really understand the problem before sitting down at the keyboard, then finding bugs should not be the primary benchmark for your code reviews. If it is you might have some bigger problems.

Now, About Your Code…

Before we can do all that though, we need to accept that everyone writes sub-optimal code sometimes. Every single one of us. Yes, even John Carmack (probably… maybe). Bruce Schneier once said of cryptosystems:

“Anyone, from the most clueless amateur to the best cryptographer, can create an algorithm that he himself can’t break.”

Same goes for code: anyone can write code so good they themselves cannot improve upon it. Yet like cryptosystems code is best improved through aggressive exposure to others, by being examined, questioned, defended and improved. Thus it is critically important to remember one thing: it is much easier to be a critic than an artist. We are not our code; our ego is not derived from specific lines typed, but rather the state of the entire final product.

Code reviews should not be not personal; ego should play as little role as possible.

The Practice

Anatomy of a Review

We’ve talked about what a code review is (and is not), now let’s talk about how a code review happens. How does code actually get reviewed? What’s the process? At Unbounce we follow a few simple rules:

Reviews are done on branches

I’ve heard tell of places that merge new changes into master and then do a code review. That’s just crazy. Run-away crazy.

In the sane world we develop on a topic branch, submit a pull request, review the request in isolation, and merge only on acceptance.

Reviews are done by others

Developers can’t review their own code. Or rather they can, but that’s just called “writing code”. It’s critically important that someone else do the review because by the time we’ve finished writing it, chances are we’ve developed a pernicious form of myopia for it. Write once/read once, yeah? It needs someone else’s eyes.

Reviews are not “in person”

Some places seem to think it’s a great idea to have the source developer sit with the reviewer and walk them through their changes. We think that’s a terrible idea. If code and design decisions must be explained in person (undoubtedly accompanied by copious hand-waving) then the code clearly violates the “simplicity” tenet and should be pushed back.

Write clearer code, fix it with comments, or reduce the scope of the problem instead. If it can’t be reviewed by a remote worker, send it back.

Reviews are not optional

Reviewing everything, like always looking both ways before crossing the street, should become a defacto habit. As soon as some code is allowed to circumvent review (“it’s a small, harmless change, lets just push directly to master”) a dangerous precedent gets set. A few lines here, a few lines there and next thing you know, whole new features are being shot-gunned through willy-nilly.

Be militant, review all changes.

Reviewers are assigned but not exclusive

At Unbounce the source developer usually assigns someone to do their review, the assumption being twofold:

1. They’ll pick a person well-qualified to review those changes.
2. They’ll pick someone who will be tough on them (it’s a problem if your developers are looking for easy, lazy reviews).

That said, anyone is free to review any code working it’s way through Review and Test Ready at any time. Just as no one truly owns any of the codebase, no one truly owns a code review.

No one is immune

It doesn’t matter if you’re a new hire or the CTO, all players are treated equally. No one’s code is too good for review.

No comments considered suspicious

Our experience has been that almost every pull request generates at least one comment from the the reviewer. Obviously this is most relative to the size and complexity of the change but as a general rule of thumb, not finding anything to say should prompt a closer look. And if there’s still nothing to say, say something nice. Code reviews don’t always have to be critical.

We Disagree… Let’s Fight!

What to do when the source developer and the reviewer disagree about a change? Who wins?

At Unbounce reviewers aren’t goalies; they don’t have the final say on whether code moves forward, they’re simply making recommendations. A developer could choose to ignore suggestions and comments and push an issue forward anyway, but by the same token if the reviewer strongly disagrees, they’re welcome at any point to raise the issue up with the dev team in general, usually through email, to get second opinions.

In our experience force-of-will pushes rarely happen, and group discussions tend to settle disagreements nicely.

Tools of the Trade

At Unbounce we use a plethora of different languages day-to-day, however most of our code is Ruby and Javascript. To that end, the tools we use to review our Ruby codebases are:

1. JIRA. We track every issue in JIRA, moving them from ‘To Do’ to ‘Coding’ to ‘Review’ to ‘Test Ready, etc.
2. Github. Which needs no introduction. Github is the glue that ties all these other pieces together.
3. SimpleCov. SimpleCov is our first line of defence. If the code doesn’t have test coverage there’s not much point in reviewing it.
4. TravisCI. Travis is something we’ve just started using, rolling it out slowly against our repos. So far it’s been great.
5. Code Climate. Code Climate tracks quality by comparing each branch against master. If a branch makes the quality of the codebase worse, it gets pushed back. If it introduces security issues, it gets pushed back. If it reduces coverage, it gets pushed back.

And that, in a nutshell, is code review at Unbounce.