About reviewing merge requests

If you catch yourself saying “Looks good to me!” And then just merge it, you are doing yourself, the author and the organization a disservice. A merge request should not be considered a triviality, it is actually one of the best places to communicate about various aspects of the code or the project

For example, you can:

1. Catch unnecessary complexity.
2. Stop introducing difficulty for new people getting started.
3. Guard against building solutions for problems you will never encounter.

A merge request is a perfect anchor

A merge request is perfect anchor to guard code quality and business use cases.

It should become a repeatable process. It is good to have some kind of checklist everyone on the teams goes through to guard against common pitfalls of software engineers.

Two heads are better than one

Two people know more than one. Where long hours of coding can result in making error you don’t normally make, another person can catch this quite easily.

Sharing knowledge

By giving feedback on documentation you find situations in which people assume other people are just as knowledgeable.

This is normally not the case. It forces the author to think about others using the code and the reviewer might also learn new mechanics.

It helps the creative principle a lot.

Properly reviewing a merge request

The most important part of reviewing a merge request is that you do it in a structured way. Every organization differs so what you find important can vary as well. In this section I will give you some pointers to consider

Repeatability

If there is no checklist in your organization for reviewing a merge request, you should introduce one. If people find it lame, you should just use one for yourself.

A checklist helps you learn what you find important in a mege request. It gives you a way in which you can stop errors from being repeated.

For example, if you merge something without test cases, and it gives you problems later, you can adjust. You can require new test cases for every bug fix branch. If this issues keeps popping up, you can then add it to your code style requirements or checklists.

It’s easy to get started

Check out the code and see if you can get it running with the README. I saw a team member do this once and though this is genius.

If you can’t get the software stack up and running without thinking, it is too complicated. It will actually hinder contributions.

The purpose is clear

It is clear what the proposed code does and doesn’t do. It is also important to know how it is connected to other components. A merge request which is too large for example, lacks a clear purpose

It’s clear how to deploy

If you are a small team without a CI/CD pipeline, deployment instructions should be clear. If you are using a CI/CD pipeline it should also be indicated. Why? A git project is often the starting point for a developer to write code. The final purpose of writing code is to solve a problem for the end user, not just to write code.

In my opinion an every merge request should be tested on that:

1. A developer can get started with development
2. A developer can ship new code
3. A customers request or problem can be solved
4. Next steps to alert customers on a new feature are clear and can be started.

Commit Quality

Make sure the commits are well-structured and descriptive. You can also ask the author to use a specific template.

For example:

• A feature branch starts with “feat-”
• A bug fix branch starts with “bug-”
• A branch contains a ticket number

Another example can be to have a ticket number in the branch name.

In addition, you should not accept very large change sets with unrelated changes.

You want the history to be understandable

Code Quality

The best way to review code quality is to take into account the clean coding principles. Code should read like a good story. Implementation details should be buried further down and method and variables names should be descriptive.