Pull Requests Volume 2: Giving Great Pull Request Reviews

Preface

This article acts as a compliment to Pull Requests Volume 1, which focused on writing great pull requests. Now I’m going to focus on the other side of things, how to do a great job as a reviewer for a pull request.

Like in the first article, I’m going to write this from the perspective of a reviewer on a team of developers working on an iOS app, using GitHub’s Pull Request feature. However, many of the things I’ll discuss apply equally to any kind of software and any kind of version control system. These guidelines are based off my experience working at Shopify and The New York Times.

The overarching theme behind both of these guides is to give examples to help software developers work better together. I’ve seen too many examples of ego getting in the way of quality. Software developers are professionals, but we often have difficulty with social things, especially interpersonal issues. That’s not only limited to how one developer gives criticism, it also includes how another developer takes criticism.

The important thing to realize here is when reviewing code, as in doing any professional activity involving developers, is this is not supposed to personal. It’s not about making one person feel good or another feel bad. It’s about improvement, both for developers and the software they make. Keep these things in mind when you’re acting as a reviewer, or when you’re receiving feedback on a pull request you’ve made. It’ll make you both better developers.

You are the Gatekeeper

As a code reviewer, you are acting as a Gatekeeper for you application’s codebase. No matter what you do while reviewing a pull request, the end question has to be: “Does this improve or worsen our codebase?” Your mission is to not accept the pull request until it improves the project, but how you accomplish that varies from team to team and project to project. Here are the things I think are most important to an iOS project, and most of these things will apply to any kind of project.

Read the Description

This one should be so obvious it’s almost not worth mentioning, but it’s important enough to still warrant being said: as a reviewer your first task is of course to read the description of the pull request as provided. Hopefully, the developer you’re working with wrote you a detailed one. This is the step where you become familiar with the issue that’s being solved. You might need to read up on a specific issue or story in your company’s issue tracker to do this, or it might be something so simple it was explained fully in the description itself.

Verify it

The next thing you should do as a reviewer is Verify the pull request accomplishes what it has set out to do. How you do this depends on how your team works.

In the most basic case, this means reading the source code, hopefully guided by the description provided, looking for how the code works. You’ll want to look at the code difference to see what was deleted and what was added. Here’s where you can spot any immediate issues, like an edge case clearly missed, or other common problems like incorrect use of comparison operators (I’ve been guilty of many more lesser-than-or-equal-to bugs than I’d like to admit). Reviewing pull requests requires a keen eye for things like this.

For teams who do their own QA, this is also the time where you’ll do your testing. This means checking out the code locally, running it, and following the test cases provided by the developer. In the best case, the developer has provided lots of cases for you to test against, but here’s where you might find your own too. Good things to look for here include strange input (negative numbers, letters vs numbers, accents and dìáçrïtîcs, emoji, etc.), multitouch interaction, device rotation or view resizing, device sleep states and foreground vs background issues, to name but a few.

If the project has unit tests, make sure the pull request tests all new functionality as needed, and that all the tests pass. GitHub has recently introduced a pull request feature to integrate with build servers, so the tests can even be run automatically before the pull request is merged in, but if not, you can always run them locally.

Above all, you want to make sure this code does what it intended to do and doesn’t introduce any new problems.

Code style

While you’re looking at the code, you should be checking to see if it conforms to your project’s style guide. Even if your project doesn’t have an explicit style guide, you probably have a good idea of the general app style in your head (and you should still consider creating an explicit guide).

Don’t be afraid to be diligent here, because even though any style violations may be minor, pointing them out isn’t petty. They add up. There’s nothing wrong with pointing out issues with whitespace, brace location, naming conventions, especially not when there are multiple slips. Both parties should be aware fixing these issues helps makes the code more coherent and consistent for everyone going forward.

Accessibility

Make sure the pull request includes proper accessibility for all new interface elements. This is really important to do as you build your application from the start because it can be done incrementally, and your customers will thank you for it. It’s simple enough to build the bare minimum accessibility into your app this way, but if you want to do a stellar job, consider talking to Doug about it.

Any pull request that ignores accessibility features should not be merged into your project until the omissions are fixed.

Localization

Much like accessibility, you should reject any pull request that doesn’t localize user-facing text in your application. This doesn’t mean the request has to include translations, it just means that any string added to the app for user-facing purposes should be localizable.

Even if your project does not currently offer localizations for non-English locales, every pull request should still include this, so your project can be expanded to include other locales at your whim. Pull requests not including localized strings should not be merged in until they’re fixed.

Documentation

This is one we’ve started doing recently on our team: every pull request that introduces new public API needs to have that public API documented. Since a forthcoming version of Xcode is going to include Headerdoc and Doxygen doc-generation built-in, we figured it was a great time to start writing docs formatted with them (we chose Headerdoc because it seems to be what lots of Apple’s headers are already documented with, but since both formats are supported, it matters less which you pick and more that you are consistent with your choice).

It’s senseless to include docs for every bit of code in the app, so we’ve set the bar at only methods and properties in our public interfaces. Private methods don’t make a whole lot of sense to document, generally speaking, because they’re often subject to change or are too internal to warrant the effort (although there’s really nothing wrong with documenting private methods, either).

Forcing a developer to include documentation for their API forces them to think more lucidly about what their API does, what parameters it takes, and how it works. In the course of writing documentation, I’ve realized once I “read it out loud” that I’d make code needlessly complicated, and immediately figured out a better way to write it. All this just by trying to explain in documentation what the code does.

As a reviewer, you should reject any code that doesn’t live up to this standard. As new code gets merged in to your project, more and more of it will be documented. New developers will be able to quickly learn how your API works, and new code will be written more clearly as well.

The Dings

The above list is a list of things either I’ve been dinged on before while having my pull request reviewed, or things I’ll ding other developers for before I’ll merge theirs in. It’s not a list of demerits, they’re not errors you should be ashamed of, they’re just common pain points, things that should not be merged into a project. You shouldn’t feel bad about mentioning any of these, just as you shouldn’t feel bad if you’re “caught” for one of them either. But if the need arises, don’t be afraid to attach an animated gif summing up your feelings.

You’ll be a happier and better development team for it.

Join the Discussion 👂🤔✍️

Please read the Discussion Guidelines before replying.

☑️ Email me when someone replies.

Speed of Light