Giving High Leverage Code Reviews

Giving High Leverage Code Reviews

Giving High Leverage Code Reviews
Code reviews have several purposes, the most common of which is checking for bugs before code is shipped. But code reviews can be leveraged for so much more. For example, at Gusto, the primary purpose of code reviews is to spread knowledge and ensure quality. At their simplest, code reviews are a quick scan of someone else’s changes with an approval to merge. At their most in-depth, they can involve pairing, reviewing code line by line, and multiple feedback cycles.

Hopefully, you’ve been given a small and digestible pull request with a clear description to review. But with any pull request, getting started and knowing what to look for can feel overwhelming and unclear. In this post, I’ll give you an overview of how to approach a pull request to provide meaningful code reviews to your team.

1. Break It Down

If you’ve been assigned to review a pull request that is too large (think hundreds or thousands of line changes), request that it be broken down into multiple PRs that are more manageable in size.

Breaking down a feature is a skill in its own right, so offer to help your teammates with this. If the pull request can’t be broken down further but is still too large, consider doing an “in-person” code review where you walk through the pull request together. Think of this as a pair programming session, encouraging it to be a two-way street where the author explains their changes and the reviewer asks questions as they arise. As you walk through the pull request, leave in-line comments to reflect what was discussed in the review, so that context is still recorded for the future.

2. Use a Checklist

It’s hard to remember everything to check in a pull request, and constantly switching contexts might cause gaps in your review, too. For example, if you’re focusing on the readability of a change, you might miss a risk to security or performance.

When I’m reviewing a pull request, I often do multiple “passes” where I focus on one attribute at a time. I start at the beginning and review the pull request with a single attribute in mind before moving on to the next. When I’ve worked through the checklist, I submit the review.

This checklist moves from general to specific checks because it’s important to focus on the high-level attributes first. It doesn’t make sense to offer a variable name suggestion if you’re also suggesting that an entire class or function be refactored.

Don’t hesitate to review multiple times and zoom in as you go.

Here are the items and prompts in my checklist. Feel free to use this as a reference, and consider drafting a custom checklist by yourself or with your team.

Meets Requirements

If your team’s tickets contain acceptance criteria, it should be easy to determine if the pull request meets the expected requirements. If not, here are some prompts to help you find the answer to this question.

  • Does this pull request satisfy the expected requirements?
  • Does the proposed UI match given mockups and designs?
  • Does the proposed change fit within the scope of the ticket?

Architecture & Design

If your team creates detailed design documents before building, this should be an easy check. However, sometimes features become more complex than anticipated, and the design gets mixed with implementation. In these cases, it’s important to take a step back and ensure that you agree with the architecture.

  • How was this feature implemented? Can I easily discern the design from the code?
  • Does it follow an agreed upon design? If not, does the structure of the implementation make sense?
  • Is it simple or over-engineered?
  • Is the code open to extension in the future? Is it specific or generic? Have abstractions been made too early?
  • If a new engineer joined your team tomorrow, could they understand this implementation?

Interactions & Side Effects

At Gusto, we still work in a monolithic application, so seemingly innocent changes can have unintended side effects on core functionality. If your system is less complex or if you’re working on a greenfield project, this may be less important.

  • Is it possible that these changes have unintended effects in other parts of the system?
  • If an existing function was changed, were all usages updated?
  • If external dependencies were added, have they been evaluated?

Test Coverage

At Gusto we don’t have a QA team, so this is another critical check. Our test suite is exhaustive and must remain that way. This check shouldn’t be green just because a test file exists. Review the tests. There is nothing worse than false positive tests or flaky tests.

  • Are there tests? Should there be tests?
  • Do the added tests have the potential to flake in the future?
  • Is the correct functionality being tested? Are the tests structured appropriately?
  • Are edge cases accounted for and being tested? Are there edge cases that aren’t being handled?

Performance

