Reviewing is a core process in software development. It is a form of communication between two parties with different purposes. As with any form of communication, they can always be improved upon and the reviewing process is not an exception. Both the code proposer and the code reviewer play their parts, but in this article, we’ll focus on the proposer.
For the developer that is proposing the code, the main goal is to pass all the checkmarks of the reviewer. Then, the code will be merged into the codebase and shipped to production somewhere in the (near) future. Ideally, the code proposer would like to get through this entire process with minimal feedback and few back-and-forths from the reviewer’s side.
To prevent this, we as developers oftentimes emphasize making our code better. The assumption is that when the quality and readability of our code is high enough, the resulting Pull Request (PR) will be easier to review. In turn, this should lead to the desired results of less feedback, back-and-forths, and time spent in the reviewing process.
However, how easy a PR is to review is not only dependent on the code that it’s proposing. A PR that contains extremely high quality and readable code can still be hard to review for other developers. That’s because the quality of a PR isn’t equal to the quality of the proposing code. More than that, there are a lot of aspects to a PR that are unrelated to the code but that can still affect how easy the PR is to review.
I’ve gone through an enormous amount of PRs during my career as a developer, from both the code proposer and reviewer’s side. As a code proposer, I’ve made countless ones that ended up being very difficult to review for my colleagues. But none of the issues was related to the code, but rather about the PR itself. On the other side, as a code reviewer, I’ve also experienced a lot of different things that colleagues didn’t do in their PRs that significantly affected my reviewing experience.
Based on this personal experience, this article will share 5 non-code related mistakes that developers make that leads to their PRs being hard to review. Using this information, you’ll be able to avoid these mistakes yourself. In turn, this will help you create easier to review PRs, spend less time in the reviewing process, get your work shipped to production quicker, and in general improve the quality of the review process inside the engineering team.
This article is an entry in Reviewtober, the October series in which we explore how to improve at the reviewing process as React developers. The other entries can be found at Uncommon React, a newsletter focused on helping you become a better React developer by focusing on the less commonly discussed topics, like testing, readability, and more.
Leaving Out Contextual Information
The most crucial information to any PR, even more, important than the code itself, is contextual information. Information like the purpose of the PR, the reason code has to be changed, what bug the current PR solves, and so on. Without these types of contextual information, it’s extremely hard or even impossible for other developers to properly review your code. It’s like asking your colleagues to verify whether a car is working properly without telling them that it’s a car, what a car is, or why the car was necessary, and only showing them the car.
So often have I created a PR and left out the contextual information. Either I thought the PR was already self-evident, too small to warrant a lot of information, that it was unnecessary, or I was just too lazy to add it. Although there were several times where I got away with this, it didn’t change the fact that my assumptions were wrong and that I neglected my reviewers.
Every PR introduces code changes with a reason, purpose, and context. Without contextual information, any code change would be purposeless and unnecessary. That’s why it’s extremely important to include them in the PR. All the contextual information around the code is what provides the reviewer with a foundational layer to look at your code and associate it with the actual real-life issue you’re trying to solve. In that sense, leaving out contextual information from your PR is equivalent to actively handicapping your reviewers.
Including Unrelated Changes
Whether it’s to fix a big, introduce a new feature, pay off technical debt, automate processes, or update documentation, there’s always a single purpose to your PR. When reviewing your code, the assumption is that all of the included code is contributing towards that single goal.
Therefore, the worse thing a code proposer can do is include code changes in the PR that are unrelated to that purpose. Not only does it add noise to the PR which can distract the reviewer, but it will also confuse the reviewer. From personal experience, I’ve learned that this happens no matter the size of the unrelated changes. No matter how small the noise is that you’re adding, in the end it’s still noise.
Under the above-mentioned assumption, the reviewer will try to relate every code change towards the specified goal. But if a part of the PR is not related to that goal, that process becomes extremely confusing. In the end, this will result in the reviewer either spending more time than necessary on understanding the PR, requiring more back-and-forts because of the unrelated changes, or not understanding the PR at all.
Letting The Reviewer Figure Out Everything Themselves
Smaller PRs have less code than larger ones which means that they’re easier to review, that’s a fact that we can’t get around. Unfortunately, not every PR can only touch 2 files and change 20 lines of code. There will be bigger PRs that will either cover a lot of different files, hundred to thousands of lines of code, or a mix of these. Naturally, these are harder to review.
But despite bigger PRs being harder to review, it’s not that we as code proposers can’t contribute towards making them easier to review. Even the slightest push in the right direction, some small instructions, or any comment from the code proposer would make a significant difference. Unfortunately, it’s something that’s frequently neglected, yielding all the consequences for the reviewers.
As code proposers, one of the worse things we can do to our reviewers is to provide them with a PR to review and let them figure out everything by themselves. Although the impact in smaller PRs is less noticeable, the effect is still present in PRs of all sizes. Not only does this create an unstructured and difficult situation for the code reviewer, but it also requires them to start from scratch. They have to actively go out of their way to understand your work without proper knowledge, go through it multiple times to figure out the best approach, and thus spend more time and effort than could’ve been necessary.
Have Meaningless Version Control History
The version control history can be one of the most impactful aspects of a PR. A properly maintained version control history can have a significant impact on how difficult the PR is to review. Especially in larger PRs, breaking up chunks of related code into individual commits can make the lives of code reviewers significantly easier. The more organised the history, the easier becomes for reviewers to understand and get through the code.
Unfortunately, I’ve experienced a lot of PRs of which the version control history was basically meaningless. Either it had one big commit, too many commits, too small, or commit messages were not descriptive enough. Especially when the life span of the PR exceeds multiple feedback rounds, but also in general, having proper version control history can make a world of difference.
There are a lot of aspects to maintaining proper version control history and can be a skill on its own. Examples of factors to include are the size of the commits (not too big, not too small), the number of commits (not too many, not too few), the commit messages, how to split the commits, and much more. It takes a lot of time and effort to do it well. But when you do it well, the effect can be felt tremendously by your code reviewers.
Rewriting Version Control History On Every Change
Building on top of the importance of version control history, there is also the stability of it. As the life span of a PR increases, so does the effect of properly maintained version control history on how difficult it’s to review. As PRs live for a longer time, the chances become increasingly higher that the code reviewer has to go through the PR several times. When reviewing multiple times, it’s rarely the case that the reviewer wants to go through all of the code on every review.
Rather, it’s common to resort to incremental reviews based on the added changes since the previous review. However, to do so the reviewer is dependent on the stability of the version control history. When reviewing subsequently, it’s necessary to know what part of the PR has stayed the same since their previous commit and what has changed.
Unfortunately, this is something that I’ve done a lot in the past. Every time I would have to process feedback or put additional changes on top of the existing PR, I would also conveniently rewrite the version control history. Either it was to amend the changes to a particular commit or to rebase the branch because it was convenient. The issue is that this obstructs the stability of the version control history and actively hinders how easy my reviewers could get through my PR.
Final Thoughts
There are two parties to a reviewing process, the code proposer and the reviewer. As code proposers, we want to pass all the checkmarks of the reviewer as easily as possible. To do so, we oftentimes focus on improving the code that we’re proposing in terms of quality, readability, and so on. But in reality, there’s more to a PR than just the code. Even if the code quality is perfect, the PR can still be difficult to review.
This article covered 5 non-code related mistakes that developer make that leads to their PRs becoming more difficult to review. These mistakes are leaving out contextual information, including unrelated changes that are irrelevant, letting the reviewer figure out everything by themselves without guidance, having meaningless version control history, and rewriting version control history on every change that you make to the PR.
A PR that is hard to review will take more time, more feedback, and more back-and-forths. You might think that it won’t affect you. But this means that your work will stall in the reviewing process even longer and it’ll take even more time before it gets into production. In the meanwhile, both your colleagues and you have to spend more time and effort on it.
In the end, it’s the entire engineering team that has to suffer.
If you liked this article, consider checking out the other entries in the Uncommon React newsletter or my Twitter for future updates. If you like my content, consider sponsoring me with a coffee to keep me going.
Top comments (0)