What to look for in a Code Review

This is part 1 of 6 posts on what to look for in a code review. See other posts from the series.

Let’s talk about code reviews. If you take only a few seconds to search for information about code reviews, you’ll see a lot of articles about why code reviews are a Good Thing (for example, this post by Jeff Atwood).

You also see a lot of documentation on how to use Code Review tools like our very own Upsource.

What you don’t see so much of, is a guide to things to look for when you’re reviewing someone else’s code.

Probably the reason there’s no definitive article on what to be looking for is: there are a lot of different things to consider. And, like any other set of requirements (functional or non-functional), individual organisations will have different priorities for each aspect.

Since this is a big topic to cover, the aim of this article is to outline just some of the things a reviewer could be looking out for when performing a code review. Deciding on the priority of each aspect and checking them consistently is a sufficiently complex subject to be an article in its own right.

What do you look for when you’re reviewing someone else’s code?

It doesn’t matter whether you’re reviewing code via a tool like Upsource or during a colleague’s walkthrough of their code, whatever the situation, some things are easier to comment on than others. Some examples:

  • Formatting: Where are the spaces and line breaks? Are they using tabs or spaces? How are the curly braces laid out?
  • Style: Are the variables/parameters declared as final? Are method variables defined close to the code where they’re used or at the start of the method?
  • Naming: Do the field/constant/variable/param/class names conform to standards? Are the names overly short?
  • Test coverage: Is there a test for this code?

These are all valid things to check – you want to minimise context switching between different areas of code and reduce cognitive load, so the more consistent your code looks, the better.

However, having humans looking for these is probably not the best use of time and resources in your organisation, as many of these checks can be automated. There are plenty of tools that can ensure that your code is consistently formatted, that standards around naming and the use of the final keyword are followed, and that common bugs caused by simple programming errors are found. For example, you can run
IntelliJ IDEA’s inspections from the command line, so you don’t have to rely on all team members having the same inspections running in their IDE.

What should you look for?

What sort of things are humans really good for? What can we spot in a code review that we can’t delegate to a tool?

It turns out there’s a surprisingly large number of things. This is certainly not an exhaustive list, nor will we go into any one of them in great detail here. Instead, this should be the start of a conversation in your organisation about which things you currently look for in a code review, and what, perhaps, you should be looking for.


  • How does the new code fit with the overall architecture?
  • Does the code follow SOLID principlesDomain Driven Design and/or other design paradigms the team favours?
  • What design patterns are used in the new code? Are these appropriate?
  • If the codebase has a mix of standards or design styles, does this new code follow the current practices? Is the code migrating in the correct direction, or does it follow the example of older code that is due to be phased out?
  • Is the code in the right place? For example, if the code is related to Orders, is it in the Order Service?
  • Could the new code have reused something in the existing code? Does the new code provide something we can reuse in the existing code? Does the new code introduce duplication? If so, should it be refactored to a more reusable pattern, or is this acceptable at this stage?
  • Is the code over-engineered? Does it build for reusability that isn’t required now? How does the team balance considerations of reusability with YAGNI?

Readability & Maintainability

  • Do the names (of fields, variables, parameters, methods and classes) actually reflect the thing they represent?
  • Can I understand what the code does by reading it?
  • Can I understand what the tests do?
  • Do the tests cover a good subset of cases? Do they cover happy paths and exceptional cases? Are there cases that haven’t been considered?
  • Are the exception error messages understandable?
  • Are confusing sections of code either documented, commented, or covered by understandable tests (according to team preference)?


  • Does the code actually do what it was supposed to do? If there are automated tests to ensure correctness of the code, do the tests really test the code meets the agreed requirements?
  • Does the code look like it contains subtle bugs, like using the wrong variable for a check, or accidentally using an and instead of an or?

Have you thought about…?

  • Are there potential security problems with the code?
  • Are there regulatory requirements that need to be met?
  • For areas that are not covered with automated performance tests, does the new code introduce avoidable performance issues, like unnecessary calls to a database or remote service?
  • Does the author need to create public documentation, or change existing help files?
  • Have user-facing messages been checked for correctness?
  • Are there obvious errors that will stop this working in production? Is the code going to accidentally point at the test database, or is there a hardcoded stub that should be swapped out for a real service?

Look out for follow up posts on this blog covering these topics in more detail.

What makes “good” code is a topic that every developer has an opinion on. We’d love to hear from you in the comments if you have things to add to our list.

Find more posts on “What to look for in a Code Review” here.

This entry was posted in Code review practices. Bookmark the permalink.

