Pull request reviews are an essential tool to improve the quality of the software a team produces. In a team working with or towards continuous deployment it is the last human quality gate on the way to production. And not only that, when done right, pull request reviews are also a great tool for knowledge sharing in a team. So we better get this right. Unfortunately, this is easier said than done.
In this post we will look at some learnings about pull request reviews I collected in my daily work.
You are going too fast
The authors of "Accelerate" propose to measure the deployment frequency of a team as one of the key metrics for a team's performance. At the same time they suggest "change failure rate" as another metric to balance out the first one. Going too fast will do more harm than good. This also applies to pull request reviews. A decent review takes time. Rushing reviews might increase throughput now but it will most likely introduce quality problems and slow you down in future. Also chances for knowledge sharing and sharing responsibility in a team will be missed. So a detailed review is most likely well spent time.
Trust, but verify
First of all, trust between the members is a really good thing. It is one of the pillars of successful teams. But as often in life, you can go too far. I have seen reviews for pull requests with boring and repetitive changes over dozens of files being approved in 2 minutes. What can possibly be wrong in such a simple change? The change was also boring for the author. So chances are that one or the other thing slipped through, better verify.
I have observed that a lot of trust in the skills of your co-workers can lead to increasingly sloppy pull request reviews. Let's work against this reflex! You have smart colleagues - this is great - but everybody makes mistakes once in a while.
Start showing love to your test code
Most reviewers focus on the production code. This is understandable because this is where the bug could be hidden that might trigger that pager duty alert in the middle of the night. But still, we produce at least as much test code as production code. I have heard of projects containing three times more test code than production code. And also test code can become an unmaintainable mess. So we better show some love to our test code as well. A co-worker of mine has the habit to start having a look at the tests when doing pull request reviews. I think this is a great idea that is worth giving it a try.
Search for the missing bits
This one is really hard to get right. When reviewing a pull request you can easily see what is included. But in a pull request review it is also very important to find things that are missing. According to the excellent post by Scott Nonnenberg about the "Top ten pull request review mistakes" we should focus on the things that are not in the pull request but should be. Did the author forget to change a piece of code, are tests missing, or are edge cases not considered? So looking at a pull request is not enough. We also need to understand the code around it. We need to look at the broader picture, understand the change and the motivation behind it completely and actively ask ourselves the question: What is missing?
I tend to look at bigger pull requests in the IDE to get the complete picture. This way I can not just look at the changes but also at the code that is around these changes.
Attack the flaw, not the person
Pull request reviews are a form of feedback and we should be careful about how to get our message across. Especially new members of the team might take pull request feedback personally and feel bad about themselves. This must not happen. It is about shipping code that is in good shape, it is about learning and growing together, about sharing responsibility and not about blaming or showing superiority. So here are a few tips:
find an encouraging tone (full sentences do not hurt)
offer help, be respectful
do not just look for the flaws, praise the parts that are really good as well
I can recommend the talk "Amazing Code Reviews: Creating a Superhero Collective" from Alejandro Lujan for more information about this aspect of code reviews.
Automate the nitpicking
Many of us appreciate a consistently formatted code base. But we should not waste our time checking indentation or ordering of imports in a pull request review. Let a linting tool do this boring work and focus on the harder parts.
Unblock
It is always a tough decision if we should focus on the current piece of work we are trying to get done or if it would be advisable to switch contexts and review the pull request a co-worker just created. In general we should remind ourselves that a pending pull request is probably blocking a co-worker from getting work done. Also postponing a pull request for a longer time will result in additional effort in resolving conflicts. So a timely review is very valuable. But on the other side the flood of pull request that a bigger team produces can be overwhelming and it can also be OK to finish up the current piece of work before diving into the pull request reviews.
There are many aspects to pull request reviews and the ones mentioned above are just a few of those. It becomes apparent that a good pull request review culture is hard to achieve. I hope the thoughts above can be a trigger to revisit this important tool and get more out of your pull request reviews. It is worth it.
Top comments (0)