The importance of this section depends on the type of work you do. At Gusto we don’t handle billions of requests per second, but we do need to be mindful of performance when loading dozens of resources and relationships. Work with your team to determine how often you need to consider performance.

  • Has optimization been properly balanced with readability?
  • Is there a potential performance bottleneck?
  • What queries are being made? Are they retrieving more data than is being used?
  • Does the proposed change have the potential to affect performance in other areas of the system?

Readability & Style

Payroll and benefits are highly complex domains, so our code needs to be simple and easily understood. As a result, I place an emphasis on this area, regardless of the size of the pull request.

  • Can you understand the code without explanations from the author?
  • Could you debug this code without the author’s help? Could you extend the code in the future without the author’s help?
  • Is it clear what the purpose of a variable is from the name? Is it clear what the intent of a function is from the name? Is it clear what the expected type of an object should be?

3. Focus On Delivering Value

The goal of any pull request is to deliver value, whether it be to our external customers or internal users. Reviewers are here to help improve the product, not to bike shed on small details.

As you review a pull request, point out which comments do and do not block merging. If you’re making a refactoring suggestion, approve the pull request, but ask that the author incorporate the change if they have time. If you find an error, make it clear that it’s not a suggestion and that it needs to be resolved before the code is shipped. I typically do this by prefixing comments with “blocker”, “not a blocker”, “nit”, and “suggestion”.

Overall, don’t aim for perfection. Perfect code doesn’t exist. If the code is error-free and better than it was before, approve the pull request.

Giving High Leverage Code Reviews

4. Be Helpful

The goal of a code review is not to prevent bugs – that’s what tests are for. Code reviews are an opportunity for learning just as much as they are an opportunity to ensure quality in your codebase. In the first few years of my career, I learned a ton from being on both sides of thorough code reviews.

I’ve found that the best way to teach in code reviews is to simply leave longer and more informative comments. Add links to documentation and articles that help explain a concept or an alternative approach. Use GitHub’s suggestion feature to offer an alternative solution in a comment that the author can commit directly.

Don’t just point out flaws and problems in code; explain why a certain change is problematic and how it can be improved.

If there’s a more performant way to write a query, leave a comment demonstrating how the change would be made with a link to the documentation. Comments such as “This is slow and inefficient.” aren’t improving the code or teaching the author. As an author, I frequently ask “why?” if a reviewer suggests a change so that I understand why their proposed solution is preferable. If I don’t ask a question, I won’t learn and improve.

If you stumble on a block of code that seems off, but you aren’t sure what should be changed, ask questions! This will prompt the author to think more, explain their approach, or clear up the code by rewriting it. A comment like “I’m not sure what’s happening in this block. It looks like you’re iterating over a list to perform a calculation, but I think the result is lost. Can you explain what’s happening?” will lead to a useful discussion.

5. Be Human

At the core of a code review, you’re providing feedback to your peers, which might be hard. But receiving feedback is harder. Everyone on your team is trying to do their best work, so take care in delivering your message. For example, if you’re pointing out an error or asking a question, make it a team effort, not their fault. This might look like: “Can we remove some of the duplication in this file?” instead of “You missed an edge case”.

Don’t forget to leave positive feedback in code reviews, too. If you appreciate the documentation in the pull request or learned something from the code, leave a comment and let the author know! Acknowledge great work and thank the author for their time and attention to detail.

Conclusion

The next time you’re assigned a code review that seems daunting, hopefully you’ll feel confident in knowing how to approach it. Break it down and use a checklist to remember what you want to review. Above all, focus on delivering value to your customers and helping your team improve.

Remember that code reviews are about feedback, and delivering feedback well is a skill. This post can teach you how to leverage a code review for more than finding bugs, but it can’t teach you how to perfect giving feedback. You have to practice flexing that muscle yourself.


Thank you to my former colleagues and Quan Nguyen for several of the ideas in this post, and to Lindsey Whitley and Lucy Fox for editing.


Source: Gusto