Peer Code Review | Set Up Your Reviewer For Success

Intended Audience: software developers

TL;DR To set my reviewer up for success, I submit high-quality PRs that are small, focused, and well-documented. This approach not only streamlines the review process but also encourages better feedback and collaboration, ultimately improving the quality of the codebase. In this post, I share five key techniques that help make my PRs a pleasure to review.

Photo by Gabriella Clare Marino on Unsplash

1: Keep PRs small

Keeping PRs small is vital to setting up my reviewer for success. I know this from reviewing other developers’ code. After reading through several hundred lines of changes, my level of scrutiny decreases significantly. I start to skim, and as a result, I start to miss things. Either that, or I have to put the PR aside and pick it up the next day after giving my brain a break. 

So, I keep PRs as small as possible. Most of the time, I do this by breaking apart changes into independent pieces. Any code changes that can stand alone and still be compiled go into their own PR. Beyond making life easier for my reviewer, there are several advantages to multiple small PRs over one extensive PR:

• Parallel Reviews: Different team members can review multiple small PRs concurrently, speeding up the overall process.

• Diverse Perspectives: With multiple small PRs, different reviewers will likely examine each one. This diversity of viewpoints often leads to higher-quality code.

• Quick Turnaround: Small PRs don’t wait as long for review. Developers are more inclined to pick up a small, quick-to-review PR than a large, daunting one.

Exceptions to the Rule

While keeping PRs small is generally best practice, there are some things I never split into a separate PR. For example:

• Tests: Tests should always be in the same PR as the code changes they test.

• Error Handling: I avoid leaving error handling as a “TODO” to be addressed in a subsequent PR. 

• User analytics: I never want to release features without analytics. This equates to “flying blind.” Adding analytics in a separate PR risks exactly that.

• Accessibility: I include accessibility features in the same PR to guarantee that, from the start, everyone can use my code.

My rule of thumb is this: if the code shouldn't be released to users without it (e.g., tests, error handling, user analytics, accessibility), then it shouldn’t be in a separate PR—splitting it off risks precisely that. 

Dealing with Large PRs

While most large PRs can and should be broken down into smaller, more manageable parts, there are rare situations where this isn’t feasible. For example, upgrading an iOS project to a new version of Xcode may necessitate changes across the entire codebase due to updates in the iOS SDK, such as the deprecation of methods that must be replaced throughout the project.

I do everything possible to minimise the PR’s size in these cases. I apply all the techniques discussed in this post to make the review process as straightforward as possible. When a large PR is unavoidable, I go the extra mile to ensure clarity and simplicity.

I use my commit history to assist my reviewer further in this situation. Consider the transition from iOS 9 to iOS 10, which required significant changes. For instance, UIAlertView was deprecated in favour of UIAlertController, and NSURLConnection.sendAsynchronousRequest was replaced by URLSession.dataTask.

These changes had to be implemented in the same PR to ensure the code would compile. To keep the review process manageable, I would have used a separate commit for each set of changes. I would dedicate one commit solely to replacing UIAlertView with UIAlertController. In a separate commit, I would focus exclusively on transitioning from NSURLConnection to URLSession. I would keep each commit clean, minimal, and focused, allowing my reviewer to evaluate the changes commit-by-commit. This approach can also enable multiple reviewers to tackle different parts of the PR, further streamlining the process.

2: Keep PRs laser-focused

PRs should be laser-focused on the task at hand. My reviewer has limited bandwidth, so it’s important not to clutter my PR with unrelated changes like typo corrections or formatting adjustments. While fixing typos and cleaning up code are valuable tasks, they belong in separate PRs to avoid distracting from the primary purpose of the current review.

To keep my PRs focused, I remove any changes not directly related to the issue described in the ticket. Here are some examples of changes that warrant their own PRs:

• Typo fixes

• Formatting fixes

• Improvements to code comments

• Updates to deprecated methods

• Rewriting code to make it easier to debug

• Rewriting code to make it easier to read

The Risks of Including Unrelated Changes in a PR

Combining unrelated changes into a single PR can lead to several unexpected negative consequences:

• Merge Conflicts: Even something as simple as a typo fix can cause a merge conflict, which opens the door for mistakes. I don’t want to waste time resolving conflicts that have nothing to do with the main purpose of my PR.

• Delays Due to Unrelated Discussions: If a seemingly minor change, like a typo fix, sparks a debate (for instance, whether to use “color” or “colour”), that discussion could delay the merge of my entire PR. This is unnecessary and counterproductive.

• Rollback Complications: If my PR needs to be rolled back due to an issue, I don’t want unrelated improvements (like typo fixes) to be reverted along with it. Keeping changes separate ensures that only the problematic code is rolled back while other beneficial changes remain intact.

My rule of thumb is this: if the PR has to be rolled back, would I want that unrelated change to be rolled back with it? The answer is almost always no.

Concerns About Slow CI Pipelines

Some developers argue, “I understand the benefits of laser-focused PRs, but our CI pipeline is slow, so separating refactors and small fixes into their own PRs means a lot of waiting.” While this is a valid concern, consider the alternative: Imagine how long it will take to untangle a large PR when something goes wrong. And worse, imagine if whatever went wrong is caused by one of the unrelated fixes tacked onto the PR!

Although it may feel time-consuming now, cutting corners can create bigger problems down the road, exacerbating issues when things don’t go as planned. I’ve learned this the hard way. The real solution is to improve the team’s processes and tooling to make them more efficient rather than bundling unrelated changes into a single PR.

3: Review my own code

