Why I have a code review process

When reviewing Pull Requests I have found it helpful to have a consistent approach. This helps me review the changes holistically, rather than nitpicking in the first few files and skimming through the later ones. The aim is to provide high-value feedback on the code so that the structure supports long-term development and it is easy to read and maintain. Having a defined process also creates the possibility I can improve it over time as I get better at reviewing code.

This is the process I use for reviewing a large, complicated, full-stack Pull Request.

Get context
Get an overview of the changes
Review the code one section at a time
1. Data model
2. Unit tests
3. Logic

Get context

Before even looking at the code, I try to find out what the intended change to the functionality is. The Pull Request title and description (if there is one) are a good starting point, but I'll also skim read any linked work items and documentation.

I don't need to understand every nuance of the work item at this stage, but I do want to know the general gist of what the changes are intended to achieve. This helps me keep the big picture in mind during the review.

If I'm struggling to understand the context, this is a good point at which to ask the code author to provide some context. This could be in the form of formal documentation (which will be useful beyond the code review) or a quick chat to explain the general idea behind the changes. This is particularly helpful for any urgent changes which have been made without a clear written definition of the requirements, or where the requirements don't explain the reason for the change.

Get an overview of the changes

My next step is to look at the code, but only at a high level. I want to get an overview of what I'm about to review before I dive in. An easy way to do this is to skim the list of file changes. The most important outcome here is to decide where to focus your attention while reviewing these changes. I'm asking myself questions like these:

Which areas of the codebase have a lot of changes?
Do any files have names that suggest they are particularly important to these changes?
What section of the changes will have the highest impact if it contains problems?
What areas are most likely to change in the future?
What sounds like it will be complicated and need careful thought?
Do any of the changed files have names/extensions which make them particularly important? For example, a change to a .yaml pipeline, a .csproj project, or Startup.cs are likely to impact the overall health of the project in a way which a change to an random .cs file is unlikely to do.

This should only take a few minutes, even for a large Pull Request, but it helps me distinguish between the important and the trivial. I don't want to waste my mental energy reviewing so many simple files that I have run out of focus when I get to a complicated file.

Review the code one section at a time

It's now time to start reviewing the actual code changes, but even here I find it helpful to have structure. Rather than starting at the top and working down, I jump around the changes. I generally default to reviewing files in this order:

The data model.
Unit tests.
Business logic.
Everything else.

However, this is not set in stone. While getting an overview of the changes I may have identified areas I want to focus on, so I will probably review them sooner rather than later.

The data model

I find it helpful to review the data model first for a few reasons.

Firstly, the data model is a critical part of the changes, so it's worth ensuring it makes sense. If the data model is wrong, everything else is likely to need to change anyway.

Secondly, reviewing the data model tends to be a quick win, as most data model changes are contained within a small number of files. This helps me get the review off to a good start.

Finally, the data model provides important insight into the rest of the changes. You are likely to come across important terminology, and you will be better equipped to understand how data is manipulated throughout the rest of the changes (and why).

Unit tests

If the changes include unit tests I tend to review these next, which I do in two stages. I start by reading just the names of the tests. Hopefully this spells out the intended functionality in a bit more detail, which is another thing that helps me to understand the rest of the changes in more depth. I then go through the tests a second time, reviewing the actual content of the tests.

One advantage of reading the tests before the rest of the changes is that you find out whether or not the tests make sense on their own. Most times someone is reading a unit test, it's because the test has failed and they need to investigate why. That person needs the tests to be written so clearly that they can understand them without reading all the application code first.

Business logic

For most changes, this will be the most complicated part of the changes. This is where there is a high risk of finding poorly structured code or confusing logic. However, by this stage in the process I generally have a good understanding of what's going on in the Pull Request, so I can review the code in an informed manner.

A lot of the code I review is for web applications built with SPA frameworks. These can contain business logic in both the server-side and client-side code. I tend to review the server-side code first, then the client-side code, but that's just personal preference. The main point is to properly focus my attention on these files as they are likely to be a core part of the overall change.

Everything else

At this stage, I'm just mopping up any files I haven't yet reviewed. I tend to move faster from this point, as I should already have reviewed the most important changes. I'm not too worried about missing things here, because I've already effectively decided these are the least important changes.

What I focus on while reading code

While following this review process, I try to focus on giving high-value feedback. This means I have my eye open for particular improvements which could be made.

Structure - do I understand how the classes interact? If not, this code will be difficult to maintain, so it's worth spending time wrestling with how the structure could be improved.
Legibility/readability - do I find it easy to understand the changes I'm reading? If not, that's a problem because I probably have more context than the next reader will have. If the code can become easier to read, then any other problems with it will also become easier to solve.
Naming - everything is easier to understand if the names are well chosen. Renaming something is typically a trivial change to make, but can add (or subtract) a huge amount of value.
Logic - does the logic fit with the context I have about the requirements? Anything confusing is worth paying particular attention to. I've found that this often includes things like boolean logic, calculations, and conversions (e.g. from one currency to another) - but I'm sure there are plenty of other examples.
Questions - I love to ask questions about anything I don't understand. Often I just need a short explanation and everything clicks into place, but it prompts me to wonder if the next reader will need help. Should there be an explanatory code comment to help them?

Conclusion

This is the process I try to follow as I review code. I'm not suggesting everyone should review code like this, and I daresay my process may evolve over the years as I get better at reviewing code. However, I have found it very helpful to have a roughly defined process, even if I vary the details a bit depending on the changes.

Tim talks tech