When you combine that with mostly remote, written communication in a foreign language between people who might not know each other that well, who are under various types of pressure (time, quality, focus…), with various skill levels and seniority, you have a good chance of things getting out of hand.

Luckily, there are some easy things we can do to make this easier on everyone. Read on to find out what!

Sidenote: This document is not a PR review checklist. It will not tell you what to check during a review or what code to expect. I wrote this to show you HOW to communicate during reviews to make the experience more useful and pleasant for everyone, but it will not define any rules. For that, I suggest you talk to your team, your partner teams, and your stakeholders and agree on some rules with them.

The Pillars of a Good PR Experience

There are just two things that need to work well in order for the whole thing to be a success:

1. Mindset
2. Communication

Now, let’s break those down.

Mindset: PRs are not Personal

In general, it is good practice to assume everybody in a PR means well. That should be your initial starting point. The PR author is not there to deliberately push horrible code. The PR reviewer is not there to “get you” and “publicly shame you”. In the vast majority of cases, the goals of the authors and the reviewers are actually the same: to deliver good code that does what it is supposed to do.

Keeping this in mind is a very good start.

This does not mean you should not be vigilant and should not do your best to find problems as an author or reviewer. But it does mean that when first communicating, you should assume mistakes are honest, decisions had a reason, and intentions were good. It is your job to confirm this and make sure you understand the context. 

Only if facts later prove the opposite should your tone change accordingly. If there are serious concerns or doubts about the intentions or skills of a team member as a result of a PR review, those should be taken to the relevant line manager (but before you escalate, try to reach out to the other person via call / DM and get as much clarity as you can). They should not be discussed in the PR itself.

Another thing to keep in mind is that PRs are not supposed to be personal, and they are supposed to be educational for both sides. People get very emotionally invested in their work and get defensive quickly. Try to avoid this. See if there are things you can learn from the ongoing discussion and become a better engineer. Always enter PRs (both as an author and as a reviewer) with the goal of learning something new. And remember, this is not only about author-reviewer communication. Reviewers can also learn from each other by working on the same PR. So when communicating about the PR, do not just think about the author, think about the other reviewers as well – are there things you can include to help them?

Please remember:

• a team will survive a certain amount of messy code
• but a team might not survive broken human relationships

As long as the intentions are good and nothing is exploding, prioritize people over code (in other words, be nice).

Communication: Be Kind and Helpful

A colleague informed me of this beautiful and very simple approach to making PR comments much more helpful. It complements what I was trying to say below and makes it much more explicit and elegant.

As a PR Author

• Review your own PR before you submit it. You might be surprised at how many things you can find yourself.  You can even leave comments in the PR yourself for other reviewers in case you think it would provide more context.

I added this method to gather more metrics for the issue described in GRG-543. We should probably consider some more centralized auditing/logging solution in case this becomes a frequent use case, but for now, it is good as it is, I think.

• It is your responsibility to get your things reviewed. Make sure you understand how reviews are prioritized and picked up by reviewers. Do you need to ask for a review on Slack? Is there time dedicated to reviews on standups? Is there a dedicated group of reviewers? If unsure, ask the repo owners for guidance. Don’t let your PR be forgotten.

Hi everyone! I have a new PR that is a bit urgent due to our sprint ending soon. Could you please take a look? Please get in touch with me on Slack directly if this can help us get it done more quickly. Thank you!

• Provide a short PR description with some context:
  • Are you new to the project? Do you need help/guidance, or expect a lot of issues?
  • Do you have a preferred way of discussing the PR (e.g., a huddle, a DM exchange on Slack, Bitbucket comments)?
  • Is the PR work in progress?
  • Is the PR very urgent? If yes, why? Did it force you to take shortcuts?
  • Was performance a major factor? Was some code optimized for it?
  • Are there any follow-up tickets that will improve on the solution further?
  • Did the architecture, API, or something else change significantly?
  • Does it have dependencies on other work/teams?
  • Is there some relevant documentation you can link to?

Integration of the new component to display a shopping cart into the main navigation. Will be further reworked under BLAH-123 and BLERGH-456. I realize the navigation component is a bit messy, unfortunately, I did not have time to fully refactor it due to the project deadlines, so I only touched the code required for the new functionality. I will try to get back to it under BLERGH-456. Please check the Jira ticket under which this was done for complete business requirements when reviewing, they are more complex than expected, which can explain some of the possibly messy code in the addItem method. Oh and I am still a bit new to this repository, so apologies if I messed something up – please let me know if there are some guidelines/documentation that I missed!

