Strategies for Reviewing Code

Code reviewing someone's PR can be overwhelming. It's a skill that needs to be taught and developed and it's not something that comes naturally in a short amount of time. There are a couple of strategies that I use to tackle large and complicated PRs.

Understand the scope of the PR

Understanding scope lets you perform focused code reviews that aren't a waste of the author's and reviewer's time. If the author hasn't provided context, then there's no way for you to accurately decipher the intent of the change, especially in large PRs. You don't know what is intentionally left out and what should be implemented. Your code review is now relegated to cosmetic and minor implementation suggestions that aren't as helpful to the author.

You can also spend some time to survey the PR to understand scope and start building a mental model of the code. Make a couple of quick passes to parse out some of the high-level concepts at play. You want to understand what has been introduced, what has changed, and how they relate to one another. At this point, you will certainly have holes in your understanding but that's okay. A mental model of the code in your head should start to form vague, incomplete shapes that should at least make the next passes easier.

Leave notes to yourself in the code review

As you are surveying the PR in the first couple of quick passes, you should leave notes to your self in the form of unpublished comments if your code review tool allows this. Why was this renamed? What is this code doing? Why was this deleted? Once you've been able to absorb the whole PR, revisit these comments to see if they are self-evident. If not, submit the comments and ask the author to clarify. These comments offload some of your brain's memory onto the code review tool so that your brain doesn't fry from keeping track of all the sticking points of the PR.

Start at the top of the call hierarchy.

When we read code in our editor, we don't read the files in the order they appear in your editor's file tree. Instead, you would open a file that has a function call that you are interested in, and you would follow it's child or parent functions to get more context. In other words, we navigate the call hierarchy. There's no reason why you wouldn't do the same in code reviews.

When reviewing a PR, look for the function that is at the top of the call hierarchy and then start carefully reviewing each level of the hierarchy. Since you are starting top-down, you will have context around why a certain function is needed since it's parent context is still fresh in your memory.

Take advantage of your editor's text search to easily navigate the call hierarchy by function/class/type names. You don't want to be scrolling up and down since in the time it takes you to scroll you will lose context (your brain's memory is pretty volatile).

Without understanding the parent call, it will become more difficult to understand why the child call is needed and it will feel like you are reviewing seemingly disparate pieces of code. You need to see how all the code in the code review fits together which is why you start top-down, instead of bottom-up or middle-out. When you review a project document, you wouldn't start from the details and work your way up to the higher-level parts of the documents.

Checkout the code and read it from your text editor or IDE

If the PR touches a lot of files, navigating the call hierarchy can be tedious. You can make this easier by simply cloning the code and using your editor's code navigation tools to jump between definitions and usages. Also, viewing code in a text editor allows you to evaluate if the code makes sense in its surrounding context. Often, code review tools will hide the surrounding context by default and will make it tedious to unravel more lines around the change.

Context matters a whole lot. You need to make sure the code does not break the current mental model of the code or unintentionally change the mental model. Remember you are reviewing the code, not the diff.

Make sure you have a clear mental model of the code

I often hear early-career developers say they're not sure what to look out for in code reviews. A misconception that people hold is that code reviews are primarily for combing for bugs or suggesting improvements to a piece of code. While this may be true to a certain extent, the primary purpose of code review should be to develop shared understanding.

Code reviews help teams develop a shared language of their codebase. If everyone has a different mental model of the codebase, or no mental model, then the code will be pushed many directions making the code more difficult to understand. You want to use code reviews as an opportunity to align on understanding of the code.

In other engineering disciplines, the mental model manifests itself in the real world, such as the floorplans of a building, or the layout of a PCB. Developing shared mental understanding is not as necessary since the constraints are visual and self-evident. But with software, everything is made up constructs designed to help us communicate with one another in lieu of observable things. The building floorplan could be a set of base classes that we've defined. The layout of the PCB could be the MVC pattern we are trying to move towards in the codebase. "MVC" and "base class" are shared languages that allow us to talk about the code we are collaborating on.

So before signing off on a PR, make sure you have clear mental model of the code that both you and the author can agree on.