Reviewing pull requests in the right way
Pull request review can be seen as burdensome, and some people dread them because of unpleasant experience. It is one of the best ways to receive feedback from teammates on your work and learn from others. However, when not done correctly, it can lead to disputes and harm the team, creating a negative atmosphere. Before things go sour, you may need to align with your team as to how the review practice should be conducted.
Most software engineering teams desire to move fast and be efficient at the same time. We need measures and quality control steps that allow us to move fast with confidence. Automated tests, QA tests, pull request reviews, and pair programming are examples of these measures. They ensure that we break as few things as possible while moving fast. Pull request review provides direct feedback on your work before it becomes available to actual users.
Pull request review can be challenging for both authors and reviewers but the reasons are usually the same. The team may have never discussed or reached an agreement on how to practice the review process, or people do not understand what they should expect out of the review process. First, the review process is about raising the bar, not finger pointing or humiliating others. It is supposed to improve the quality of the work under review. Many bugs can be detected and prevented with help of fresh eyes before they slip into the production. It is also a way to learn from your colleagues and grow your team. However, before everything else, the work must be reviewable. Otherwise, reviewing itself does not yield much but leads to problems.
You need to agree and define what is reviewable or not. The size of the work must be digestible, it should not demand long and interrupted time from reviewers. There is no rule but if a review takes longer than half an hour, then it is too big. It is better to split the work into smaller prs. There should be enough context provided both in the pr description and commit messages. Especially focusing on why the change is made in a particular way and what considerations led to that choice. The work must be separated into smaller commits with smaller context so the review can review commit by commit.
The review process takes time and humans have limited attention. The reviewer should not spend time on typos, comments on coding styles. Make sure to have a linter run automatically before requesting a review so that these non-essential issues can be addressed beforehand. If the work is not complete yet, mention that it is just a draft and specify which part you want to get feedback on.
The work under review should pass all automated tests and be covered by either new or existing tests. The review may start reviewing from the tests after reading the pr and commit messages to understand the intention behind the change. They can even write more tests to cover the use cases. Then the review may move to reading the code and confirm if the intention and the work are aligned. So, tests are crucial for understanding the intention. You should structure each test in three parts: ‘given,’ ‘when,’ and ‘then’; or ‘arrange,’ ‘act,’ and ‘assert.’
The reviewer should not leave comments like ‘this is not good’ or ‘you should not do that’. Instead, the reviewer should ask questions to clarify the intention of the work. If the reviewer thinks that the code does not look good, then they should explain why and suggest alternatives. If the suggestion is about the architecture or similar concepts that may require intensive refactoring, it should be stated that the refactoring can be a follow-up work.
In short, let’s not forget both reviewers and authors are human beings. Be respectful and try to focus on helping people around you grow and unblock them. Feel free to share your perspective respectfully, keeping in mind that others also have their viewpoints and opinions. Use the review process to both learn from and teach your colleagues, fostering collaboration. When things get a little argumentative, offer a synchronous call or your help to pair on the subject. Have a good intention.