Skip to content

Code Reviews: A Framework for Startups

You know how your teachers and parents always told you to get a friend to proofread your paper before you handed it in for marks? Well, it’s age-old advice for a reason.

There’s a lot of electronic papers, ie. code, being handed in here at Summify and when most of that code will be affecting a @#$@ load of data, reviews become critically important. Until last week, our squad captains, Mircea Pasoi and Cristian Strat, were reviewing all the Summify code – not the most sustainable practice. It was time to bring the entire team into the code review process, but we needed a structure to help guide the move. The following framework has been working well for us and we wanted to share it with you.

Our tool of choice for code reviews is Review Board.

Getting Your Review On

1) Goals

Eliminate bugs as early as possible

Share knowledge to learn other sections of the code, new libraries, tricks, and algorithms

2) Take your time

Doing so will speed things up. It may sound like a paradox, but finding bugs in later stages, such as post-release, can cost 10-100 times more than it would cost to catch them during review. The following data on fixing bugs is from the book Code Complete.


If you ever catch yourself thinking “Oh, I’m kind of busy, should I really bother nitpicking this piece of code?” the answer is always Yes. You might find a bug which will save your team hours upon hours of work down the road – think longer term.

3) Ensure correctness

When done properly, a code review is one of the most effective ways of detecting bugs. A proper code review will detect about 55% of bugs, double the effectiveness of unit tests (see the below table from Code Complete).

Everyone has experienced those moments when someone looks at your code for 10 seconds and points out a ginormous filthy bug, right under your nose, despite the fact that you were looking at it for the last 3 hours. Sadly for them, it doesn’t mean they’re 12,000 times smarter than you; they just have the fresh eye and the clear mind.

When you’re reviewing someone’s code, you’re the fresh onlooker and you’re in a very good place to spot bugs, so do it.

4) Demand quality

Don’t be afraid to ask for unit tests, good refactoring, and adherence to coding style. Never accept code that’s of lower quality than the average code found in the repository.

Demanding quality during reviews begets quality further down the road, leading to faster test writing, second nature coding style and fewer lazy habits, refactoring code when necessary. Remember, you’re not slowing things down, you’re just forcing people to learn how to write good code, fast.

Also, always ask for screenshots when the change involves UI.

Besides being a code reviewer, it’s equally important to submit good code for review. Here are some tips:

Submitting Quality Code – That’s Your Job

1) Don’t rely on reviewers to find bugs – submit code with confidence

You should be so sure of your code quality, personally unable to find any more bugs, that you’d be happy to deploy it into production right away, even without a review process. Your reviewer may come up with something you haven’t thought of, but never lower your own standards.

2) Reviewers have a tough life

Understand that your reviewer only sees a diff and some bug types are very hard to detect this way. For instance, if your change requires that you search for all occurrences of function X in the code, your reviewer cannot confirm you did an exhaustive search. That’s your responsibility.

It’s also your responsibility to think about the implications of your changes. Does it break the iPhone? Does it break the archiving process? Does it grind the website to a halt? Realizing these implications is 80% of an engineer’s job. This is the meat, the hard questions engineers must think through. Remember the time when you thought something over for five hours and then made a two-line change? The hard part was figuring out if that was the right change, and that’s your job; it’s not the reviewer’s.

If you find yourself thinking “Hmm… I’m gonna change this but I don’t know if I’m breaking stuff,” then you’re not doing your job, and you’re offloading your work to the reviewer.

3) Run the tests

Reviewers won’t do this for you and they can’t figure out if you break tests. Better yet, implement continuous integration – we use Jenkins.

4) Leave no comment behind

When submitting a second or third diff for the same change, your reviewer assumes that for every comment he or she wrote you either

a) implemented the change, or
b) replied to it

Don’t expect the reviewer to go through the list of previous comments and validate that you’ve addressed each one. That’s your job.

5) Attach screenshots if you’ve made a UI change.

6) Learn and improve

Learn from mistakes and shape up your code. Shoot for a 90% “perfect ship-it” rate. Every time your reviewer has to return your code, a kitten dies. When you pass 90% it means the code review process really works and everyone’s doing a great job.

Sharing the code review responsibilities across our team is really paying off and soon we’ll be leaps and bounds ahead of where we were just a few weeks ago. It feels like we’ve upgraded to Summify v1.1. A few more upgrades like this and we’ll be an unstoppable kick-ass team, sort of like a Canadian version of The A-Team, or an organized crime syndicate – the Summify Mafia. Anyways… until then, we’ll continue asking ourselves “how can we improve team processes and communication to increase productivity?”

Are you doing code reviews? Share your thoughts, productivity tips and questions below.

P.S. Please, think of the kittens.

About these ads

12 Comments Post a comment
  1. Tibi #

    Nice post.

    Can you give a rough approximation of how long it takes for a diff to get shipped from the moment it was put on reviewboard on average?

    I worked with code reviews at almost all the jobs I had and although I did find it very useful, sometimes it might be blocking you from building a bigger change, as sometimes bigger features are composed of several patches.

    Is there a way to mention a diffs priority in reviewboard? Like let’s say maybe a diff is blocking someone (the person working on it can’t work on anything else until that diff goes through) and another diff fixes a critical bug. Or is this problem solved through verbal comunication inside the team?

    September 21, 2011
    • Mircea Paşoi #

      Probably ~1h on average, although some stuff can take up to 24h to get shipped (b/c of multiple iterations on the reviews).
      Verbal communication definitely helps and if something is really really urgent you can do the review after pushing and deploying the code (post-commit code review).

      September 22, 2011
  2. I don’t use the Summify product any more but hadn’t gotten around to unsubscribing from the blog feed. This was a pleasant surprise to me to read this. It’s something I’m just starting to learn about and it was incredibly helpful and I passed it on to a lot of team members. Thanks for posting!

    September 22, 2011
    • Robin Campbell #

      Thanks for sharing James! We’re glad you found it helpful. If you’d like to let us know how Summify could be better suited to you please drop us a line at team[at]summify.com.

      September 22, 2011
  3. A.A.A. #

    Should the reviews always be more skilled and experienced than the developer ? What if the reviewr is wrong in some task (don’t know some patterns or refactoring paradigms, or don’t accept some knowledge from good book)

    September 23, 2011
    • Mircea Paşoi #

      Not necessarily. Mistakes will happen, but in a creative environment avoiding mistakes is often more expensive than making them and fixing them fast.

      September 23, 2011
  4. Great article, guys! Code-review is probably that one thing that totally shifted my perspective on how code should be developed/deployed.

    One question: why do screenshots are that important when reviewing code/posting a review?

    Almost all the time, I download the diff, patch it locally and test the change. Any UI change is visible.

    September 23, 2011
    • Mircea Paşoi #

      You don’t always want to download the diff and apply it, and screenshots are super easy to review in ReviewBoard.

      September 23, 2011
  5. Can you please expand on how you use continuous integration / jenkins?

    September 24, 2011
    • Mircea Paşoi #

      Whenever someone pushes code to the “master” branch Jenkins automatically runs all the unit tests on a dedicated machine. If anything fails, an email is sent that the “build is unstable”.

      September 24, 2011
      • Tibi #

        Why don’t you do this before commit? User should not be allowed to commit code without successfully passing test-cases. Facebook released the thing they’re using to the public https://github.com/facebook/arcanist. It’s pretty neat.

        October 2, 2011
      • First of all, the stuff Facebook open-sources isn’t too good/stable. Second, doing this before commit would only be a small benefit for such a small company, we can communicate fast enough that it’s not a real problem.

        October 3, 2011

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Follow

Get every new post delivered to your Inbox.

Join 88 other followers