Code Reviews

The Goal of Code Reviews

The purpose of conducting a code review is to:

  1. Ensure the highest quality product.
  2. Provide quality feedback to a developer so that they can improve their craft.

Return on Investment (ROI) of Code Reviews

Conducting a code review takes time. At a minimum we're talking about 1 hour of time per User Story. Two people participate in the code review so, that's two man-hours. If programmers cost $100/hr, that's a $200 investment. What's the return like?

In short, properly conducted code reviews result in:

  • higher quality software which reduce carry costs.
  • more efficient software development process reduce production costs.

Return for the Coder

  • you presumably have done the best you can, already. Any feedback you get is likely to be a fresh perspective. Considering other perspectives will make you a better coder.
  • more often than not, you will learn something new: a new technique, a new way of looking at the problem, a new design approach.

Return for the Reviewer

in addition to the very same benefits the Coder experiences…

Return for the Product Owner

The party footing the bill is likely most interested in the payback. The ROI story for well-conducted design reviews is compelling:

Feature Benefit
Code Reviews reveal design and coding defects before they go to QA or BA review.
the scrutiny of a fresh pair of eyes often catches bugs and other issues that would have been missed and passed along down the development cycle.
Reduces development costs:
  1. in general, defect removal is almost always cheaper the earlier it happens in the process.
  2. defects are found before the QA and BA review the work, reducing their efforts (compare the QA effort when the feature "just works" vs. documenting a bug).
Code Reviews result in better cohesion.
When the team performs peer reviews, they pollenate their ideas of which most important is the consistent expression and use of design elements. This is best achieved where folks are forced to read the code; if they have to strain to understand what is a concept that exists elsewhere in the code base, the lack of cohesion can be detected.
Low cohesion and repetition often go hand-in-hand: the same concept expressed more than once is duplication. Detecting and eliminating duplication makes for easier to understand/maintain code.
Software that enjoys a consistent design is easier to maintain and thus reduces "carry costs"
  • future developers spend less time trying to understand the code they are fixing or modifying
  • elimination of duplication means its easier to extend or in otherwise modify that part of the system.
Code Reviews means that more of the team understands how more of the system works.
During a code review, the reviewer must come to understand what's happening in the code. Instantly, there are at least two people who have an intimate knowledge of this part of the code. If the team is good about spreading out the code reviews, the team can enjoy a depth of knowledge for the minimum effort.
The project team can better absorb roster changes and more flexibly execute avoiding inefficiency costs.
  • if more than one person knows about an area of the code, someone out sick or on vacation has less of a schedule impact as others can provide coverage.
  • if for some reason, a significant amount of effort in a given iteration deals with a specific part of the system, if the team members are already familiar with the code, they'll naturally be more productive.

Conducting a Code Review

Two Roles:

  • *coder* is the person mostly responsible for writing the code.
  • *reviewer* embodies the fresh perspective.

Attitude

Dear Coder,

As you sit down to have your code reviewed, do yourself a favor and figure out how to check your ego at the door. You're not always perfect. ;) This is not _your_ code, this is _the team's_ code. As hard as you might have tried, you might have missed a few things, here or there. Or in the throws of implementation you missed a step or misunderstood a requirement or chose a design that resulted in awkward code. It's okay. The way we get better is to do our best, _identify and learn from our "mistakes"_, and armed with that new information revise our skillz. If the reviewer provides you with constructive criticism, be on guard not to become defensive. If you can disidentify from your creation and rather focus on how this process (which sometimes includes constructive criticism) will help you become a better programmer; you'll not only _get_ better, but have fun in the process… and isn't that the point?

Dear Reviewer,

As you sit down to review someone's code, do you and your buddy a favor and check your ego at the door. You're NOT here to point out every little thing that's "wrong" with their code (in fact, err on the side of being gentle). You're merely role-playing; you're casted as the gal/guy who's just inherited this code. You're here to find out if you understand the code well enough to maintain it. You're a _partner_ with the Coder; you're not expected to somehow "know better" or some such. You're peers. Chances are, in fact, _you_ will be learning something in this process. Perhaps you _will_ be a maintainer of this code someday (you never know) or need to debug your way through here or you'll see something that The Coder did that can be a new tool in _your_ toolbox. Seek to understand as a primary goal. If you're relaxed and learning, not only will it put The Coder at ease, but you'll both have more fun in the process… and isn't that the point?