Before submitting my code for peer review, I review it multiple times myself. I consider the code ready for a second pair of eyes only when I can find nothing to improve on. Creating a personal checklist has been incredibly helpful for catching issues I frequently miss.

You might wonder, “Why review my code if someone else will review it anyway?” The answer is simple: “The quality of the output of any step is limited by the quality of its inputs.” Karl Wiegers said that in his 2002 book, Peer Reviews in Software. The better my PR is when I submit it, the better feedback I’ll receive in return. I always take a few minutes to fix typos, correct formatting, and remove unnecessary whitespace before submitting my code for peer review.

Who Cares About a Few Typos and Formatting Errors?

Some might argue, “Who cares about a few typos or formatting errors? The reviewer should be focusing on the real issues!” My response: Are you planning to fix those typos and incorrect formatting at some point? If yes, why not do it now? If not, why not?

For many reviewers, cosmetic issues like typos and inconsistent formatting are distracting. They can make it harder to spot actual bugs. Let's say I’ve written a new function and added a set of twelve unit tests for the function. And let's say that I haven't been very consistent with the names of the unit tests. The inconsistent naming can make it challenging for the reviewer to catch missing test cases. I want to make it easy for the reviewer to catch my mistakes--not hard.

Sloppy code can obscure deeper issues. If the code isn't clean, it’s hard to focus on what lies beneath. Ultimately, it’s my responsibility as the author to clean it up before submitting it for review. I won't waste my breath arguing for the right to submit sloppy code for peer review. Code review is about finding issues I can't spot alone, not catching the ones I could have addressed myself.

Photo by Tristan Ng on Unsplash

4: Highlight workarounds and hacks

Meet Lily. Lily works in a tiny, charming yarn shop in London. The creaky floorboards and narrow staircase add to its character, but one day, an elderly woman with a cane trips on an uneven floorboard while browsing. Lily's heart stops, but luckily, the woman catches herself. Relieved, Lily realises how dangerous that floorboard is and knows she has to do something.

Her first thought is to grab something noticeable—maybe some bright orange tape to mark the uneven floorboard. But she hesitates. The store's owner is passionate about keeping the interior photo-ready for Instagram since it's a famous spot for knitters. Bright orange tape would ruin the store's aesthetic.

Lily decides to move the "new yarns" display over the floorboard to prevent further accidents while keeping the store looking beautiful.

The following day, her colleague Jess arrives to open the store. Unable to guess why the new yarns display has been moved, Jess moves it back to its original spot. Later that day, another customer stumbles over the same floorboard.

Just as Lily chose to mask the uneven floorboard to keep the store looking beautiful, developers often disguise workarounds or hacks to avoid disrupting their code’s appearance. They may use clever methods to make these solutions seem like "clean code," adhering to best practices and conventions because they are taught that workarounds and hacks are “bad.” 

However, just as Lily failed to leave any clues explaining the new placement of the yarn display, camouflaging workarounds in code can lead to bigger issues. Here’s why I flag problematic code clearly—like putting out orange traffic cones or wet floor signs:

• Visibility for Reviewers: If I don't clearly mark my workaround, the reviewer might overlook the underlying issue. Without understanding the context, they may not scrutinise the solution adequately, missing potential problems.

• Future Developers' Awareness: Future developers who come across the code might inadvertently remove or modify the workaround without realising its purpose (as Jess did with the new yarns display). This can lead to bugs, security vulnerabilities, or other issues.

In software development, it's essential to be transparent about workarounds. Lily didn’t have the option of immediately fixing the uneven floorboard. However, she could have left a note for Jess explaining why she moved the “new yarns” display. Developers need to ensure their solutions are well-documented and flagged so that their code remains safe and functional in the long run.

5: Utilise the PR description

Far too often, PR descriptions are left blank or contain only minimal information, missing an opportunity to provide valuable context to your reviewer. A well-crafted PR description can make a world of difference in the review process, transforming a potentially confusing task into a clear and efficient one. Here are some key elements I like to include in a PR description:

Screenshots: A picture is worth a thousand words, and annotated screenshots are worth even more. Screenshots, especially "before" and "after" comparisons, provide a quick and effective way to illustrate changes. I use them to guide my reviewer’s focus, showing exactly what they should look for.

Highlight Areas for Extra Scrutiny: If there are specific parts of my code that I’m unsure about or that are particularly complex, I call them out in the PR description. I let my reviewer know where to focus their attention so they can provide the detailed feedback I need.

Include QA Instructions: My reviewer and my future self will thank me for including detailed QA instructions. If special conditions are required to test the code—flags that need to be toggled, a particular user account, or a test payment method—I include that information in the PR description. I don’t want my reviewer to waste time searching for details I’ve already gathered. Efficiency is key, and as an engineer, avoiding duplicated effort is a priority.

Clarify Confusing Acceptance Criteria: If the acceptance criteria for the task were unclear or required extra effort to understand, I share that clarity with my reviewer. I don’t make them go through the same process of deciphering requirements that I did.

Discuss Alternative Approaches: If I considered different approaches before settling on the one I implemented, I briefly explain why I chose the solution I did. This can provide valuable context for my reviewer and help them understand my thought process.

List Related Tickets: If I created any additional tickets during the development of this PR, such as FIXME notes, TODOs, or related refactors, I include them in the PR description. This helps maintain a record of ongoing work and ensures that nothing falls through the cracks.

In Conclusion

Building a reputation as a developer whose PRs are a pleasure to review can significantly impact your experience as a developer. When others see the care and effort I put into preparing my code, they’re more willing to review my work. Additionally, this approach has inspired some of my peers to adopt similar practices, fostering a more efficient and collaborative development environment.