Reviewing Pull Requests Delightfully
How to review code effectively in a positive and helpful way
Hi friends, let’s continue our journey through the Delightful Pull Requests life cycle. In a previous article, we talked about the art of crafting a delightful Pull Request or PR. In this one, we’ll look at what happens next: reviewing the PR.
Giving feedback is hard, and providing effective feedback is a skill we ought to master. In this article, I will offer concrete advice on becoming a better code reviewer and a better code reviewee.
Why is this step necessary?
We can all relate to the importance of code review; it’s the catch game. We do it to catch bugs earlier, to catch code quality issues, to catch and cherish learning opportunities for both the author and the reviewer, and to catch up on others’ work making sure we’re all on the same page. Plus, it’s fun!
I, for instance, love the anticipation of a PR review. I make it a special occasion whenever I settle in to review my PRs. Tea when it’s chilly, Bubly when it’s warm. It’s the perfect way to set the tone for a thorough and thoughtful review process. It is exciting. The only reason I don’t get popcorn while reviewing PRs is…, who likes sticky, buttery fingers on their beautiful white keyboard? Not me.
But be aware! As with all good things, code reviews also have some downsides. It can slow down development. It can produce unnecessary arguments. The worst downside is the amplification of poor company culture, difficult personalities, and unprofessional behaviors.
Code review is complex because it’s a delightful combination of hard and soft skills.
But don’t despair. We can avoid those downsides by following some basic rules or codes of conduct.
Some Guidelines to Make the Review Process Work
First, turn-around review times
After sending your PR for review, expect the process to take a couple of days, and plan for it. It would be best if you allowed time for you and your reviewers to have proper discussions. This time will make for the review process to happen harmoniously.
Remember that this is a catch game. The more you catch here, the better it’ll be when your code is in production. You are invested in this process, don’t rush it. It’s better for the future you, the team, and the company to take the time and do it properly.
Second, only send your PR for review when it’s ready
I recently had a PR up depending on another PR. The other PR was still open, and I did a cherry-pick on a commit, which is fine. We want to reuse as much as possible instead of copy+paste and creating duplications. But my mistake was I sent my PR for a review. The reviewer came in and needed clarification after seeing those unmentioned changes in my PR and commenting on them. They were documented in another PR but not in this one.
It was my mistake. I was wasting the reviewer’s time. I can’t expect people to know and trust me when I say: this will be merged into the main branch soon; ignore those changes here. That was not cool, and I’m sorry for that. I did what I should have done from the start: leave it as a draft until it was ready for review.
No biases
Code reviews should be classless. Even the most senior person on the team doesn’t produce perfect code. It’s better to make no assumptions. The same applies to junior members of the team.
Tips to provide feedback, not just criticism
One tip is to frame feedback as a request or question. For example:
- “Rename this variable” → “Could you rename this variable (to blah)?”, or
- “Move this method to another class” → “Should this method be moved to another class?”, or
- “Break up this function into two smaller ones” → “Consider breaking up this function into two smaller ones.”
Another tip is never to say “you.” There are exceptions, of course. You can use it in questions like: “Did you mean…?” or “Can you please…?” but avoid it in most other situations because it associates the person to the work, and then the “ego” comes alive. Try replacing “you” with “we, me, code.” For example:
- Me: “Your code is unintelligible” → “I am having trouble understanding the code.”
- We: “Can you refactor this duplication?” → “Can we avoid duplication here?”
- Code: “You need to write unit tests for this code” → “This code needs to be covered by unit tests.”
And last, explain things upfront when giving feedback, to prevent requests for clarification.
As the reviewer
As the reviewer, remember that a problem almost always has multiple solutions. Some will be better, but there will be more than one good solution. You need to distinguish between common practices and your personal taste. And don’t force your solution on the author if theirs is also acceptable.
Offer sincere praise. It reinforces good practices. When work exceeds expectations, in the presence of high-quality code, when a new team member picks up quickly, recognize and praise.
Provide code examples. Code can, in most cases, speak better than words. And as a positive effect, if your suggestion is good enough, it might become part of the code base.
Use known labels like nit or nitpick. Nitpicking refers to giving too much attention to unimportant details. Let the reviewee know when some of your comments are non-blocking nitpicks or if they are blocking and must change.
Review atomically. Raise all the issues you find at once, and keep the number of review iterations low, as going back and forth multiple times increases the stress levels and frustration for the reviewee.
Don’t let PRs hang around for long. Finish what you started, and do it promptly.
As the reviewee
If a reviewer asks you some questions about the PR, please answer them in kind. If they asked numbered questions, answer them in numbered matching answers. The more they understand, the better they can make a decision.
Be diligent and understanding about answering questions. Do not assume everybody knows what you know.
Redirecting the reviewer to the RFC document is not a satisfying answer. You’re setting up the reviewer for extra time reading a lengthy document you already (should) know and can quickly summarize in one sentence or two. It is your responsibility to know your stuff and to be able to explain it in plain English to reviewers.
If your PR is still a WIP (work in progress), keep it as a draft, save the reviewers some time and trouble, and don’t send it for review. Most people have it configured, so code owners will get notifications when you set it as ready for review (in GitHub). If you did this by mistake, you can always change it back to draft so people know this work isn’t ready yet. Stating WIP in the title or the description is not the best way to tell. Reviewers will get notifications depending on the status, not the title.
Generally, always try to have two reviewers: one that understands the business logic and another that will check the code quality.
And remember, it’s your responsibility to set yourself up for success with your reviewers. Doing this ensures your PR is not the last one reviewed.
Difficult situations
Take things offline. Whenever there is a back-and-forth discussion on the PR, take it offline and discuss it with the person face-to-face, over chat, or on a call. Update the discussion in the PR and close it once there is an agreement.
When facing difficult personalities, in situations with difficult reviewees making the same mistakes over and over, or unwilling to implement suggestions, or difficult reviewers being gatekeepers, insistent, overly critical, and blocking PRs; it decreases team morale. In such cases, the highest possible code quality is not worth it.
Hopefully, you won’t find yourself in such situations, but if you do, a reasonable solution might be to discuss 1-on-1 to understand the other person and find common ground. If you can’t find an agreement, seek an outside opinion, ask multiple people and go with the solution the majority chose. If all else fails, escalate to a senior and let them handle it. If it works out in your favor, great. If not, accept it and move on.
The point of PR reviews is not to block code until it is perfect. The purpose is to have good enough code merged in a timely manner.
The End
Alas, we have reached the end. Welcome to the Council of Wise Reviewers. That was a lot, and hopefully a helpful lot.
And as I said before, code reviews are the perfect learning opportunity for us all. Please take advantage of it, and have fun while at it!
Happy reviewing!
