Building software isn’t easy. Add the opinions and needs of multiple people into the mix and it can feel almost impossible at times. Learning to collaborate with multiple people on the same software is an essential skill for every software engineer nowadays, as almost no software gets built and maintained by a single person.

Collaborating on software can happen on many levels, from each person building a piece of a bigger system all the way to pair (or even mob) programming. Choosing the optimal level depends a lot on the complexity of the problem you’re solving, but as a general rule of thumb, I would advise erring on the side of collaborating more closely when possible.

Nonetheless, there are very few teams that actively pair or mob program the entire time. Pairing and mobbing can be significantly more exhausting, especially when done remotely. On top of that, async teams might not even be working at the same time. Yet, we want to make sure that everyone in the team is aware of the changes that are being made. For most teams, this happens through a pull request (or merge request, depending on the tools you are using).

Modern software collaboration tools like GitHub and GitLab facilitate pull request reviews and approvals before merging the changes to the main branch. Depending on the size of your team, you might spend quite some time reviewing other people’s code, so let’s find out how we can optimize the process for everyone involved.

Understanding why we do code reviews

As with any process, it’s important to understand the problem it solves and the value it is supposed to bring. If your process isn’t solving a problem, it’s called overhead.

A good heuristic to figuring out what problem a process is solving is imagining its absence and noting down the consequences. In the case of code reviews, let’s imagine a project where everyone pushes to the main branch, which is shipped to production with a CI/CD pipeline.

People on the team will only be aware of the code they’ve contributed. This will lower the collective learning rate in the system and might even result in duplicated or even conflicting implementations of certain behaviour. 

Getting design feedback about the code you contribute after it’s gone to production will make it more unlikely that feedback gets implemented. More often than not, a system built by several people without a collaborative design vision being checked and refined continuously will end up being a big inconsistent mess. 

Obvious bugs that aren’t covered with automated tests will end up in production more often. That extra pair of eyes will spot things you as an author may have missed for the same reason you are often blind to your own typos while writing.

Turning all of this around again, I feel like we can summarize the value of code review as:

  • Fewer bugs in production
  • Higher code quality
  • More collective learning opportunities

Those look like good reasons to spend effort in our development cycle. Let’s see how we can best approach a code review.

Reviewing code 

The first thing I try to do when I review a pull request is to gauge the scope of the change. Is this fixing a bug? Changing the behaviour of the system? Change the structure of existing code? Removing dead code? A combination of the above? Understanding what change is being made and why it is made is crucial because it will guide the rest of the review. Here are several types of pull requests and how I usually approach them as a reviewer.

Removing dead code

This is one of the easiest to review, though I often see people simply approving these pull requests. You can bring value by checking out the code locally and doing some targeted usage searches. Figure out if the parts that were removed were really unused. Maybe you can find more parts that became obsolete due to the removed code. I usually make it a personal challenge to either prove the author wrong or give them more code to remove. As much as I love thorough colleagues, I’m always a bit sad when I can’t find anything else to throw away. We will talk about added or updated dependencies later, but removing a dependency is very similar to removing other dead code and should be reviewed as such.

Fixing a bug

First things first: make sure you understand the bug. Once that is done, I usually check if the author added an automated test to prove the bug. If there’s no test, I try to come up with a simple test scenario to suggest in the comments. Only then do I look at the fix. I wonder if the fix would solve my suggested scenario. Maybe I missed something and need to revise my scenario, maybe the author missed something in the fix by not having a test for it yet. If the fix looks good, I try to explore the potential side-effects of the fix. Note that all of this only touches the behaviour of the system. Last but not least, I look at the changed code (and its surrounding lines) to see if anything is unclear or can easily be improved with a simple refactor. Maybe we can clarify something by renaming a variable or function call. Fixing the bug might have altered the behaviour to the point where something needs to be renamed to better encapsulate that behaviour.

Adding a feature

Reviewing a new feature is similar to reviewing a pull request that solves a bug, although the scope is slightly bigger. Understanding what problem the feature is trying to solve is essential in validating the proposed solution. I strongly suggest adding new features in pairs or mobs as much as possible. Shortening the feedback loop here will drastically reduce the delivery time and raise the quality, exactly what we need when building new features. If I have to review a feature anyway, I usually do a double pass: first I do a behavioural review, then I do a structural review (we’ll come back to behaviour vs. structure later).

Changing a feature

Reviewing a behavioural change in an existing feature is very similar to that of a bugfix since bug fixes are often changing behaviour from something that didn’t work to something that did. They also share a similar test workflow if you decide to include manual testing in your review flow (hint: you should). You start by confirming the behaviour before the change and you then confirm the behaviour after the change. Additionally, regression tests can help ensure that nothing else was changed along the way.

Refactoring existing code

Since refactors shouldn’t introduce any behavioural changes, we can fully focus on validating the structural improvements that are proposed. Again, understanding why the decision to refactor was made will help you answer some of the questions you should ask when reviewing these kinds of pull requests. Depending on what kind of refactoring happened, these questions might vary significantly. Here are some helpful ones I often use:

  •    At a glance, is it more clear what this piece of code is responsible for? (clarity)
  •    Are classes in a more logical place than before? (cohesion)
  •    How hard is it going to be to extract or (re)move something now? (coupling)
  •    Was this covered by tests before? Is it covered now? Are the tests easier to write than before? (testability)
  •    Is this introducing or removing abstractions? Why? (complexity/yagni)

