Nowadays it’s almost unthinkable to work in a Software Development team and not use Pull Requests to do Code Reviews. Pull Requests have become an essential part to our day-to-day work, but there are cases where we’re still not making the best use of them.

In this article, I will explain why it’s important to create good Pull Requests, what “good” means, and what you can do to raise effective Pull Requests.

The Need for Code Reviews

When I ask engineers why they send their code for review, the most common answer is “so other developers can find bugs in my code”, or to “check that the coding standard is followed”. The most shocking answer I’ve heard, more often than I’d like, is “because I need to merge my code.”

A professional software developer should aim to deliver software with high quality standards. This doesn’t just mean delivering code that works, but code that solves the right problem, that follows the right patterns, that is readable and maintainable, etc. We shouldn’t do code reviews to tick a box; we should do code reviews because we want to deliver good code.

We have several tools that allow us to do automated tests, be it unit tests, integrations tests, end-to-end tests, contract tests, etc. And similarly, we have linters to enforce coding standards. When used appropriately, these tools are much better than us mere humans at finding bugs in our code or identifying if there is a space where there shouldn’t be one. So finding this type of bugs is not a reasons to do code reviews.

So, why bother? Because someone with more experience can tell us when a different pattern or algorithm should be used, or that we should be using another library, or that not all requirements are being implemented!

As you can see, code reviews are really useful despite the rise of new technologies, including AI, but it has a cost: someone else’s time.

Help me help you

When we ask someone to review our code they spend some of their own time doing it. Note two important things from that sentence. First, that reviewers make use of time, a very scarce resource. Second, the use of the word ask rather than tell. Hopefully we ask, in a nice way, since they are using something so precious to help us.

One lesson I learnt from my mother when I was a kid is to make things as easy as possible when you ask for a favour from someone, partly out of respect, and partly because it makes more likely that they’ll help us again in the future. (Those were actually two lessons, one moral and one practical).

It’s precisely for those two reasons that I think we should make every practical effort to make our Pull Requests easy to review, so reviewers can spend as little time as possible looking at them. We are responsible for the code that we merge, not the reviewers. They are helping us ensure that we’re doing the right thing. This is why, we should help our reviewers help us improve our code.

Making Pull Requests Easier to review

There are many steps that we can follow to make Pull Requests easy to review. Not all of them are always applicable, or at the same time, so use your own judgement when deciding what you can do.

Prepare Short prs

As far as I know, there is no formal definition of what a small or a large PR are, but usually you’ll know when you see a small PR or a large PR. I consider a PR to be short when it doesn’t require a lot of mental effort to navigate though it. This definition is not very concrete, so let me show you some examples:

This is what I consider a short PR. There is one somewhat long file with unit tests, and I am happy that they added them. The actual feature changes are contained in a few lines of code and they are easy to read: https://github.com/rubocop/rubocop/pull/13287/files

And this is what I would consider a large PR. Note that there are several files changed, and that there are different types of changes on them: https://github.com/temporalio/temporal/pull/6726/files

I have repeatedly witnessed how some people raise large PRs, and how more people approve them, only to immediately have to fix something that was not observed by the reviewers.

Almost everyone I’ve discussed this topic with agrees that PRs should be short, but few of those people really do short PRs. After thinking about this for many years and discussing with some software developers, I believe that the main reason is that although engineers want to make short PRs, they usually don’t know how.

The way I do it is actually quite simple. It consists of two steps:

1. Reduce the scope of a task by splitting it in separate tasks
2. Complete the task and, if the PR is too large, break it down in smaller PRs

The first point, about reducing the scope, is something that junior engineers really struggle with. If this is you, I’d advise to work on this skill that will become handy for the rest of your career. I may post an article on that in the future, but in the meanwhile ask any experienced teammate whenever you feel that you could split some tasks.

The second point is something that I’ve seen irrespective of the level of experience.

Some people I’ve talked to honestly don’t know about how to split the code once they think it’s ready to ship. On a previous article I discussed the importance of mastering our tools, and this is an example of the impact of not knowing how to use them. In my case, the key was to learn how to use some git commands to do it. I will write a separate article to cover this separately, as I don’t want to deviate from the main topic.

Sometimes when I write code, I end up with a piece that I deem to large to send for review. I use git and I learn how to use it so I can take this big chunk of code and easily split it in different branches, so I can raise different PRs. When there are conflicts between the smaller PRs I know how to easily resolve those conflicts, so that’s not a problem.

There are a few cases when raising a short PR is impossible. For example, not long ago I changed an import on about 100 files for a refactor in our system. Due to the nature of the change it was not practical to reduce this any further. However, I was very careful to not add any other changes to that PR: the only change was that import on 100 files, and I added a comment explaining exactly that. My PR was approved in less than 5 minutes without sacrificing quality or adding unneeded risks.

PAIR PROGRAMMING

On those few occasions when you have to write a long PR and the advise above is not practical, doing pair programming could save the day.

When two people code together they usually cover all the things that they would consider during a code review, but while the code is being written, so it can be just approved by the other person as soon as the Pull Request is sent.

The only drawback of this approach is that PRs are also used as documentation for past changes. If you use this approach to merge a large PR, anyone going back in history to understand a change may have a hard time doing so.

Code Walkthroughs

Code walkthroughs were one of the main ways of reviewing code before the internet era. When the first source code versioning tools appeared this was still a popular way to do code reviews. And they even regained popularity when extreme programming became a thing.

I believe that this shares the same pros and cons of pair programming, so please still avoid large PRs when you can.

Comments

You probably know this already, but PR submitters can also add comments to the PRs. If you can add a comment that will give useful information to the reviewer please do it, but take these two things into consideration before you do so:

• Consider if the info should perhaps be a code comment, rather than a PR comment
• Please avoid comments in the code unless they really add value

Screenshots

If you are making UI changes I would strongly advise you to add some screenshots that show the change. If you can show the before and after even better!

Screenshots are extremely useful to visualise why a change was needed, and you may be shown a better way to achieve the same outcome. This is only possible if the reviewer can understand what you’re trying to do, and, as you know, a picture is worth a thousand words.

Meaningful Descriptions

When you raise a PR, please take the time to explain why you’re doing that change, give any relevant context to the reviewer can understand, and summarise what you are changing.

Even if you know that the person reviewing the code knows all that, the person looking at that PR in 2 years may not.

Videos

If you can you record a video with a demo, that’s bonus points. You can even recycle the video to do a demo for your team.

Demos are a great addition to any PR if you can give useful information to the reviewer. Just as the description, the demo may include context information, show interaction with other services, or how a frontend elements behaves on a specific event.

Summary

In this article, I explained why we do code reviews, and why we need to do more when we raise pull requests to make the review process easy for the reviewers. I will not deny that the suggestions I made require more effort from the PR submitted. However, I think that that is precisely the idea. The effort should be made by the person raising a PR, rather than the reviewer. This will allow you to deliver better quality code and reduce the time it takes to get your PRs approved.