How to Take the Pain Out of Code Reviews

Code reviews. There may be nothing in software engineering that’s simultaneously more painful and more helpful. They’re helpful because they provide specific feedback looking at the code as code. They go beyond “Does it work?” and become learning opportunities for both the author and reviewer. But they’re painful because … well, have you been in those meetings? It’s always painstaking work to read so closely and look for small mistakes. Now do it in a three-hour meeting. It’s no wonder everyone emerges exhausted and snippy.

Why Do Code Reviews?

CSS Code SnippetIf code reviews are so painful, why do we do them? Ultimately, there are only two reasons. First, they improve overall code quality, and second, they improve developers’ skills.

Code reviews improve overall quality because they provide a venue to evaluate code and only code. They provide a defined opportunity to consider code structure, layout, style conformance, problem-solving methods and other aspects of the work. (You can see more on my code review checklist, although details will vary based on your specific environment and team.)

Also, code reviews help developers improve their skills by providing a a feedback loop. They’re a way to look at an individual developer’s work, acknowledge their great ideas, and identify specific areas for improvement.

Most software products are the result of a team working together, and the code review is one of the few times the group has a chance to look at individual work out of the context of the overall product. System tests evaluate a product as a whole: They look at the work of one or more teams. Unit tests evaluate a piece of the product: They look at the work of multiple people over time. Code reviews look specifically at the work of one person.

Because they’re so focused on the individual, code reviews can be scary. They can turn into “pick fests” where reviewers gang up on the developer. Taken to extremes, this creates an extremely toxic environment. To keep the code review effective, you have to turn it into a positive experience. To do that, they have to be egalitarian — everyone’s code must be subject to review. In addition, make sure code reviews are about the code.

Types of Code Reviews

Code reviews come in several flavors: ad hoc, architect-driven, and peer. Any of these can be done at any point in the software development process — before the code is checked in, before the build is done, after the initial testing process or even after release.

Ad hoc code reviews are tactical reviews conducted when the team — or someone on the team — feels they’re necessary. A team might conduct a code review when changing a core algorithm, or for a new developer just coming up to speed. They’re driven by circumstances and not by a defined pattern or trigger. For a team that doesn’t do code reviews regularly, ad hoc reviews are often an easy way to start.

The major risk of ad hoc reviews is that they are prone to become a punishment mechanism. It’s very easy to use an ad hoc review to gang up on a developer. Imagine a scenario in which one person’s code “magically” comes up for review far more than others’. That’s a recipe for creating a seriously unhappy developer.

Architect-driven reviews are the traditional approach. In this model, an architect or senior developer reviews the code. The emphasis is on oversight and senior technical resources educating junior technologists. In some cases, the reviewer is the team lead. In others, the reviewer (or review committee) is a separate team, often an oversight or “office of the CTO” committee.

These are the code reviews everyone dreads. They tend to go through large swaths of code in meetings that last hours, then hand down findings without discussion. I avoid this model as much as possible for general code reviews. However, it can be useful when conducting a highly targeted review that looks only at specific aspects of the code. For example, an architecture-only review.

Peer code reviews — where team members review each other’s code — are becoming increasingly common. They cover all aspects of the code and emphasize discussion over review. The idea is that explaining your code to another team member — even a more junior one — is a learning experience for both of you and can help each person to identify flaws in the code. Pair programming is a form of real-time review.

The downside of peer code reviews is that they can end up being a case of the blind leading the blind. To make them work, at least one participant has to be senior enough to have good instincts.

Facilitating Code Reviews

Conducting an effective code review takes some set-up. It doesn’t have to be terribly involved, but a little effort can make everything a lot easier. In your review, consider the following:

  • Source code branching.
  • Code commenting systems.
  • Atomic units of review.
  • Static code tools.
  • Work tracking system.

Next week I’ll look at source code management tools, and also how the branching or commit mechanisms you use affect how easy — or difficult — it is to isolate code for review.

Comments

  1. BY RobS says:

    I just decided to coin a new term: “Blind Code Reviews”
    The idea is that anything that’s checked in is pulled out for review from the whole team. However, before presented, all programmer-identified pieces are removed and the code is handles like an accounting team handles an audit.

    For example, look at naming conventions, code assembly, possible areas for speed-efficiency (e.g. loops), how information is passed from one component to another (method-to-method or module-to-module) etc.

    After the review is completed, notes are shared so that all can see (1) if the code needs to be updated (2) if the standards need to be updated

    The idea of the blind code review is that you can’t pick on someone if you don’t know who is the “culprit” (of course, the culprit also does not get credit for great new ideas) so the process is likely to be more objective.

    Anyway, this was a spur-of-the-moment thought so I’d appreciate any feedback on the idea.

  2. BY Me says:

    If it is to be reviewed in such a manner, do it before the code is “checked back in”. I don’t believe the review needs to be blind; there is no room for ego when the goal is to produce the best possible product. Also, inability to identify the author of great ideas eliminates the potential of that person being a mentor to others.

    • BY RobS says:

      Valid points…but clearly you never worked for a government agency :) (egos everywhere and no room for improvement in any direct way)

  3. BY pacde says:

    Really code review goes further than improve the quality of the code, it also provides assurance that someone isnt injecting malicious code (either inadvertantly or on purpose). Code review may be a very good control to keep your code clean and secure.

  4. BY adamchenwei says:

    Its interesting how the code review could have such a great influence on the working environment. Great article!

  5. BY BrianS says:

    In 25 years of development, I went through a code review exactly once.
    Most of the time the teams I’ve worked with are just plain too busy to bother.
    I find it a bit disingenuous and ironic for a commenter going by “me” to disagree with blind reviews… :)

    • BY Me says:

      Are you “accusing” me of knowing more about the subject than I am letting on? Regardless, I am not hiding. My website(s) and email address are available. Feel free to send a message to “me” even if it’s to tell “me” to get stuffed.

      P.S. I’ve never been subjected to a code review; everyone else was too busy, but I know for a fact it could always be improved someday, even by “me”, given the time, but someday never came.

  6. BY Gary says:

    Following a Checklist for the Code Development and marking it during review will make a Code Review task very simple for everyone involved.

Post a Comment

Your email address will not be published. Required fields are marked *

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>