PR Review Best Practices

TL;DR

Good PR reviews are essential to a high-quality codebase. They are also great opportunities for learning. Make your reviews a top priority!

Why did you write about this topic, Kev?

Because I am in love with PR reviews.

I enjoy reviewing other’s code because:

• I see different ways to solve problems and often learn best practices.
• I strengthen my software engineering muscles for reading code.
• I get to express my opinions and offer feedback.

I enjoy when others thoroughly review my own code because:

• The reviewer often catches bugs in my code.
• The reviewer suggests cleaner ways to refactor the code.
• The reviewer points out what they like in my code –this makes me feel all warm inside .

We all come from different backgrounds, skill levels, and languages (spoken languages and programming languages), and PR reviews are our chance to learn and grow from each other while simultaneously increasing the quality of our code and decreasing the amount of bugs.

Frequently asked questions (FAQs) from PR review skeptics

1. Question:
   Shouldn’t we just trust each others code and blindly approve everything?
   Answer:
   No, we all make mistakes. It’s not that we don’t trust each other, it’s that we care enough for one another to review each others work and improve quality. High-quality code requires at least two sets of eyes ( ).
2. Question:
   Isn’t code review just another form of trolling done by crusty old developers for the sole purpose of making my life harder?
   Answer:
   Sometimes it can feel that way, but no, that’s not the purpose of a good PR review. The purpose is to improve the quality of the code, catch bugs BEFORE they are deployed to production, and to help facilitate teaching and learning amongst the team.
3. Question:
   I’m a senior+ level engineer, I write perfect code. Shouldn’t I get special permission to merge my PR without a review?
   Answer:
   No. Even if your code is perfect (which it’s not), it still benefits your team to have someone review your changes if for nothing else other than to knowledge transfer and to teach best practices.
4. Question:
   I’m a junior developer still learning the basics, it would be a bad thing for me to review someone else’s code, right?
   Answer:
   Wrong. It’s a good thing! If you don’t have much feedback to give then focus on learning. Ask questions like “why did you do it this way?” and “how does this work?”. Sometimes asking questions about complicated pieces of code can help the author realize a cleaner way to refactor it.

“Good code reviews use the same quality bar and approach for everyone, regardless of their job title, level, or when they joined the company.”

-Source: PR/Code Review Best Practices

Best Practices

Now that we’re all onboard with making the most out of our PR reviews, let’s talk about some best practices for both the author and the reviewer of a PR.

Expectations for the PR author

#1 The author has reviewed their own code

This means they’ve thoroughly looked over every line of the diff and they like what they see. It’s amazing how many small things you’ll catch when you do this –thus saving the reviewer a lot of time.

If you have a little extra time, annotate your own PR with comments. Point out the big changes and the stuff you want feedback on. Start the discussion around your code!

#2 The code has been run locally and thoroughly tested –it works!

This is usually a pretty obvious step when making sure the happy path works as expected. But also make sure to test the not-so-happy path. What happens when the API you’re using returns a 500? How should the code handle that?

#3 Merge conflicts are resolved

Yeah, resolve those. If it gets complicated, resolve the conflicts with the other developer who made the overlapping changes.

#4 Old unit tests pass and new ones (when applicable) have been written

It’s a good idea to run the suite of unit tests related to the code you’ve changed to make sure everything still passes ( ). Your CI pipeline probably has a check for this, but checking ahead of time will save you time.

And if you’ve created new classes and methods, make sure the business logic is covered with new tests.

#5 Be humble; Expect to iterate on feedback given from your peers

If you don’t understand a reviewer’s comment, ask clarifying questions.

If you understand the reviewer’s comment but disagree with them, then try your best to articulate why you wrote the code the way you did. Start a collaborative discussion. For example:

“I went with X because of [these pros/cons] with [these tradeoffs]. My understanding is that using Y would be worse because of [these reasons]. Are you suggesting that Y better serves the original tradeoffs, that we should weigh the tradeoffs differently, or something else?”

More material on this topic: Google’s engineering practices: How to handle reviewer comments

Side Note: a few small PRs are easier to review than one big PR. Even if you aren’t quite finished and the PR is not mergable, you can still get feedback. It’s often a good idea to start getting feedback sooner with small incremental changes.

Expectations for the PR reviewer

#1 Be thorough

Take your time. Do the following:

• Understand the context behind the code change. Every PR has a link to a Jira ticket for a reason. Read it! And if needed, jump into a quick huddle with the author to ask high-level overview questions.
• Think about the big picture. Did they cover the happy path and the not-so-happy path? How could this break? Are we logging potential errors effectively?
• Look at every line of the diff. Don’t skip anything unless there’s a time-crunch.
• If you have time, pull down the branch and test it locally on your machine.

In the general case, look at every line of code that you have been assigned to review.

Source: Google’s engineering practices: What to look for in a code review

#2 Be nice

Write at least one positive comment if not many. Don’t suppress your positive thoughts even tho you’re mostly trying to look for ways the code can be improved.

#3 Offer constructive feedback

Give your honest opinion about the code you read. If you have suggestions for improvement, comment about it. If you see a bug, point it out. Most likely the author will be grateful for your suggestions.

Try not to focus on the small, unimportant stuff like syntax preferences. And if you choose to comment about syntax, make sure the author knows your suggestion is optional.

Reviewers should always feel free to leave comments expressing that something could be better, but if it’s not very important, prefix it with something like “Nit: “ to let the author know that it’s just a point of polish that they could choose to ignore.

Source: Google’s engineering practices: The Standard of Code Review

#4 Ask questions if something is unclear

Clean code is easy to understand code. If you don’t understand what something is doing then there’s a good chance the code needs to be refactored.

“Any fool can write code that a computer can understand. Good programmers write code that humans can understand.”

-Martin Fowler

There’s a lot that goes into making code “clean“ or “easily-understood“ by humans. Such as:

• Class, function, and variable naming
• Small functions that only do one thing
• Limiting nested if statements or loops within loops
• Clarifying comments from the developer when the code is weird, non-standard, or has complex syntax
• Etc

At the end of the day, the only true measurement of code quality is how many WTF’s per minute you experience while reading the code; Or at least that’s what this surprisingly accurate comic is telling us :

#5 Suggest unit tests to be written

Usually the codebase has a unit test coverage threshold to meet, and that encourages/forces us to write unit tests for our code. However, as a reviewer, it’s still a good idea to keep an eye out for areas that would benefit greatly from a unit test or two. The focus should be on business logic.

Also, bug fixes should usually be coupled with regression tests to make sure the same bug doesn’t happen twice. If you are reviewing someone’s bug-fixing PR, then make sure the original problem is covered by a unit test. If it’s not, suggest to add one.

#6 Communicate quicker

PR comments are great, but sometimes a comment isn’t enough. If your comments are getting wordy, or you and the author are going back and forth trying to understand each other, discuss your feedback over a quick call instead. This will ensure your feedback is not misinterpreted.

Conclusion

PR reviews are the gateway to high-quality code. Let’s do our part to improve each and every codebase we touch by being fully committed to both receiving and giving a thorough code review.

Remember, you and your coworkers are on the same team and are working towards the same goal (click here to see the goal).