How to properly review a merge request

One of the most important parts of a merge request is proper reviewing. Over the years, I have reviewed a lot of merge requests and believe I am pretty good at it. This has led me to write this blog: I want to share some tips that - I think - can help you in becoming a better reviewer.

As you could have read in my earlier blog, I am a big fan of merge requests because they contribute tremendously to software quality. A merge request (also known as a pull request) is the request to review newly written code by several colleagues before the code becomes a definitive part of the software. These peer reviews ensure the code is checked for correctness, written clearly, and meets the agreed standards. Only when everything is in order will the reviewer agree to 'merge' the new code with the existing code.

Preparing for the review

In most Git repository managers with merge/pull request functionality (Gitlab, Bitbucket), there are several user preferences that can be configured to aid you in the reviewing process. If you tweak these settings to match your needs, it helps you conduct a better review. In the examples below, I will highlight two of the settings I use and recommend.

Visualisation while comparing changes

First of all, it is possible to configure the way that the comparison of changes is visualised. Personally, I prefer the side-by-side mode because the inline difference mode hinders me from reading the final source code fluently.

As you can see in the image above, you have to skip the red lines to read the complete result in the inline view. While in the side-by-side view (shown below), the complete result is already displayed on the right-hand side.

Visualisation of changed files

Another setting that can be customized is the way how changed files are arranged. For instance, Gitlab displays all changed files at once by default. As a result, I have difficulties finding where the next changed file starts and I am not really able to follow which changed file I am currently reviewing. I prefer using the Show one file at a time setting and clicking through the changed files one by one. This also helps me in checking the correctness of the location of the changed files.

The above images show a file 'data.tf', which has multiple changes. In the All changed files view, you can hardly see which changes are in the same file. This isn't an issue in the One file at a time view (shown below) because you know that only one file is displayed.

Reviewing a merge request

Requirements

Before starting the review, I try to find the requirements that led to the merge request. Usually, merge requests are coupled with a ticket or issue number where you will find the requirements. If it is unclear what the requirements are, it will be very hard to conduct a good review. I strongly advise you to contact the person who created the merge request to discuss this.

Once the requirements are clear, I always try to imagine how I would tackle the implementation. During the review, I check how the changes compare with what I had in mind. If it deviates from what I expected, it can still match the requirements. If I think my solution would have been better, I place a comment to discuss my alternative.

Testing

Test coverage is important. The first thing I usually do when reviewing is to search for the test that has been added or changed in the merge request to check if the test assertions match the requirements. If the test is well written, it clarifies the requirements and helps do the rest of the review. I am always surprised (and a bit disappointed) if a merge request does not contain a new or modified test case. Most of the time, it turns out it should have been there. Merge requests almost always (except for refactoring merge requests) introduces new functionality, which must be documented with a test.

Naming

I carefully consider the naming for all classes, tests, and functions. More information about this can be found in my blog about naming things.

Commenting

If I add a comment on a merge request, I always try to make it constructive. I will outline why something can be improved and, above all, how I think it should be done. For example, if I don't like the name of a class, I always include the alternative name I have in mind. When I have had an in-person discussion with the developer, I always add a summary to my comment so that the next reviewer also knows what has been discussed.

Commit messages

I often use commit messages to trace why and when certain code was added. I don't like 'squash before merge' because the resulting commits often become too large and tracing back the reason for changing a piece of code becomes harder. I check if the messages are clear and whether they are according to the standards agreed with the team. I also check if the commits are cohesive and logical; if not, I suggest squashing those commits.

Style

In my opinion, checking code style like line endings, white space, line length etcetera should not be part of a merge request. Discuss code style with your team and choose tools to maintain the style so that it will never be an issue in merge requests. For instance, IntelliJ offers the functionality to format a file during a commit automatically. Another option is to use a pre-commit hook, see, for instance, Ktlint for Kotlin.

Source:

• Screenshots taken from https://gitlab.com/gitlab-examples/ops/configure-product-walkthrough/-/merge_requests/5/diffs