Understanding which parts of a refactor were done automatically using reliable tools can help reduce your cognitive load while reviewing. If the refactoring was done using small incremental commits, I strongly suggest reviewing these pull requests commit-by-commit to follow the author’s train of thought.

Adding dependencies

Reviewing a new dependency is a bit different from other reviews. Once we understand why we need the dependency we can think about potential alternatives. Depending on how active the community of your programming language is, you may have a lot of packages solving similar problems. Picking the right one can depend on a lot of factors but will mainly be driven by community support and popularity. Be mindful of bringing in very large dependencies to solve small problems and don’t forget to verify the licenses that come with the package.

Updating dependencies

Nowadays we have great tools like dependabot (part of Github now) that automatically keep your dependencies up-to-date with continuous pull requests. However, the review process for manual and automatic updates remains the same. Check if there aren’t any breaking changes. Semantic versioning can mostly be trusted, but you’re never 100% sure, especially when dealing with less widely-used packages.

Reverting a previous pull request

The main objective here is validating why the revert is happening. If it broke something, maybe there’s a simple fail-forward fix that doesn’t set you back as much. In any case, verify if the revert is reverting only what needs to be reverted. Pay special attention to merges and resolved merge conflicts in the git history. If the previous change you are reverting happened a while ago, check if no further changes were made since then. You might forget to revert something else or you might be leaving behind dead code.

In reality, pull requests tend to contain a mixture of some of the above. When was the last time you created a dedicated pull request to add a dependency? Right… Nonetheless, if you can identify the different types of changes that happen in a pull request, you can do multiple review passes, each focused on one aspect.

Optimizing pull requests for review

Most engineers prefer writing code over reviewing. It makes sense, in this case, to optimize the process for the person doing the review by putting the burden on the person writing the code. Here’s what we can do as developers to make reviewing easier, knowing that we will be rewarded with better reviews.

Small and focused

As stated above, different modifications require different review approaches. The best way to facilitate reviews is to create separate pull requests for each one. Do you want to add a feature that relies on a new dependency? Make two pull requests! It’s not that hard to do. People simply won’t bother giving feedback about your added dependency when they have 500 lines of feature code to review. Keeping clear separation according to the categories listed above has the natural side-effect of creating smaller PRs, which will be easier to review quickly and properly. On top of that, you can incrementally ship these changes to production and profit from their respective value. An added dependency or small refactor can already benefit the rest of the team without them having to wait for the rest of your feature.

Structure vs behaviour

If you have a hard time splitting up into small, one-dimensional pull requests, at least make this rough separation a hard rule. A pull request either introduces new behaviour or it changes the structure of existing behaviour. Reviewing pull requests that do both is a downright horrible experience. When you encounter this as a reviewer, don’t hesitate to ask the author to redo the pull request with a better split.

Better commits

When reviewing larger pull requests (> 50 lines modified) I notice a significant drop in focus. The cognitive load of asking questions about a big diff on your screen is just too much. I prefer to look at those pull requests in smaller increments. Having them split up in atomic commits helps a lot. Especially when dealing with refactors, commit-by-commit review allows me to follow the author’s thought process as if I’m walking through a story. Practice committing smaller changes more often and writing clear commit messages for each step. It not only helps the reviewer understand your thought process, but it leaves a journal to be inspected later and gives you short breaks to reflect on the change you just did. Ideally, find a workflow to commit straight from your IDE to reduce the context switch between code and commits, they should be one and the same.

Pairing for free reviews

If you still don’t like doing code review, make a habit of pairing up more often. Every additional pair of eyes will ask the important questions along the way instead of delaying the feedback until the moment the pull request is created. In my experience, changes that were pushed by multiple people simultaneously are more likely to get shipped quickly and result in fewer bugs.

 Automate what can be automated

Throughout my career, I’ve seen way too many pull request reviews where more than half of the feedback is code style-related. If you disagree with a certain code style as a reviewer, make a pull request that adds tooling to enforce this new code style. You now have a place to structurally discuss the preferred style with the rest of the team instead of hijacking an otherwise unrelated pull request. Adding more CI tools like static analysis can further reduce the need to review for things that are automatically caught by the tooling.

Pull request descriptions

As stated above, most code reviews start with a good understanding of the problem and the scope of the proposed solution. As a reviewer, I don’t want to spend time digging for this information. The burden is on the developer to properly summarise all of this in the pull request description. 

Review your own code

Right after I create a pull request and write the description, I review my own code changes before anyone else does. To facilitate this, I always write code with a dark background and I review with a light background and a different font. This is jarring enough to create the necessary mental separation for me to spot most of my own mistakes.

Follow up quickly

As soon as you have a pull request ready, it becomes more important than anything else you’re working on. That means that the moment feedback comes in, you should address it by either providing an answer or an additional code change. We take advantage of the fact that the reviewer has our pull request fresh in their mind by quickly acting upon it so we can request a re-review. Ignoring their feedback for several days and requesting another review after that shows disrespect to their review work and will require additional context-switching efforts from the reviewer.

All things considered, code reviews are a great way to spread knowledge and increase the quality of delivered code. It takes some effort to do well, but using the guidelines above will make it an overall more enjoyable experience for everyone involved.

More guides on coding principles and good practices