• If you cannot implement some of the suggested changes, make sure you follow up in writing, either by explaining why the change will not be implemented or by providing links to follow-up tickets/PRs where those comments will be resolved.
  For cases where time is the limiting factor (approaching deadline, bigger suggested changes, etc.), always consider what would cost more—making the change now or postponing it (which carries its own overhead in planning, focus switching, etc.). Failure to do this can really frustrate reviewers, who dedicate time and energy to the review and then get nothing back.

Thank you! I don’t think I will be able to make these changes under this PR since they would require more time, and there is a risk we would miss the deadline, but I think your suggestions are totally valid, and I will cover them under BRUH-789 if you don’t mind.

• Always indicate if you resolved the comment. If you resolved it using a different approach than the one suggested by the reviewer, explain why.

I managed to resolve this differently by moving the whole method to the PaprikaChipsService. It probably should have been there from the start… Thanks for the comment!

• Always thank the reviewers in comments for their time!

Thank you!

As a PR Reviewer

• Before starting the review, check who the author is and what their background is. Tailor your approach based on that. A junior developer who joined a remote team 1 month ago will require a dramatically different approach than a 5-year senior who sits next to you in the office. If unsure, ping that person directly and ask.

Hey! Nice to meet you! I just picked up your PR, but since this is the first time we will work on something together, I wonder how you would prefer to do it? We can just go through it together in a huddle if you would prefer that, or I can just leave comments in Bitbucket. I noticed this is your first PR in this repo, if you need help or something is not clear, please do not be afraid to ping me, I will try to explain if I can.

• Explain why you are asking authors to change something. Provide links, context. See your comment as a possible way to share knowledge. Do not assume the author will automatically understand. Authors have various skills and seniority levels, and something that is obvious to you might be completely new to them. 

I suggest you rewrite this to use streams. Unless you need really hard-core performance (which does not seem to be the case), streams are much better for readability/maintainability. Check this out for a pretty good introduction to streams and a comparison to standard loops. 

• Provide specific suggestions to fix if you can. If you do, don’t forget to explain why. This is good practice even if you know the author will understand. There might be other PR reviewers who might not understand, and this can still be an education for them.

a.b?.c instead of a.b.c.This will prevent runtime errors in case b is null, since if you look at the method above, there are cases where it can be null. See here for details.

Both Author and Reviewer

• Be friendly, polite, and empathic. There is another human at the other end. Do not write anything you would not want to read yourself in your own PR. Use emoticons (but don’t overdo it). Don’t be sarcastic and don’t mock/make fun of the other person’s work. Don’t forget to also give credit if you see something cool!

Nothing to change, I just really like how you solved this.  Well done.

• Be aware of the team’s working agreement or coding standards. Is there a Boy Scouts rule in place to always leave code in better shape than you found it? Are there specific requirements for tests, external dependencies, formatting, etc.? If unsure, ask before you start development, submit the PR, or start the review!

Hi everyone, I’m wondering if it’s okay to include Lombok in this project. Could you please let me know? I could not find any explicit rules regarding dependencies. Thanks!

• Consider reaching out to each other directly for more complex reviews, reviews with urgency, or reviews with a lot of findings, and have a huddle/chat to avoid a lot of comments/ping-pong

Hey! Would you have time to talk about this PR <link> today or tomorrow? I see there are a lot of changes, and the requirements are complex as well – discussing it in person, at least for the first round, might be easier. Thanks!

• Stick to facts. If you have an opinion about why something should be done, provide links to support your statement.
  • For more subjective topics, be prepared for compromises. If you cannot find definite sources to support your case (this happens often in discussions about code formatting, etc.), consider just doing whatever feels easier / quicker / more friendly to the other party. Mutual respect and a good working relationship in the team are usually better than completely uniform code. If necessary, agree to follow up on a team-level meeting to reach an agreement there.

I don’t think we actually have an agreement about this in the team, but we should try to agree on some naming pattern for test methods. Let’s keep this as it is for now, but I will add this to the agenda for our next sync if you don’t mind. If you have any suggestions, please let me know!

• Do not jump to conclusions. Respectfully ask for context and reasons.

I am not sure why this needs to be package-private. I see you refer to it in the tests, is that why? I think they could be refactored to avoid this, but on the other hand, I am not sure it is worth the effort… Could you please provide a bit more context? We can also chat on Slack if that would be quicker. Thanks!

• Be mindful of the time. Will it take you more time to argue about something, or will it be easier to just implement the change and move on?
  
• Put the conclusion back into the PR in writing if you agree on something in a huddle or some other communication channel. This can help others who did not participate in the call understand what was decided and why.
  
• Be prepared to be wrong.

And I think that’s it! Stay nice, keep a positive attitude, make friends, and write good code and the future will be bright.