bran.name

The Two-Part code review

Published

This is the article version of a video I've made: The Two-Part code review (12:27).

Code quality is a crucial aspect of software development. I define code quality as how easy it is to change and maintain the code over time. One of the biggest problems in our industry is that code slowly becomes harder to change over time, and I think it's our own fault as software engineers.

Most teams can easily manage a green-field project for the first half year, but after two years, it becomes unwieldy, and parts of the code have become very costly to refactor and make changes to. Let's explore how to prevent our code from rotting, from becoming a big ball of mud, and how to keep it maintainable and easy to change.

A good code review consists of two parts: Talking and Line-by-line. The first part is more important than the second part. In the first part, face-to-face communication is crucial, either two people behind a desk, or a video call with cameras on. In addition, the code author should rotate who their code reviewer is. As humans, we're biased towards finding the easiest path forward, so we tend to pick the same reviewer again and again, once we've figured out who will give us the least resistance on our merge request.

Part 1: Talking

The Talking Part is about explaining how the author understood the problem, their mental model of the solution, and then ask for a review on code quality topics (such as Abstractions/Interfaces, Separation of Concerns, and Coupling). The reviewer does the same thing from their perspective: ask the author to explain how they understand the problem, what solutions they came up with, and why they picked a specific solution over other potential solutions. The reviewer should be asking why the author solved the problem the way they did.

As the author, you might want to prepare this part so you have a clear story to tell and are able to answer questions. You can try to answer these questions and write them down in your note-taking system (by the way, do you use Notion or Obsidian yet?).

A lot of mistakes are made while understanding a problem or while picking the right solution, it's also quite common to forget acceptance criteria if the user story is somewhat big. The beauty of the Talking Part is that you can even do some of it before you start writing code. Validating how you understand the problem, how you intend to solve it and what the key tasks are doesn't need written code, that's asking for a review on your thinking.

Arguably, the most important topics for code quality are these things:

Worth mentioning as well, is that Test-driven Development will force you think about these topics while writing the test (so before writing the code), and thus yields higher quality code.

As a reviewer, this is where you really add value, this is the non-optional part where you need to pay attention. If you want to skip something, skip the line-by-line, not the talking! This interaction between humans is what causes high code quality on the long run, it prevents code rot.

As software engineers, our job is not only to "build the feature", but also to build a system that will last. This is how you do that. It's not optional. Yes, our short-term priority is to add value to your employer and customers/users. But the long-term part of that is making sure you add this value in such a way, so that you can keep adding value in the future at a reasonable cost.

Part 2: Line-by-line

The Line-by-line Part of the code review is the thing most people are already doing. As an author, it's important to do your due diligence: linting has no warnings/errors, the tests are green, the code has been committed into a separate branch, the branch name makes sense, the commit messages make sense, and the merge request or pull request has been created.

As a reviewer, read all the code line-by-line and judge it based on its correctness, security, and readability. Don't read it like a book cover-to-cover, read it in a logic way, I personally love the follow-the-data approach to reading code.

I like Maslow's pyramid of code review by Charles-Axel Dein (it's not perfect, but it is helpful). Use it to prioritise your comments; if there’s one or more correctness issues, don’t add comments on readability (yet), first the more important things.

It's ok to have multiple phases in a code review, and adhering to a very structural and methodical way of reviewing (like with this pyramid) will make sure you won't forget important things before mentioning less important things. That being said, you don't want to give 100 (or more!) comments on a merge request anyway, that'll hurt the author. That's not what being a nice human is about. If you feel like you have a 100 comments you could make, start pair programming together instead.

Lastly, use your version control system/platform like GitLab or GitHub to record your comments, don't send them over chat, email or any different way. This written record of history is useful for onboarding and other forms of knowledge transfer. I always advise people who are onboarding to read the last 5 merge requests from a team to get a feel for the patterns that are used. Being able to read that stuff is highly valuable.

TL;DR

Let me emphasize again that to get high quality code, we need to talk about code. Doing a code review is not reading code by yourself. It's absolutely crucial to hear the train of thought from the author, in person, to get the bigger picture. By following the workflow presented here, we can build systems that will last.