How to Review Code

I’m a software engineer. I review a lot of code almost every day.

As a gatekeeper of the codebase, I treat this responsibility seriously. I strive to do a thorough job so that we don’t have to pay for our mistakes down the road.

The problem is: I don’t have 8 days a week, or 25 hours a day.

So I decided to get better at code reviewing: I want to provide high quality, detailed comments, and I want to do it fast.

Over the years, I’ve evolved my approach to suit my goal. Here’s my current process for a typical code review:

Start with the change description

In the early days, I’d jump into the code right away and try to judge it on its own merit. Now, I read the PR (code change request) description carefully to understand the goal of the change (why do we need it at all?) and interesting design decisions (why do we do it this way?). If the answers aren’t clear without looking at the actual code, I ask the author to revise their description.

This is time well spent as:

1. The change description is the most important piece of metadata that will be read repeatedly by future maintainers when they debug and try to learn the history.

2. Knowing the author’s goal helps the reviewer propose better solutions, or, in some cases, question whether this is the right thing to do at all.

Focus on the interface first

Once I understand what the change is trying to do, I read the code. However, I don’t go through the files alphabetically. I always start with the programming interface (e.g. for C++, it’s the headers).

There’s no point reviewing the implementation if the interface is poorly constructed. It will just be a bunch of busy work. Premature focus on details will slow everyone down.

There’s another benefit of interface-first: it ensures that the interface is judged fairly.

If I read the implementation first, I will already know what each function does when it comes to reviewing the function interfaces. This hampers my ability to spot inaccurate naming, insufficient documentation, and unnatural contracts, for example. I may think that a confusing API (application programming interface) is clear, with knowledge that I don’t realize I have gained.

What are the top 3 important things in programming?

Abstraction, abstraction, and abstraction.

If an API provides good abstraction, code using it will be easy to understand and reason about. Things will feel natural. Mistakes will stand out. Surprises will be rare.

A good function should have a clear contract (What are the inputs and the precondition? What are the outputs and the postcondition? What are the side effects?) that’s intuitive. Someone who has never seen its documentation should be able to correctly guess what it does.

Often in the first few rounds of a code review, I look at the API only. I don’t review the actual implementation until the author has addressed my concerns with the API. This means the author can get my feedback quickly and we will avoid wasting time on the implementation, which may be rewritten due to the interface revision.

When reviewing an API, I make sure that each piece has a clear contract and they fit together well. The exit criteria are that I must feel that I can confidently program against the API without looking at its implementation.

Review the implementation (including tests) last

After I’m happy with the interface, I move on to the implementation. At this point, most of the design points should have already settled, so we can focus on the correctness, readability, and efficiency of the code. Although, sometimes we may discover things that require us to revisit an earlier design choice.

I check that all important changes are covered by tests, and hold test code to the same bar as production code.

There’s some difference in the coding style though: the #1 requirement for test code is that it must be obviously correct, as typically there are no tests for tests themselves. Therefore, in tests we want to avoid excessive abstraction and instead express things in a direct way, sometimes at the cost of repetition.

Other than that, I insist that test code is well thought out, correct, efficient, readable, and easy to maintain.

Let me know what your favorite code review approach is. Thanks!