A good pull request is small
Agile teaches frequent delivery so we get fast feedback. Lean is similar in that small batch sizes help decrease risk and find issues faster.
This is all true for code as well! In this article, I’ll share how my team in MoJ transformed by keeping our PRs under 200 lines of code.
I have written this for any team using pull requests and their managers. If you’ve yet to adopt a PR process in your team, or you are working alone. I highly recommend doing so.
Review your code for quality
Any artisan or craftsman will polish their work. Nothing is perfect on the first try.
So why would we merge our code without review? It is clear why pull requests have become common practice and the classic Code Complete even has an entire chapter (21.3) dedicated to it.
McConnell makes the case that 45-70% of defects can be found during a formal review, a number that is far higher than that of writing tests (do tests and reviews!).
For a review of system code written in a high-level language, reviewers can prepare at only about 125 lines of code per hour (Humphrey 1989).
McConnel, Steve, Code Complete (2nd Edition), 21.3 Formal Inspections.
However, reviewing code takes time and focus. Doing this for a long time will render poor results and as McConnel states it may take an hour to properly review 125 LOC. This makes it obvious that even 200 LOC may be too much.
Finally, aside from finding defects, reviewing our code makes it more maintainable.
I even find reviewing my own code useful. Seeing it in another light (the Github diff for example), helps me find things my IDE didn’t show. Files I added unintentionally or debug code lines.
Common reasons not to do PRs
Doing reviews is common practice now but there are still times when it isn’t being done or done without care. Let’s look at the reasons:
- Urgency
- Stress
- Laziness
- Resourcing
- Feasibility
Urgency. We “cannot” wait for a review. We have to release now because thousands of users are waiting for a fix.
But, a pair review might save us from making a mistake that turns a 5-minute outage to a 1-hour outage and loss of data. If it is so important to get a fix out right away, is it not also important enough for someone else to help to review it?
Stress. We don’t have time to do reviews. However the earlier we find bugs and issues the cheaper they are to fix. If we are so short of time, can we afford not to review?
Laziness. Let’s face it, reviewing code isn’t fun. It’s a chore. Like doing dishes or washing clothes, but, because it’s a chore it doesn’t mean we can skip it.
Resourcing. There is no one to do the review. But any review is better than no review. Maybe a member from another team can review? Or, at least, you can review yourself.
Feasibility. Reviewing a 3000 line PR according to McConnels measures in Code Complete would take approximately 24h. Without cutting corners that’s impossible. Just skimming a PR will not help you find issues either.
The solution? Keep the PR small, this is a pre-requisitive to do good reviews.
Why I pushed having small PRs
I didn’t aim to make smaller PRs. I wanted better PRs. PRs with more comments, comments that got addressed.
We had big PRs that when commented on, changes would take days which no-one was keen to do.
But doing PRs is accepted practice right, it should be helpful? Were we doing it wrong?
So I went out to research and came across this article on The anatomy of a perfect pull request.
First, I thought I would measure and increase the number of comments. After reading the article though, I decided to try small PRs. I began with my own, trying to build a process and examples.
I found how not only did I get more comments but it was also much easier for me to explain the work I did and make sure I had test coverage and refactoring under control.
This led me to write Work Work Stats, an application to monitor your PR Github project PR size. Later I PullPanda does the exact same thing (and more).
It was also good because setting a goal for PR size is easy. But it requires things such as unit tests and a CI pipeline. These are things that are worthwhile but hard to motivate. Now we had the motivation, we couldn’t do small PRs without them.
It might sound strange but, if you want to have 200 LOC PRs you have to have unit tests. And so, this seemingly arbitrary goal forces you to add an enabler for high quality.
Ways to better PRs
So how is it possible to do PRs under 200 LoC. Surely it’s impossible?
Impossible no, difficult yes. When I was working at Rakuten, a fellow engineer showed me how after he had completed one set of work he split it up. Each PR was small and had a concrete purpose. It was easy to follow and a lot easier to review.
But if we have a continuous deployment, won’t this get released before it’s done? Yes, it would, but that’s fine isn’t?
If you have a solid set of tests. Both unit tests, integration tests and preferably UI tests. If all pass, you have not changed or broken anything. You have only added in scaffolding for the next part of your story.
If you have a bigger feature or a UI that’s not completed, you can add feature flags. Feature flags allow you to merge first and release when you want to. It requires a little bit more overhead but actually makes things easier.
Finally, adding code bit by bit to master is rather easy. It is also much less painful and risky than testing and merging big release branches.
It is easy to start
Try it! It’s easy to start this kind of thing. Even if you don’t have a great CI and test system, starting with really small PRs, you can still safely merge them. But having the CI and tests add confidence.
For example, if you have a one-liner changing an if-statement. Try PRing that one thing instead of bundling it with fifty other changs. You will see your team members easily spot any mistake and it will fly through review quickly.
And of course, 200 LOC is a goal. It isn’t a rule to die for. Sometimes your PR will be 500 lines, it’s fine.
If you aim for 500 LOC though, you’ll see that you end up way higher. Generated files or binary files doesn’t make sense to count nor to review. Just consider, should these files have been changed?
That’s it for this time. I have many more things to say about reviews. Since started this good-pr-journey I have learned a lot. This post is only the first of what I’ve come to know and I hope to continue sharing what good PRs are. It’s a big topic.
I hope you found something worthwhile in here though and if you have ideas or suggestions I always appreciate your thoughts. Thank you for reading!