More than an approval
Pull requests are often seen by their functional requirement: merging new code. That is the baseline — the most basic function. A good (humanly conducted) code review is critical for many reasons that other blog posts expand on more: security reviews, quality guards, and code guidelines streamlining. These are all important when conducting a code review on a PR, but even that is often forgotten. Sometimes, a PR is approved without a code review which makes you wonder why we have PRs in the first place. The reason for this quick ‘solution’ is probably time, budget, and maybe motivation. What we must understand is that new code that passes the PR code review becomes collective code which means that all members of the team are now responsible for it. Merged code that is not reviewed is, therefore, a hotbed for bugs, security issues, breaks of trust, and lack of responsibility to the client.
Yes, it is the responsibility of the PR author to provide a clean, tested, documented solution that solves the referenced problem or feature. No, it is not the sole responsibility of the PR author to spot every issue or human error by themselves.
Ask questions
Pull requests are more than approval. They are places where people can come together and learn from each other. It is a misconception that only ‘seniors’ can code review or that ‘juniors’ cannot do a code review. Yes, there should probably be someone else besides a ‘junior’ to do a check, but that should also be the case for the ‘senior’. This is more about how authors look over their own mistakes, regardless of their experience level.
Ask questions about things that are new to you, things that inspire you, or make you wonder how they work. Juniors can learn from certain frequently-used practices, seniors can learn from a new kind of syntax, and so on. A PR is a perfect opportunity to see how the other person is working. Personally, I even try to follow the thought pattern of the PR author. I’m curious about how people’s minds work and how this differs from my own. That is why I ask questions — not only because I question a certain piece of code, but because there are more things that I do not know than I do.
Compliment at least once
A thorough code review is often never appreciated. Maybe this is because it seems to conflict with the basic PR: approve/reject option. There is no ‘fantastic’, ‘awesome’ or even a ‘great job’ option, and can become pretty toxic and overwhelming, especially if there are a lot of ‘negative’ comments on things that need to be changed. What both parties have to understand is that we still work with humans. There is ego, pride, vulnerability, and uncertainty at play. If we treat the author like a piece of code that has to be improved, we’ll miss the mark.
If you have something that needs changing, approach the problem carefully. Explain why it needs fixing, how it can be fixed, and what your understanding is of the problem. Even more importantly, find at least one piece of code to compliment. This could be a descriptive error message, a well-formatted function, a defensive code structure, or even a variable name. As well as the code that needs changing, see the code that makes you smile and compliment it.
Conclusion
The solution for a healthy workspace is so simple. But budget, time, motivation, or even just plain ignorance can be why people are afraid to ask questions and compliment each other. But both of these things are critical to a sustainable pull request code review system in a team. The effect of constant negative reviews won’t be obviously immediately, but they could build up into a toxic work environment.
Being open is an asset that can help you in many different situations. PR code reviews are no different.
Thanks for reading.
Stijn
Subscribe to our RSS feed