Tips for Creating Merge Requests and Doing Code Reviews

The post provides tips for creating and reviewing merge requests (also known as pull requests). These tips will allow you to step up your game and manage the process like a pro.

Introduction

In software development, the process of creating merge requests (MRs) and peer-reviewing them via code reviews (CRs) represents a vital practice. Here is a couple of reasons why:

• Finding bugs and other defects. It is better to catch bugs during code review than in production. Apart from bugs, you also want to aim at finding performance issues or security vulnerabilities.
• Slowing down the process of gradual degradation by improving code quality and maintainability.
• Learning something new. The author of the code might use something useful that you have not seen yet, like a programming-language feature or library function.
• Getting familiar with code that is new to you. Code reviews help transfer knowledge about the codebase among your peers.
• Increasing the sense of mutual responsibility within your team. This facilitates collective code ownership.
• Finding a better solution. The reviewer may be familiar with a library, technique, or algorithm that will allow you to make the code shorter, more efficient, etc.
• Complying with formal requirements (e.g. QA). Depending on the company policies or business area, CRs may be mandatory.
• Writing better code. I do not know about you, but when I know that someone will review the code that I am writing, I tend to care more about it and generally produce better code.

Like many other aspects of software engineering, writing MRs and doing CRs is a skill. You can increase your proficiency by gaining more experience and learning new techniques. The present blog post will not help you with the former, experience-related part, but it will be helpful with the latter.

Before we take a look at tips on creating good MRs, let me address the following question: Why does GitHub call MRs pull requests (PRs)? There are basically two reasons:

• Historical: The PR terminology originates from the git request-pull command, which is now rarely used.
• Practical: If the PR was created from a fork, you cannot straightforwardly merge the underlying branch as it is on a different remote. What you need to do is to pull the changes from their repository into your repository via git pull. You can see the exact steps by looking at GitHub’s instructions for merging a PR manually.

Anyway, from a terminology viewpoint, you merge PRs even on GitHub. Since I like the term MR more, I will use it throughout the rest of the blog post. Simply bear in mind that a MR and PR are the same concept.

How to Create Merge Requests

Our work starts with creating a MR.

Do a Self-Review Before Submitting a MR

The reviewer is not responsible for your carelessness. Before submitting a MR, you should go over your changes and:

• Ensure that the code works as expected by including tests for all the new and modified code.
• Ensure that the code conforms to coding guidelines, including formatting.
• Proofread variable and function names, comments, strings, etc. and check for grammar and typos.

In general, when creating a MR, assume that there will not be any CR and the code will go straight into production, i.e. that you are the sole person responsible. That is, take extra care.

The Complicated the MR the More Detailed the Description Should Be

Try to put yourself into the reviewer’s shoes. If I were to review code, what information would help me in the process? A few examples:

• Describe design and implementation decisions.
• What were the major issues?
• Why did you decide to solve them in a particular way?
• Were there any other options?
• Are there any problems to discuss?
• Is there anything that needs to be done when deploying the code?
• Include any relevant tickets from your bug-tracking system.

As the author of the code, it is your responsibility to make the MR as reviewable as possible.

Make Commits in a MR Read Like a Story

Who says that programmers cannot be writers? From my code-reviewing experience, being able to go over the commits from the bottom to the top and seeing the connection between them facilities the review process. Otherwise, it forces the reviewer to look at all changes together, which may be much more difficult if the MR is big.

This also means that each commit should be atomic. In short, an atomic change revolves around a single topic and one topic only. This means that you should separate refactoring, adding new functionality, and fixing the current code into separate commits. The advantages that you get by creating atomic commits are outside the scope of this blog post, so let us move on to the next tip.

Do Not Be Afraid to Leave Comments by Yourself

If you want to discuss something with the reviewer, leave a comment. Sometimes, you may not be sure about a change or would like to debate over several possible approaches. This is fine. However, leaving a comment is not a way to excuse your negligence. Always do your due diligence and come prepared into discussions.

