Jan 15 2014

Code Review in Four Steps

When I started at Big Nerd Ranch, I was starved for code review. I had received very few deep, insightful comments on the code I had produced up to that point. Then the code review process at Big Nerd Ranch changed the game for me. 

one does not simply do code review

I quickly found out how valuable code review is—not just as a programmer, but also as a reviewer. I began learning about new strategies and tools for solving problems, along with new ways to use the tools I already had. And reviewing code myself forced me to think about my preconceptions of code and figure out how to effectively communicate my views to others.

The Process

The code review process is quite simple, really:

  1. Read all code written by a developer over the last few days.
  2. Understand the changes.
  3. Offer actionable feedback.
  4. Follow up with discussion.

Read

You should look at the diff itself as well as the context that it exists in. To do so:

  1. Open up the project and select the target branch.

    branch

  2. View the commits.


    commits

  3. Starting with the oldest commit you would like to review, read them in chronological order.

As you read through the commits, consider the notes and analyze the diff. You’ll begin to build a mental model of the story being told by the commits. With experience, you’ll be able to quickly decide what is important enough to devote time to, and what may be quickly breezed over (such as whitespace commits).

Understand

It’s one thing to read code and another thing entirely to understand it. You can start your review by looking for obvious mistakes such as typos, syntax errors, stale comments and the like. You should also consider the readability of the code. If the role of an object is unclear, you may consider commenting on its responsibility or naming. (Naming is a notoriously difficult problem, and code review can be a great way to work fuzzy names into concrete identifiers that clearly communicate the purpose of objects and methods.)

The goal here is to understand another programmer’s decisions. This may involve viewing more context or asking questions. If you have difficulty grokking a set of changes, that might indicate a flaw in the design.

After gaining understanding, you are ready to comment on the code.

Respond

The best question to ask yourself when leaving a comment is: “Is this comment actionable?” While it is encouraging to litter code with “+1″ and “Awwww yeah! Dat code is HAWT!”, that kind of feedback can grow old.

Remember: Every comment you make will notify the programmer whose code you’re reviewing, and potentially consume his or her time with reading and following up. 

Your comments should include a call to action:

  • “Have you considered eager-loading these records to reduce the number of queries needed? (i.e. SomeModel.includes(:related_models))”
  • “This object appears to carry too much responsibility. You may consider extracting each responsibility into individual objects to more clearly define the boundaries of logic. What do you think about that?”

Notice that the last example is followed up with a question. This reminds the person you’re reviewing that your word is not gospel, but rather a suggestion. The developer may have good reason behind his or her decisions, and should be given the benefit of the doubt.

When reviewing, always ask yourself if a comment you’re about to leave is actually valuable. There are many times that I’ve typed a long comment and subsequently thrown it out because it really didn’t offer value. Yes, throwing it out is a waste of your time, but sending it may end up wasting the developer’s time as well. 

Discuss

If you’ve provided excellent comments on someone’s work, conversation will spring up. Further discussion may be needed in order to find a suitable solution, or the reviewee may simply disagree with you. Be ready to support your opinions, and don’t be afraid to admit your own error.

Go forth and provide value!

Confronting your assumptions about the way code should look and feel forces you to clearly articulate those ideas. You will inevitably grow as a result, and I am amazed by how quickly knowledge spreads in both directions when code review is taken seriously by everyone involved.

Do you practice code review? Does your process differ? Let us know how!

10 Comments

  1. Dave Worth

    :+1: is valuable to me. As are non-actionable comments such as “I don’t know the solution here but lines 7-379 seem weird. I would say the prior could distract or encourage depending on the reviewee. The latter is still valuable regardless of the audience.

    Thanks for the insight Jay!

    • Great point! Sometimes I consider the desire to gush praise on the reviewee a hint that I’m not reading with a critical enough eye. But there will be times that the subject is producing incredible work, which totally deserves some love :). When I feel like I “smell” something in the code, I try to dig a little deeper and see if I can offer anything actionable, but you’re right that just casting a net can be enough to let the subject know that attention is needed.

      Thanks for the comment, Dave!

      • Steven Harman

        Code reviews are as much an opportunity to learn, and teach, as any other. As such, I’ll often ask questions or put forth an observation, without a clearly defined action, in an attempt to spark more conversation during which a better way forward might emerge. I find this tactic particularly useful when used within a Pull Request-based workflow as the context and scope of change are more clearly defined and focused.

        And one big thing to remember, which you’ve mentioned, but warrants reiterating – check your ego at the door because at some point, you’re going to be wrong. ;)

        • Jonathan Wallace

          This! In my opinion, the best thing about code review is an opportunity for diffusion of style and conceptualization about the problem domain.

          For this reason, I try to let a code review be as “noisy” with comments as I feel the need to be. I let the onus be on the reviewed person to respond as they see fit.

  2. Steve

    But which projects to review?

    • We use an assignment rotation. Each dev is assigned a dev to review. For an iteration or two part of your job is to review the artifacts produced by your assignment. If they work on a single project, your focus is there. If they’re spread across multiple, you may want to ask where they would like you to focus or review ALL the things :)

  3. Marco Pivetta

    Nice blogpost!

    I may add that being too picky is not a problem with people that know you: they will start learning and will avoid common mistakes later on.

    And now for a question: I find it practically impossible to provide good reviews without an open pull request (where old comments are hidden by github/bitbucket automatically), and therefore I don’t review until a PR is in place. Don’t you think that commenting on single commits causes a lot of useless noise that could be avoided assuming that the author fixes his own issues in his wip branch?

    • Thanks, Marco!

      In practice, I agree that it often doesn’t make a lot of sense to review code until it’s “final”. We don’t _always_ use PRs, so it does sometimes make sense to review commits by author on a branch. When I’m reviewing a programmer that isn’t using a PR workflow, I’ll often dive into their “staging” branch (presumably with “done”, close to client-accepted features, but not necessarily in production) and look for commits to review. We also encourage interaction between reviewer/reviewee on deciding what to review. They may be actively working on a feature branch and want some input as they go.

      Inevitably working through commits may lead to unnecessary comments. I will sometimes “read forward” briefly if something catches my eye as an “obvious” mistake before I comment. We’re always thinking about better ways to “collect” content to perform reviews on :)

Leave a Comment

Join the discussion. Do not worry, your email address will not be published.

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>