14 Responses to What to look for in a Code Review

  1. RPC says:

    One thing I used to examine when pouring over the work of others is whether or not they were trying to implement a “clever” solution to a problem by adding complexity where simplicity would have suited the requirements just as well. Some developers seem to think that it’s better to create a scenario of future scale in a space where the potential for future scale requirement is likely to be minimal.

    • Trisha Gee says:

      That’s a good point! Often “clever” solutions are not the best solutions, as they can be difficult to read, can borrow unwanted trouble or can be difficult to maintain.

  2. Kashyap Avadhani says:

    Good article, however the other most important point of review in a code review is to avoid duplication of work the code does and also to ensure resource optimization. Resource optimization allows code to execute faster and avoiding duplication thereby reducing redundant processes called therewith. Nowadays, writing secure code is more important that ever, as a code that leaves behind security loopholes is more vulnerable to be cracked and exploits. Note organizations that develop secure code have a protocol of test for code review using simulators that actually check for security loopholes in the code review.

    • Trisha Gee says:

      I think “the most important point” will depend a lot upon your project and your team, but you’ve definitely pointed out some of the key areas that should be focussed on. Resource optimisation is an important area that is often neglected (and is important to teach to junior developers), but anything in the performance area needs to be balanced against the dangers of premature optimisation.

      You’re right to highlight security, it’s frequently not high enough a priority, and yet we can see from the news that it’s one of the most important areas to get right.

    • Anonymous Coward says:

      :-) As always, it all depends. The dark side of staying DRY is strong coupling. IMO/IME it takes experience to strike a convenient balance (i.e. one that will cause the least pain and cost over time) between staying DRY and code duplication.

      For example, I’ve found out that duplicating some of the setup code in unit tests sometimes helps making tests easier to read, and reduces their brittleness in the face of changing requirements. Implementing ten different sorts, each one particular to a specific type and using a specific comparator, is waste, and should be avoided – sorting is well defined and generic, there’s no business requirement that can make the generic algorithm change.

      • Philipp Dolder says:

        @Anonymous Coward:

        I actually have slightly different measuring sticks for productive and test code:
        for example in test code I value readability and seeing all relevant information in the test higher then removing all duplication.

  3. Uncle Bob’s (Robert Martin’s) book, Clean Code, covers this well.

  4. Janne says:

    Thanks Trish for the nice article.

    Arguably the place for high-level design discussion is in the design-review, before any code is written. Things like variable naming, method and class size etc. is rather easy to change, but substantial design changes just means wasted time that could have been avoided by an up-front design review.

    • Trisha Gee says:

      Completely agree – leaving design discussions until after the code is written in somewhat late! Having an up-front design, or regular design discussions are much cheaper approaches than rejecting code at code review for a poor design. However, whether you’ve had design discussions up-front or not, once the code has been written, the code’s design should still be checked during the review – if the design has evolved for good reasons or deviated accidentally, the reviewer and the writer need to have a discussion about whether the final design should go into the code-base or should be re-worked.

      Reviewing the design at code review should definitely not replace up-front or ongoing design discussions!

  5. Anonymous Coward says:

    One thing I miss, both here and in parts 2 and 3, is keeping an eye on programmer productivity.

    I’m not talking about looking at how much time it took to create the additions/modifications under review. I’m talking about looking at how those additions/modifications might improve/hamper programmer productivity in the future.

    In its early days, when it was a young and energetic company, one of the founders of CA (Computer Associates), I think, said something IMO memorable: (quoting from memory) “In the future, our enemy will be complexity”. Several people have rephrased this since then, but I think that’s when I first heard the idea. (Ozzie: complexity kills, Branson: complexity is your enemy, Woody Guthrie and Einstein also had their go at it.) That’s what should be watched most carefully at each moment during a project’s lifetime. Code reviews lend themselves exquisitely to this.

    Accidental complexity is easy to introduce. More often than not, IME, it’s not recognized as such. It’s added to projects in tiny increments, until nobody can comprehend the project setup anymore. That’s how you get to a big ball of mud – http://www.laputan.org/mud/.

    Carefully watching for such tiny increments during code reviews and preventing them from surviving and propagating is IMO critical to a project’s long term success, even if simplicity isn’t considered an important factor in a project’s long-term success, in mainstream programmer culture. (I think that’s because we are all very good at forgetting past failures.)

    • Trisha Gee says:

      Absolutely. In fact, the Code Complete book also states complexity is the enemy. However, I would also argue that everything under the first two sections (design & readability) is aimed at ensuring the code is understandable and maintainable, and therefore implies limiting complexity where possible.

      But it’s a good point to explicitly state.

  6. Uwe Wardenbach says:

    Fighting complexity: a code review should always include an assessment of cohesion and coupling.
    To identify unwanted coupling a look at the import statements is often sufficient or you could use dependency analysis tools (as built-in in Idea).

    • Trisha Gee says:

      Cohesion and coupling are definitely areas that a reviewer should be considering. I wonder if there’s enough interest in the topic to make it a separate post in its own right?

  7. great information for improved programming.

Leave a Reply

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