Improving Code Quality using Pull Requests

Originally posted in company blog

Pull requests are a bit like writing automated tests. First, the usefulness of them is hard to measure, since each project is different. At the same time using both become industry standard. Yet, there is still a lot of skeptics. They think that tests/pull requests slow developers from delivering functionality faster. Finally, both lose meaning, if they are used just for the sake of using.

Another interesting thing is how tests and pull requests, besides their main purpose, improve other areas. For example, tests and especially TDD can help improve code quality and design. In this post, I will try to provide good practices for pull requests. And I will also try to mention how they can improve other areas.

Before starting, one could ask why do pull requests at all. I will not go deep into it, each team can decide why pull requests are important, or why they are not. Yet, these are the things where pull requests are useful:

• Giving confidence for the author, that the code is good enough for merging
• Sharing knowledge and understanding
• Making ourselves less lazy
• Noticing bugs

How to prepare Pull Request better

Keep it small

That is one of the most common problems with PRs, and it can affect overall experience while using pull requests. It’s hard to understand changes and notice problems when there are a lot of files. Depending on team members, that usually results in two situations:

reviewers just skim through files and approve without reading review drags on because it causes multiple discussions and the author must fix a lot of files There are different techniques to reduce the size of PR. What you can do depends on the type of project and release cycles. You could first introduce a partially working solution, and then with following pull requests expand it. For example, if you need to implement a data table with filtering and sorting, you could start with just a table, then add sorting with other pull requests and finally filtering. Or if you have a single repository for the project, you could introduce API changes first, and UI later.

If you do not want to merge partially working functionality into a common branch, there are other techniques. You could introduce pull requests into the feature branch. With this technique, you need to create branches from your main feature branch and create pull requests back to it. It does add overhead and playing around with branches, but it might work for you.

There are other ways how to split a large problem into smaller ones, and it depends on how each person tackles it. But by trying to split it into smaller chunks you can at the same time improve your problem-solving skills. This, in turn, helps in task estimation and other areas, because even before coding, you can split a task into easy to digest parts better.

All things considered, think if large pull requests are not a symptom of too big tasks/stories. All these techniques are rarely needed if tasks are small enough.

Refactor with separate Pull Request

This tightly relates to the previous tip about small pull requests, but I wanted to mention it because it is a good technique for development even without using pull requests. The general rule states:

First refactor to make it easy to add new functionality, and only then add new functionality.

It’s a great technique because instead of over-engineering, you allow new functionality guide design of the system.

If you follow this rule, you can create simpler pull requests for your team to review. This adds a lot of benefits. It automatically splits huge pull request into few smaller ones. At the same time, if refactoring affects a lot of files, you introduce them sooner, rather than later. I have not always followed this rule, and it often caused painful merge conflicts, because other team members were editing the same files.

The only downside of this approach might be if you introduce refactoring tightly to new feature you are implementing and for some reason, the feature is no longer added to the codebase, and you are left with code refactored and prepared for functionality that is not needed.

Use git commit messages wisely

Another general programming tip that translates well to pull requests is good to commit messages. It documents code and all the changes in a pull request. Think of it this way: instead of creating description where you list changes you have made, commit messages will document that. And in pull request description you can write other relevant information. And as a side effect, I really liked the description, I heard from colleague, Lina Vegienė:

Writing meaningful commit messages, makes you structure tasks better and divide your work into meaningful parts.

Comment code yourself

If you have doubts about the implementation of a feature, better discuss them before start coding and not do that in a pull request. But sometimes it might be less important things you are not sure of. For example - naming method or class. You can mention that in the pull request and somebody might suggest another name that you like better.

Another way you might help reviewers is by commenting important places of the code or even explaining why you have done the things you have done. You can use it to guide reviewers to the changes you have made.

Introduce new ideas with PRs

As I’ve mentioned earlier, it is better to discuss things before starting to code. But sometimes you might end up in the situation, where it’s just hard to imagine if the suggested solution would work without seeing the actual code. In this case, you can create proof of concept solution and create a pull request for it. It doesn’t have to be merged, but PR review page is a good place to discuss that solution.

How to Review Pull Request Better:

Do not approve PRs blindly

One of the worst practices to do if you are doing pull requests. If multiple developers approve requests without reading them, it means that you aren’t actually doing code reviews and pull requests, you are just creating an illusion.