Larger Changes/MRs Should Be Pre-Approved

This should be done to minimize the risk of them not being accepted. Before opening a huge or controversial MR, reach out to the authors of the code and get their pre-approval. In this way, it is less likely that your work will come in vain.

An extra tip: When in doubt, ask for a concept review to verify that the way you have decided to go has potential. In this case, the MR does not have to be complete or have tests. It can only be a draft to show your idea.

Every Comment From the Reviewer Should Make You Think

Comments from the reviewer should make you wonder: Why have I not thought about that? How can I improve the code, MR, etc. in the future? If your reaction to a comment is “Oh, no, I was afraid that they were going to find out.”, you are wasting reviewer’s time. As advised earlier, if you are honestly not sure about something, you should raise a comment by yourself instead of calculating on not getting caught.

Include Only Directly Related Changes

Do not include irrelevant fixes of typos, formatting changes, etc. In general, do not solve multiple issues in the same MR. It makes the MR bigger and harder to review (“What is this change doing here? Oh, it fixes a separate issue.”). Also, by putting unrelated changes into separate MRs, you enable reviewers to go over them in parallel. For example, the harder of the two MRs may be reviewed by someone with more experience and the easier one can be reviewed by a junior who is getting familiar with the project. If you put several unrelated changes into the same MR, how would you make that possible?

Consider Squashing Minor Fixes via Interactive Rebase

Instead of including commits like “Fix typo in the previous commit.”, consider squashing them into the original commit via interactive rebase. However, since interactive rebasing changes history, it might be unwanted as it requires you to do a forced push, which can make subsequent reviews harder. It all depends on your preferred way of doing reviews and dealing with comments. Some people like that, some do not.

Accept the Fact That Not All MRs Will Be Merged

Sometimes, you find that your proposed change is not the way to go. C’est la vie. Learn from that experience and move on.

How to Review Merge Requests

After a MR has been submitted, it is time to review it.

What to Focus On

There are lots of areas that you can focus on during a review. What I recommend is to focus on the most important aspects first, and then move to the less-critical ones. Who cares if there is a typo or incorrect indentation when there is a security hole or when the code does not do what it is supposed to do?

Let me divide the aspects you should focus on into four categories P1 through P4, depending on their priority. Each category also includes sample questions that you should be asking.

P1: Does the code do what it should and does not do something it should not do?

• Does the project work and do all the tests pass?
• Are there tests for the new code?
• Has the documentation been updated?
• What about backward compatibility? Are there any specific actions needed, like increasing the version number or changing the database schema?

P2: Is the code safe?

• Are errors correctly handled?
• Is it impossible for the program to crash?
• Is the code free of security flaws?
• Is the code thread-safe?
• Are there no resource leaks, like memory leaks?

P3: Is the code readable, maintainable, and not needlessly inefficient?

• Does the code fit into the project or was it hacked there (e.g. via shotgun surgery)?
• Is there a more idiomatic way of writing something?
• Can the code be shortened by using existing libraries?
• Is there no duplication?
• Are there no useless things that unnecessarily slow down the code?

P4: Does the code conform to project’s coding conventions?

• Spaces vs tabs.
• No useless trailing whitespace.
• Naming of variables (snake_case vs camelCase).
• Code formatting in general (placement of curly braces, line wrapping, etc.).
• Typos and grammar in strings and comments.

Be Respectful, but Brutally Honest

If there is something wrong, it is your duty to report it, but in a respectful way. Withholding an issue only because you are afraid that the other side might get upset is plain wrong.

You Are Reviewing the Code, Not the Person

So let us not get personal. For example, instead of “You have made a mistake here.”, write “I believe this should be X because of Y. Could you please verify?”.

Strive to Make Useful and Informative Remarks

…and leave the useless ones at home. In general:

