Reviewing a Pull Request

Being asked to review a pull request can be a daunting task, but it is a very important one. The goal is to assert that the change does what it sets out to do and in a logical way.

Here we will go over some tips and best practices you can follow when reviewing a pull request.

Understand what you’re reviewing

In order to efficiently review a pull request, you need to fully understand what you are being asked to review. Read any information provided in the pull request description thoroughly. If there is an issue tracking number attached to the pull requests, open that issue and read what its all about, so you can get a complete understanding of the issue at hand.

If you don't understand the change that you are reviewing, then you cannot approve it. Doing so could lead to unsatisfactory code entering production. Instead, leave a comment on the pull request asking for clarification or talk directly with the author.

Take your time

This is the last chance to prevent erroneous code from entering production. Thus, it is important that you read every line of code that has changed. It is far better to spend an hour or more reviewing the change than letting a bug enter production. It would likely take you and/or other project members much longer to debug and resolve the bug afterwards. On top of that, you will have potentially released a buggy application to your users.

Check the commit messages and branch name

Having good commit messages and branch names makes it much easier to find where changes happened without having to dig into the code itself. This can be very helpful in the future for finding when a particular change in the project occurred.

Check that the commit messages follow the team standards and reference any relevant issue numbers. The same applies for the branch name.

Check the acceptance criteria

Acceptance criteria are there for a reason and so you must ensure that the author has met all the criteria set out. If they haven't, then the pull request is not finished. You should leave a comment informing the author of the missing acceptance criteria.

If any missing criteria "will be addressed in a separate pull request", make sure that the author has created a relevant issue for this and left a TODO comment in the codebase that links to said issue.

Check for any out of scope work

It is often very tempting for a pull request author to sneak in out of scope work. Whilst it may seem like a quick win, it can actually distract from the acceptance criteria of the story. Also, there will be no reference to this change in your issue tracker.

This goes both ways though. A reviewer should not request out of scope work to be fixed in the current pull request. In both cases, create an issue and separate pull request for the out of scope work.

Code readability

The code contained within the pull request will eventually be merged into the codebase. It is therefore important that the code is readable. This sometimes means opting to write more lines of code, rather than shorter and more complex code.

Some topics to keep in mind when checking for code readability are:

• Large functions
• A large number of nested conditions
• Magic numbers
• Ambiguous variable and/or function names
• Unused code

Clean code

Just as the readability of the code is important, so is the fact that it's kept clean. Even if using linters, some styling issues can still sneak through. Thus, you must keep an eye out for them when reviewing code changes.

Some topics to keep in mind when checking for clean code are:

• Correct indentations
• Typos in comments, variable names and function names
• Incorrect grammar in comments
• Duplicate code/functionality
• Duplicate or overly large assets
• Unused code is removed

Shorter code

Whilst keeping code readability in mind, you should keep an eye out for code which could be shorter whilst still being easy to read and understand.

Things to look out for include:

• Functions with duplicated code
• Is there a shorthand syntax available?
• Returning a boolean rather than a condition

Optimal solution

It is very important to confirm that the solution is optimal. You want to ensure that build and/or execution times do not deteriorate, as this will slow down the development process. Non-optimal solutions can also have a negative effect on the user experience of the app.

Tests

If the project makes use of tests, and it should, it is important that the author has included them in the pull request. Unit tests are another tool that helps prevent bugs from entering production, so it is a must that you review these tests with the same attention to detail as the code itself.

Other test types such as UI and snapshot tests (if used) should also be included.

Other project standards are met

Every project is unique and will have its own set of standards. These should be set out and clear for everyone working on the project. Reviewers should assert that all these standards are met.

Examples of project standards include:

• Documenting code
• File headers
• Project structure
• Variable and function naming
• Anything else your project team desires

You should now have a better understanding of how to go about reviewing pull requests. The more pull requests you review, the more natural this will all come to you.

To get some more tips that can be used by both authors and reviewers, read our Pull Request Tips and Customs post.