How to improve Pull requests?

You’ve finally completed your task, added the necessary unit and snapshot tests to verify your changes and be in pixel-perfect alignment with your designer’s mockups.

Well done 🎉🎉🎉 ! What’s next?

Of course code review!

A code review is a process where someone, other than the author of the code, examines the code to identify problems but also provides valuable feedback to improve software quality. Code review is of crucial importance because it helps to find bugs in early stages, enhances overall code quality, promotes collaboration and knowledge sharing among team members of the team & ultimately ensures the delivery of reliable software.

For this reason, I intend to share a range of methodologies & guidelines that we use in XM during the execution of a new Pull Request (PR) in order to optimize the reviewer’s experience to the maximum while also respecting his/her valuable time.

# Review your own code

Always start with a self-review of your own PR. The goal of this self-review is to refine and improve your code as much as possible before seeking feedback from others. One effective way to do it is to put yourself on the seat of the reviewer, this can freshen our perspective and produce new insights. Another way to refine your code is to challenge yourself by asking questions like:

• How would I improve my current implementation?
• Is it obvious what this code is supposed to be doing ?
• Does naming make sense?
• Am I missing any edge/corner cases?
• Does the code meet the team’s coding standards?
• Can this be broken down into smaller code changes?

# Keep it short

People are reluctant to review large PRs or even skip them for a long time and with a good reason. A big PR means a lot of changes and a lot of changes means spending more than expected time to review the PR. It’s unfair to give a reviewer a diff consisting of hundreds or thousands of lines of code changes. Additionally, the quality of code review reduces since the reviewer can’t focus on every detail for such large PRs. Here are some useful tips:

• Each PR must have a single goal
• Avoid adding unrelated code changes — Just open another PR 🙂
• Each PR must have have less than 200 lines of code changes
• Ensure all commits are small and also have descriptive commit messages

# Take good care of the PR description. Use a template!

Treat your PR description as an opportunity to improve the code review experience, making it more meaningful, accurate and fast. For the code reviewer and someone that will take a look at your PR in the future.

A good way to do that is to use a template for your PR with predefined sections. Here are some section examples:

## Explain what changed (How and Why)

Here you should describe with every detail the problem, the solution and the reason for doing these changes. You will save both you and the reviewer’s time by providing all these details. More specifically:

• Write a descriptive title for your PR
  – This provides clarity & improves the overall understanding of the proposed changes even without opening the PR.
  – Bonus tip: We use the JIRA task in PR’s title for easy access.

• Explain the problem you try to solve as simple as possible
  – Give examples of why this is a problem
• Propose your solution and explain why you pick this approach
  – Document any alternative approaches that you considered as bad ones
  – Document any approaches you attempted and were unsuccessful
• Add comments to facilitate the reviewer
  – Highlight important lines of code that will quickly give context to the reviewer
  – Pose questions when you are not sure about something

## Demonstrate your changes (How it looks like)

Here you should showcase your work, especially if it’s UI related. As the quote says: `A picture is worth a thousand words`, and there are many reasons to include this section in your PR template. Presenting visual evidence can significantly aid reviewers in verifying that your modifications achieve the intended results.

• Provide a link with the updated designs (e.g. Figma link)
  – This will facilitate the reviewer to compare your UI changes according to designer mockups.
• Attach images, videos or gifs for every scenario that might be affected
  – A good approach is to demonstrate the diff in a table with two columns, (Before & After) since it’s easy to see the changes side by side.

• Provide logs before & after your changes or even show network logs (e.g. proxyman, postman)

• For UI tasks compare snapshots tests with design’s mockups

## How to test your changes

The scope of this section is to provide a step-by-step guide for anyone who wants to test our changes.

If needed, start by providing the preconditions for testing this piece of code. For example:

• An account with the credentials if any
• The environment that has been tested (Prod, staging, test)
• Any additional information that is required in order to properly test it

Continue with providing guidelines for both the reviewer and the QA team.

### How a developer can test it

• Clearly provide specific steps with actions on how to test all the related scenarios.
• Encourage reviewers to examine more platform related aspects such as the view hierarchy, or memory leaks etc.
• Add custom logic for testing purposes in complicated scenarios
  – Make temporary hardcoded changes just to verify a case
  – Enable Network Link Conditioner to modify network speed
• Run the application in DEBUG mode & verify the logs

### How a QA person can test it

Although for most of the cases a QA person could apply the Dev’s steps, there are scenarios that’s not the case.

• Nothing needs to be tested by QA:
  – Assume an irrelevant change on Jenkins pipeline or on fastlane. There is no point for a QA person to test something like that.
• Special notes for QA:
  – Provide some additional information for the QA team regarding the modifications being made to the code and potential scenarios that could be affected. Be explicit in letting the QA team know that they might need to perform regression tests of a particular functionality once again and ensure its stability.

## Other platform PR

• Leave a link to the other platform’s PR (in our case Android) for someone to be able to verify that the two platform implementations are consistent.
• Avoid duplicate time on specs since one platform will analyze & clarify the specs and provide the PR and the other platform will have directly all the required changes for this feature
  – This will reduce the development time of the second team.

# Assign a reviewer(s)

Don’t forget to set a reviewer in your PR. If you forget it no one will look into your PR. Here is an approach we are currently using:

• Assign at least one reviewer
  – If it’s a feature PR, you should have an a-priori dedicated reviewer
  – If it’s an ad-hoc PR you can try to find the developer which has the most context of the code that changed. If you don’t know that, you can use the following quick and dirty git diff command to find the person with the most contribution in the changed files.

git diff --name-only {the parent commit of your branch} . | 
grep 'swift' | 
while read filename; do git blame --line-porcelain "$filename" | 
sed -nE 's/^author-mail <(.*)>/\1/p' | 
sed 's/@.*//'; done | 
sort | 
uniq -c | 
sort -rn

# Notify regarding the status of PR

• Always let your colleagues know when your PR is ready for review.
• Inform the reviewer (ideally in a dedicated Reviews channel) that your PR is ready for review, letting him also know its priority.
• Use a priority tag to inform the reviewer about the urgency of this PR

# In conclusion

Appreciate your colleagues’ time when asking for PR reviews. Being a good team player means that you want to make their job easier. Also, keep in mind that code review comments should not be taken personally. Embrace feedback as an opportunity to improve & be respectful when reviewing others’ PRs. Remember that nobody knows everything and fostering a collaborative & respectful environment benefits the entire team. To sum up, as the saying goes

It’s nice to be important but it’s more important to be nice.

Special thanks to George Sotiropoulos, Nikos Linakis and the amazing mobile team for contributing and proofreading.

References — Further reading

[1] https://artsy.github.io/blog/2020/08/11/improve-pull-requests-by-including-valuable-context/

[2] https://www.danijelavrzan.com/posts/2023/02/create-pull-request/