Here’s a Spotify album to accompany your reading: Tamino - Amir.

Disclaimer

And below is my framework for conducting code reviews within a large product-oriented software company. I do not intend this to be used as a universal guide by anyone, as situations, companies, and software differ. Most likely reviews in mission-critical software and a new young start-up look different. Consider this as another opportunity to look at what happens inside another person’s head.

Prelude

For the last four years, I’ve worked in a product company with a large code base and many teams working on multiple repositories and projects. The whole product is feature-rich, it is also quite versatile in the way features are implemented. On top of that, there’s a distributed system inside that adds complexity when features involving several services are added.

Our review process is mostly blocking: you must get several approvals before your changes reach the main branch and get deployed to the production environment. There are quite a lot of features being developed in parallel, as well as several fixes, refactorings, and tech-debt improvements.

As you can imagine, the number of reviews you conduct as a Senior+ engineer is quite high, and they seem pretty important.

Below are the steps I usually take when reviewing something important coming into the codebase and domain I’m familiar with.

Top-down approach

Figure out change request importance and priority

Not all code is born equal and it’s economically not feasible to apply the same standards to every line of the codebase. Some throw-away code changes to systems with a shallow level of importance may not require you to do an extensive review and can save a lot of time.

Understand the change in size

Generally, I take a look at the size of the changes first: just the number of changed files and the number of code lines to review. There’s universal advice to make code samples for the review tiny, like several lines of code, but I don’t believe this approach works well. Atomic changes, suitable for releasing and testing in legacy codebases hardly ever can be done that small.

Imagine you have a large scope of changes that, for some reason, can not be split into several reviews. Maybe it doesn’t make sense in terms of releasing and testing them separately, whatever.

For these situations, I prefer having an upfront call with several people, where the code author will describe the flow of the code, high-level principles, and the general train of thought.

It allows you to speed up the whole process of reading through lots of changes and provide some high-level suggestions quickly.

Understand what exactly you’re reviewing

Well, quite obvious. What can you do if you do not understand the reason the code you’re looking at actually exists? It’s generally a good idea to trace the changes back to their requirements. Normally it is achieved by things like a good change request description and a reference to the ticket (via some commit formatting e.g. following the structure like PROJ-1119: Some feature short description) and/or the problem description.

From there you should be able to answer the question why? the code change exists.

If it’s not clear to you - it may be a good idea to contact the author to do a short call or (which is better) provide the aforementioned details.

Understand high-level design of changes

For me, the most important first thing to do when starting to look at actual code lines or during the explanation call is to get a high-level idea of what moving parts are and how they solve the problem described above.

I move top-down mostly. If the change is for example some new handler - I examine the API and try to understand who calls who and when (it’s helpful if you have some prior architectural notes in linked documents or the description). Here I make a mental map of how data flows in the subsystem and what components interact with each other from the largest to smallest ones, and try to find incoherences with my mental map of how stuff already works inside the codebase or how it is intended to work.

Decide, whether the change should exist at all.

Sometimes changes do not make sense. At all. At many levels. In a well-designed development process these things should not happen (they should be caught on a product discovery phase, architecture design phase, or whatever pre-development pipeline you have). But they happen 🤡.

So here is an important phase for me to do research regarding anything I see that doesn’t make sense, and this may require you to go down the rabbit hole of tracing down architectural and product decisions and pushing back or finding compromises on how to make things ⭐the proper way⭐.

Ask questions

In case you see something you don’t understand: ask. This kind of situation also means that the code can be improved by: better naming, better documentation, better data flow design.

Analyze the behavior

This one is simple: the code should do what it’s intended to do and shouldn’t do what it’s not intended to do. Given all other steps went successfully (they do quite often!) and I understand the reasoning, data flow, and component interactions I try to hunt down failure scenarios.

Start “positive-only”

At this step you follow the code flow and see, whether it does what you expect to happen for positive cases.

Question the “positive-only” approach

Make sure the code under the review knows how hard things can be in the real world. Things go south quite often. Do not assume there’s a key in the dictionary passed through 5 levels of your amazing architecture; validate it upfront. Make sure errors are taken into account: they’re logged, propagated back to the callee and the whole flow does not break entirely. Or, break loudly and return that nice 500 status code screen, preventing data corruption. It depends. But what you want to be sure about reviewing someone’s code that’s there’s a notion of “I know things get screwed, that’s how I handle this.”.

Strive for simplicity

I don’t like complex code and attempt to reduce the complexity where possible and where it makes sense. The most important rule here for me is the codebase’s maintainability; that’s why I can easily sacrifice neat constructs and metaprogramming in favor of dead simple, structured, and easy-to-follow code, almost primitive.

At this stage, I try to find out places that can be prematurely over-engineered: complex class hierarchies solving one task but in a weird way, sophisticated refactorings of things that are not expected to change or expected to change in different ways, you name it.

Ensure enough tests are written

Testing in software engineering is a very debatable topic. I try to avoid enforcing the one and only way of proper testing. Still, I want to be sure that existing tests cover positive and negative scenarios and that there are at least some amount of tests that cover most of the subsystem (yes, I’m that one person who likes integration tests with the database not being mocked, throw a stone at me).

Roll-out safety

The question I tend to ask myself is, Is the code I review safe to roll out to production?.

It is a difficult thing to answer (everything breaks) but at least, I question the following:

Interaction issues

This topic (well, as almost every one of my statements) deserves way better coverage, which I will tackle in some new posts, but developing in microservices environments has a lot of specifics.

Here are several things I pay specific attention to during reviews:

  • Overall latency impact of the new functionality to the service.

  • Strategy of error processing and load mitigations from the caller and the callee sides.

  • Do not forget to have timeouts and deadlines for network calls.

Code style

I hardly ever look at the code style during the review phase. I’m a firm believer that these checks should be enforced by automatic tooling (yes, I do like black), and regular developers should not spend time discussing styling questions but instead focusing on design aspects and correctness.

General notes

There’s not so much here. The most important one is being honest and empathetic, remembering that there’s a living person behind the screen: a person with deadlines, maybe not that experienced in the specific codebase, maybe not that experienced in the technology, perhaps with several deadlines upon their shoulder.

I try to be understanding yet rigid about things I find harmful or useless, always staying professional in my communication. If you don’t like something, show it and communicate it politely. Provide some explanations for your opinions. If you see several rounds of reviews - speed up the process and move to the synchronous communication channel whenever possible. Praise neat solutions with words of appreciation. Stay human.

References and links