• Include a reason why. If you do not like the current approach, explain why, and be as specific as possible.
• If you criticize something, include an alternative way to consider. Stay away from mindless bashing.
• Include links to supportive material, like articles or talks. If there is an issue that is described somewhere else, then there is no need to get into details. A simple link suffices. If there is no article and you have already seen the issue several times, consider writing one yourself.
• Ask questions if you do not understand something. Do not pretend that you were able to grasp everything just to save face. Always strive to learn. Also, the fact that you do not understand something can mean that there might be a bug or the code is not self-explanatory enough. Ask for clarification.
• Ask questions to make the MR creator think (e.g. “What happens if this function fails and returns a null pointer?”).
• Report issues properly. Include steps to reproduce, expected behavior, and actual behavior.
• An advanced tip: Consider reporting an issue by crafting a failing test. A failing test includes everything from the point above and allows the other party to directly include it into the codebase.

Show Honest Appreciation

Remember that it has to be genuine and specific. That is, it should not be cheesy or generic. Examples:

• “Cool, I did not know about distutils.util.strtobool(). Nice!”
• “Thank you for analyzing the referential Perl code, it must have been hard.”
• “I have learned a new word today (‘spuriously’), thanks!” (From a spurious test fail.)

Always Leave a Comment

Even if only a plain and simple “LGTM :thumbsup:”. Merging a MR without any comments may leave the impression that you did not even bother looking at the code.

Mind the Wording

Pay close attention to the words that you choose. Recommendations:

• Use “I suggest” or “Consider” for non-critical issues.
• Use we and us instead of I and you (collective code ownership).
• Prefix minor issues with “Nitpick:” so the author sees that you are just being thorough.

One MR Can Be Reviewed by Multiple People

Changes to critical parts of the code should be reviewed by multiple people. Also, if you have been assigned to review a matter in which you have only limited experience, do not be afraid to ask for help.

Finish the Review in a Timely Manner

Do not wait a month to do the review. First, it discourages people from contributing to your code. Second, it prolongs the review process as everybody needs to get familiar with the code after not seeing it for a while. If doing a self-review before submitting a MR is the author’s responsibility, finishing reviews in a timely manner is the reviewer’s responsibility.

Additionally, if you are the author, either respond to the comments on time, or write that you are unable to do so. In that way, project maintainers can either do the changes themselves (if the MR is important) or close the MR.

How to Discuss Comments

Finally, both the MR author and reviewer need to be able to effectively discuss comments.

Show Appreciation

It feels good to be appreciated. It also shows the author or reviewer that you welcome their comments. Examples:

• “A very good point.”
• “Nice catch!”
• “I did not know about that, thank you!”
• “The proposed alternative is indeed better. Let’s use it.”

Do Not Take Comments Personally

It is (well, should be) the code that is being discussed, not you. So, do not take comments personally. The fact that you have made a bug or missed something does not make you a bad person. Do not feel sad about it. Learn from your mistakes and always try to do better in the future.

Do Not Be Afraid to Disagree

A CR should be a discussion, not a list of commands. However, should you disagree, explain why. And please, let the reason not be “Screw it, I am too lazy to do that” or “I decided I do not care”.

Do Not Be Afraid to Ask for Help

You can invite other people into the discussion and ask for their opinion. However, beware of the Law of triviality (also known as bikeshedding), which states that people give disproportionate weight to trivial issues.

Add a Reaction to All Comments and Mark Discussions As Resolved

Explain how each issue has been resolved. This is helpful when someone else (or even you) stumbles upon the issue in the future and would like to see how it has been resolved and why.

For trivial issues, marking the discussion as resolved is enough though. Or even a simple thumbs-up emoji may often suffice.

Summary

To conclude the post, here is a recap of the most important tips for creating MRs and reviewing them:

• Do a self-review before submitting a MR.
• Try to make the MR as reviewable as possible.
• Every comment from the reviewer should make you think.
• Focus on the most important aspects first.
• Strive to make useful and informative comments.
• Focus on the code and leave personal issues behind.
• Show honest appreciation.
• Do not be afraid to disagree.