At the beginning I have mentioned, that one of the qualities of a pull request process is created confidence that your code is good enough to merge. You get this confidence because you know that other developers reviewed requirements against code, and checked readability. Reasonably sized pull request was approved 5 seconds after creation, you have a problem.

If you are busy at the time, better leave pull request for others to review and come back to it later. I would rather wait longer for my pull request to be approved, but at least then I would know that somebody looked into it.

Be polite, ask first

Sometimes you do not know why the certain code was written the way it was, and you might tell someone to fix a certain problem. Sometimes it is polite to first ask the question and then suggest improvement. For example: Why have you iterated list four times instead of one?

When you really are familiarized with code, go ahead and suggest improvement. But don’t forget to be polite as well. There is a difference how you can communicate the same information:

Split this method into three smaller ones

vs.

It would increase readability if you would split this method into 3 smaller ones.

A thing to remember is that people reading your comments do not see your facial expression. And you might have best intentions with your suggestion, but once it becomes text, it might seem mean or rude. And I had many situations where I wanted to just be concise and clear, but the person receiving the suggestion was not happy about that.

Use code snippets

When I started doing code reviews, I often wrote things like “This could be simplified”, “You could use lodash here”, etc. But then the author was not sure what I mean or did not know how to implement the suggestion. Gradually, I replaced generic messages with code snippets. It does take more time, but at the same time, it’s much easier to compare the current code with suggested improvements and see if it’s even worth doing. Also, the snippet might not be 100% accurate, the main goal is to convey information to the author more clearly.

The thing to remember is that this suggestion is not a hard rule. If you feel that some part of the code is hard to read, you can express that in different ways. Sometimes just stating concern about complexity might be enough. It might help the author notice a problem and improve the code on his own.

Review the code even if you are new

Often new team members are not sure who’s pull requests they can review or even when they can review PRs at all. Each team can have a different agreement when a person can “approve“ pull requests, but you should start reading and even commenting pull requests right away. You might even add questions for the author about general ways of coding in the project and learn best practices.

How to Improve Overall Pull Request Process

Make use of static analysis tools

One of the things reviewers tend to notice is code style. It might be multiple empty lines, unused variables, and functions etc. Pull requests help to align different styles of code across developers. It does not mean that one style is better than another but having a consistent style across codebase is better compared to random style for each file. However, using pull request for that has multiple problems. One is that developers are just humans, and they will not always notice these problems.

Another is that comments about code style can take quite a lot of pull request review. They are for sure easy to fix, but when there-there are ten comments about style and one about readability, then the most important can get lost easily.

All things considered, I am not saying that it’s not worth to mention style problems in PR, but if you notice certain problems too often, consider using linting tool to notice those before pull request. If a robot can do things for you and even better, why do it yourself? Moreover, it’s easier when the robot notices problems - you cannot take offense.

Have a checklist for things to require before approving

Each developer decides if a pull request is ready to be merged, or not. But if the reviewer is stubborn, he could avoid approving pull request, even though there are no major problems with it. When you start noticing, that kind of behavior, discuss and agree on common values for the code. Then using values as a basis, decide on things required for the pull request to be approved. For example, if tests are important, make a rule that bug fix requires a new test or existing test fix. Having list of required things before approve really helps for new team members and reduces the number of discussions when PR is not being approved.

Stop large threads of discussions sooner than later

Programmers love their code. Imagine you have worked all day on the particular algorithm and somebody just starts to dissect it and tells to do things differently. Even worse, the reviewer might not be particularly polite about that. He might be wrong, or he might be right, but the point is that this could cause long discussion thread, where you exchange arguments why one or another way is better. Even if the conversation is polite, you should stop it before it gets too long and has a live discussion about that. Sometimes it’s enough to just go to other developer desk and chat, and sometimes you might need to schedule a meeting.

Gamify process

When you find trouble making developers commit to reviewing pull requests, instead of adding rules immediately, try adding some fun to the process. If you have a TV in office box that shows stats, add a new section which shows some information about pull requests. For example, you could show requests, that are pending for quite some time and are not reviewed. Additionally, you could have stats like “Reviewer that added most comments this month” or other fun statistics, that would encourage people to commit to reviewing. There are many other creative ways to improve the process and encourage the developer to participate.