A Guide to Thorough Code Review

10 steps for ensuring that a team is always delivering high-quality software to customers while learning from each other.

When working on a team of software engineers, code review is one of the most important processes for ensuring quality as well as a vehicle for learning and personal growth. Most engineers get started working alone and are not accustomed to reviewing a large chunk of someone else’s code, nor reviewing their own work, for that matter. Like anything, it’s a skill that takes practice and learning by trial and error. However, not every team has the luxury of a consequence-free environment in which to make mistakes that may otherwise put poor-quality software in hands of paying customers. To start off on the right foot and avoid costly mistakes there are some basic guidelines that team members can follow to ensure code review is thorough and upholds quality standards.

But First, A Bare Minimum

As a reviewer, you should add at least one comment to the code your are reviewing. This proved to be a highly-contested measure when I introduced it on the teams I was managing, but I stand by it. A comment does not necessarily have to be a request for change, but as a forcing function for thoroughness, a reviewer really should have something to say about what he or she is reviewing. Too often I have seen pull requests on GitHub that contained no comments or discussion at all, just a “+1” from a single reviewer. While it’s certainly possible that the code in question was perfect and needed no changes, clarification or improvement whatsoever, that is highly unlikely.

And When Commenting, Use Code Snippets

Before discussing any specific kind of issue, I want to highlight one of the most important techniques for basic communication between developers when reviewing each other’s work. Rather than trying to explain complicated issues using written English (which, by the way, may not be everyone’s first language), it is always much more efficient to write code snippets that demonstrate the idea that you wish to express. Developers are usually more fluent in code and may not remember all of the highly-technical names and phrases used to describe the features of a programming language. In Swift, for example, it is much easier to write “??” than to say “the nil coalescing operator”. When preparing a code snippet, reviewers are also forced to test their own idea before suggesting it. I have found many times that some improvement I wanted to recommended wasn’t actually possible when I tried to write it out in code, which helped me to understand why the author selected his or her particular strategy in first place. And finally, when a suggestion communicated by a code snippet is accepted, the author can copy and paste that snippet with few or even no further modifications to directly include it into their work. That’s efficiency and it makes a big difference.

10 Steps for Thoroughness

When reviewing someone’s work, such as in a pull request, reviewers should pay careful attention to the following issues and leave comments or ask questions wherever and whenever it is uncertain that the code in question meets quality standards:

1. Meeting Product Requirements
   The most beautifully-written and meticulously-tested code could easily miss the mark on the feature’s basic requirements. Some engineers prefer to defer this responsibility to other team members such as quality assurance testers or product owners, but some amount of time should be spent making sure that right work has been done in the first place. It’s also a waste of a reviewer’s time if the work must later be rewritten anyway because of some small communication error that resulted in the incorrect execution of a task. Spend enough time up front to thoroughly read and internalize whatever product requirements are available and critically evaluate how well features developed meet them.
2. Readability
   Identify areas where code is anything less that effortless to read. Some engineers pride themselves on being able to fit as much complex logic as possible into a single line of code, but this is not always the best choice when fellow colleagues are expected one day to read, understand, modify and debug it. Cyclomatic complexity and excessive indentation from functions, switches, if-else statements and other flow control mechanisms should be avoided. High-level logic should exist separate from low-level logic, and a function, class or other object should ideally have a single responsibility. If there is a more concise, less complex or more readable way to write a line of code, don’t hesitate to recommend it.
3. Code Style Consistency
   I hesitate to put this so high on the list because of the risk that engineers will spend all of their code review effort nitpicking at the style choices of the author. These superficial details mean nothing to the end user and can be a source of negativity and toxic arguments. Most of the time it’s best to remember that everyone has their preferences and rarely are yours objectively superior to anyone else’s. But the code submitted should at least adhere to best practices defined by the vendor of the language, framework or development environment as well as those adopted or created by your own team for specific reasons. In the end, there should be enough consistency that it’s hard to determine the author just by looking at the code.
4. Logical Errors and Unsafe Code
   Identify any potential bugs by carefully reading the logical flow of the code and try to foresee any undefined or problematic behaviors that may arise when unexpected values are provided or when other unexpected conditions or edge cases arise. Look for situations where crashes may occur and ensure that proper error handling is always used. Do not allow any unsafe code to be deployed live because it can and will crash.
5. Clear and Logical Naming
   Code should be self-documenting with accurate and informative naming conventions, liberal use of intermediate variables and small, sensible interfaces that don’t hide subtleties or mislead consumers at call sites. APIs should be designed to be easily understood by those who are unfamiliar with them. Complex APIs should have sensible defaults for ease of use but the option for advanced users to get fancy with options and customizations.
6. High Performance
   Ensure that selected algorithms and data structures are performant in accomplishing the task at hand as well as larger data sets that may not be expected. Offer suggestions for alternatives and include links to relevant references so that team members who might not be unfamiliar with a particular technique can quickly read up and make use of your suggestions. Don’t hesitate to discuss performance in terms of Big O Notation.
7. Documentation and Comments
   The best documentation is no documentation, i.e. self-documentation. But when that’s not possible, ensure that comments explain concepts that are non-obvious or have other implications. If a hack or workaround is being used, make sure to add a comment about it so that it can be more easily identified and cleaned up later. If you have to ask a question about how something works it usually means that the code is missing a comment to explain it.
8. Test Coverage
   Ensure that highly important pieces of logic are covered by unit tests. If the code isn’t easily testable, perhaps it should be refactored with testability in mind. Identify logic that by its nature may be easy to break. Make sure that this code has appropriate test coverage with detailed assertion messages so that if another developer causes a test failure, the issue can be quickly and safely resolved.
9. Proper Use of APIs and Frameworks
   Apply your knowledge and experience the programming language or frameworks to ensure that other team members are using these tools correctly. Recommend better ways to accomplish similar tasks that may involve less complexity or less risk of error or unexpected behavior. Identify code that is too imperative and recommend more declarative solutions. Make sure the right tool has been selected for the job at hand and avoid the proliferation of multiple solutions to the same problem.
10. Scalability and Flexibility
    Consider how the application will perform as well as how we as developers will perform when managing some this code over time. Identify interfaces that are likely to grow as more variability is added and ensure that a robust enough solution is designed from the beginning. Consider what kinds of changes or additions might be requested in the future and how adaptable the current architecture is to such changes. Also consider how easy or difficult it would be to disable or cleanly remove a feature in the future.

Further Reading

For more information about great code review techniques and some scientific data to back it up, check out 11 proven practices for more effective, efficient peer code review. Here is a short summary of these 11 practices provided at the end of the article:

1. Review fewer than 200–400 lines of code at a time.
2. Aim for an inspection rate of fewer than 300–500 LOC per hour.
3. Take enough time for a proper, slow review, but not more than 60–90 minutes.
4. Be sure that authors annotate source code before the review begins.
5. Establish quantifiable goals for code review and capture metrics so you can improve your processes.
6. Use checklists, because they substantially improve results for both authors and reviewers.
7. Verify that the defects are actually fixed.
8. Foster a good code review culture in which finding defects is viewed positively.
9. Beware of the Big Brother effect.
10. Review at least part of the code, even if you can’t do all of it, to benefit from The Ego Effect.
11. Adopt lightweight, tool-assisted code reviews.