What to look for in a Pull Request review

Preface

We are currently integrating several development teams from around the world for our current client; one from the Ukraine and another from Columbia. Overall the process has been smooth, but there have been a rough spots. Most of these rough spots have pointed to processes that we — IcarusWorks — have not defined and documented clearly. Much of the knowledge from which we base our work is transferred in-person across the desk, and working with teams around the world has quickly pointed out these holes in our processes.

On Pull Requests

One of the processes that has lacked definition, and the focus of this post, is the process for both giving and receiving reviews on GitHub Pull Requests. I’ll be focusing specifically on the thinking that goes into a Pull Request (PR) review, and likewise what I hope reviewers of my code are going through when they review my code.

Questions to ask in a Pull Request review

Here are the questions I look to give or get in a code review. There are probably more good questions, but these are the ones I find most important. These questions might also help determine whom the reviewer(s) should be for each PR:

• Does it solve the problem as defined in the ticket?
• Does it fit into the overall application architecture?
• Is it well architected as a whole?
• Does it follow best practices?
• Is the syntax correct?
• Does it improve or less the maintainability of the code base?
• Is it inline with — or does it enhance — the branding and styles?

Often when I’m reviewing I usually need to read the code a few times to hold each question at the fore of my mind while I’m reading.

Reviewer traits

Another factor to take into account when giving or receiving a review is if — and how well — the reviewer is qualified for the review. For our current client, we’re using Ember.JS for the UI and Ruby on Rails for the API. Some traits we look for in reviewers that make them well suited for a review:

Both UI & API

• Have knowledge of — and an eye for — security and vulnerabilities
• Understand the overall goals of the business
• Familiarity with code styles defined for the project

API

• Understand the server architecture
• Solid Ruby knowledge

UI

• Ability to put on the end-user-hat when reviewing code and imagine what the processes the new code introduces will feel like. In short: empathize.
• Have an eye for design
• Understand the brand
• Knowledge of BEM and how to write CSS in a maintainable way for a large application
• Solid Ember.JS knowledge
• Understand UI app architecture. This ties into ember knowledge with add-ons.

Closing

The questions I’ve laid out above are straightforward, but if they’re each taken into account when giving a review, the review will be solid.

In case you’re curious, we have more articles and info on software development, who we are at IcarusWorks, and life on our publication page and website.