Guidelines

  1. Ask questions rather than make statements. A statement is accusatory. "You didn't follow the standard here" is an attack—whether intentional or not. The question, "What was the reasoning behind the approached you used?" is seeking more information. Obviously, that question can't be said with a sarcastic or condescending tone; but, done correctly, it can often open the developer up to stating their thinking and then asking if there was a better way.
  2. Avoid the "Why" questions. Although extremely difficult at times, avoiding the "Why" questions can substantially improve the mood. Just as a statement is accusatory—so is a why question. Most "Why" questions can be reworded to a question that doesn't include the word "Why" and the results can be dramatic. For example, "Why didn't you follow the standards here…" versus "What was the reasoning behind the deviation from the standards here…"
  3. Remember to praise. The purposes of code reviews are not focused at telling developers how they can improve, and not necessarily that they did a good job. Human nature is such that we want and need to be acknowledged for our successes, not just shown our faults. Because development is necessarily a creative work that developers pour their soul into, it often can be close to their hearts. This makes the need for praise even more critical.
  4. Make sure the discussion stays focused on the code and not the coder. Staying focused on the code helps keep the process from becoming personal. You're not interested in saying the person is a bad person. Instead, you're looking to generate the best quality code possible.
  5. Remember that there is often more than one way to approach a solution. Although the developer might have coded something differently from how you would have, it isn't necessarily wrong. The goal is quality, maintainable code. If it meets those goals and follows the coding standards, that's all you can ask for.
  6. Keep it focused. Stay focused on the code under review. If there are other problems that come up, make note of it and take care of that in another session. Also, no need to get nit-picky; if there is a style difference and there are no guidelines/coding standards, let it go.

Process

Guidelines

  • No longer than 45 minutes at a stretch. This is about the maximum length of solid concentration.

Step-by-Step

Zeroth Pass: The Personal Review

Conduct a code review with yourself: follow the steps described below on your own. You'll get lots of benefits:

  • Your silliest mistakes won't be seen by others (except VCS code goalies, of course)
  • Your pairing review will go faster and/or save time for the more interesting issues.

First Pass: The Talk-Through

  1. Review the requirements together (recontextualize).
  2. The *coder* identifies starting point(s). They take the first step in the conversation.
  3. The *reviewer* then attempts to drive the rest of the conversation, step-by-step, talking through the code, out loud:
    1. Walk through the code along a typical/main flow, step-by-step.
    2. Describe what's going on. State assumptions. Explain what the code is doing.
    3. Take notes of any items that you run into from the checklist, below. But don't talk about it during this phase.
    4. Each time the *coder* has to make a serious clarification to the *reviewer*, this is likely an identified concern. Spend some time, here, figuring out what the core issue is.
      • Is it just that the *reviewer* needed to understand a technology better? (e.g. wasn't familiar with an API) — not a problem.
      • Is the design inherently a little confusing, surprising or not self-consistent?
      • Is this chunk of code too big? Does it need to be broken down?
    5. If there are significant problems, stop the review, do what it takes to fix the code then restart this process.

Second Pass: The Checklist

  1. Together, the pair walks through the code with the "The Code Review Checklist" in hand.
  2. Talk through any items brought up by the checklist.

The Code Review Checklist

  • Design
    • Does the complexity of the solution match-up with the complexity of the problem? (evaluating complexity)
    • Are the responsibilities of each class clear? (evaluating cohesion)
  • Comments
    • Does the documentation that exists add context to the surrounding code? Does the code speak for itself? (remove extraneous commenting)
    • Is there project-specific context being assumed that if stated explicitly would improve a reader's understanding of the code? (add required commenting)
  • Project Conventions
    • Are naming and typographic conventions being adhered to? (automate as much as possible with Checkstyle or the like).

References

Unless otherwise stated, the content of this page is licensed under Creative Commons Attribution-ShareAlike 3.0 License