Here’s your personalized workspace.
Explore Grammarly’s suggestions to ensure your writing is at its best.

# Best practices for Pull Request Reviewers

:::info
:bulb: The template is for facilitating the best pull request etiquette agreed upon by us as an organization/Team that fits our culture better.
:::

## :book: Topic on the table

## :small_blue_diamond: PR communication
**1. Rephrase your objection as a question**

Bad: "This change will make XXX impossible." (This is hyperbole; is it impossible?)
Good: "How can we do XXX with your change?"

**2. Avoid hyperbole**

Simply state your concerns and ask questions to help get to the desired outcome.

Bad: "This change will destroy performance."
Good: "It seems like doing X might be slower than existing Y; have you measured/gathered data to show it isn't?"
Better (if you have time): "In the meantime, I am gathering data to try to verify that X is not slower than Y."
Also good: "This change changes this single loop O(n) to a doubly nested loop O(n²); won't this affect performance?"

**3. Keep snide comments to yourself**

Some thoughts are better kept to yourself. If you can't be civil, don't engage.

Bad: "I think this change is bad and will ruin everything."
Bad: "Are you sure that software engineering is the right career path for you?"

**4. Engage positively**

Maybe you had a different idea about how to solve a problem. If you engage positively, you might end up discovering a solution that is better than either original option.

Bad: "This change sucks, my version is better."
Good: "I also have a similar change at this location XXX: maybe we can compare and/or combine ideas."
Also good: "I have a similar change in progress, but I chose to do X because ZZZ; why did you choose Y?"

**5. Remember that not everybody's experience is identical to yours**

An otherwise completely competent engineer could go for years without knowing some fact that you take as common sense. It's okay to state the obvious, as long as you aren't patronizing or snide about it.

Bad: "Can't you see that this is wrong?"
Good: "This is incorrect because it causes a null pointer exception when X is Y."

**6. Don't diminish the complexity of something that's not obvious**

Remember that things that are obvious to you may not be obvious to everyone. Suggesting alternative approaches and pointing out useful examples can help get everyone on the same page.

Bad: "Why not simply from the nozzle?"
Good: "It might be possible to frob the nozzle, which would simplify this part (see XXX for an example)."

**7. Be respectful**

Sometimes a submission just doesn't meet a minimum standard for quality. It's okay to say so, but it doesn't cost anything extra to be respectful.

Bad: "This is stupid code written by a stupid person."
Good: "Thanks for your contribution. However, it cannot be accepted in its current form; there are multiple problems (as outlined above)."
Also good: "As outlined above, there are multiple problems with this submission.  Maybe we could back up a step and talk about the use cases instead?  That could help us find a path forward."

**8. Manage expectations (and your time)**

If a submission is too large to be reasonably reviewed, it is okay to let the submitter know right away. Keep moving forward.

Bad: "I'm not merging this, it's too big."
Also bad: Ignoring it until it goes away.
Good: "Could you please break this down into smaller changes? I do not have a lot of time for code reviews and this one is just too large/complex to review in one pass."

**9. Say please**

Just saying "please" goes a long way toward showing that you respect the submitter's time, especially when you want something to be different due to formatting or style, which might seem to be a minor detail of the change. Examples:

"Could you please separate the whitespace changes into another pull request?"
"Could you please align these variable definitions so they're easier to read?"

## :small_blue_diamond: Take a Deeper Dive:

Take a deeper dive into the code pull the branch locally and tinker around with the new methods created

## :small_blue_diamond: Provide examples:
It's very useful to articulate your code suggestions by not only using words but also providing a sample example to elaborate.

It's often time when I read a suggestion and im left with ??? around what is the ask of the reviewer.

Ig: We have a User class that was committed and has a lot of responsibilities:

class User
  attr_accessor :name, :email, :phone_number
  def initialize(name, email, phone_number)
    @name = name
    @email = email
    @phone_number = phone_number
  end
  def send_email(message)
    # Code to send email
  end
  def call_user
    # Code to call user
  end
  def update_phone_number(new_phone_number)
    @phone_number = new_phone_number
  end
end

user = User.new(name, email, phone_number)
user.send_email(message)
user.call_user
user.update_phone_number(new_phone_number)

A less effective comment:

This is a good start, It might be possible to split the responsibilities.

A more effective comment
This is a good start, It might be possible to split the responsibilities, which would simplify this part see this example.

User.new(name, contact)
EmailService.new.send_email(to, message)
PhoneService.new.call(phone_number)

## :small_blue_diamond: Addressing conversations peers' conversation:

It is best practice to address any comment on PR either by emojis or by commenting back on the initial comment and/or resolving the conversation once there is no more back and forth.

## :small_blue_diamond: PR Eyes:

It is best practice to have at least two eyes on our PRs. This number is also based on the number of developers on the team. It simply allows spotting any errors or areas of improvement that one may have missed.

## :small_blue_diamond: Approving VS Block PR:

**When is a PR approved?**

"When all conversation is addressed and suggestions code quality has been meeting with two thumbs up"

**When is a PR gets blocked?**

"When there is an obvious problem that if metered will cause a problem where we have to rollback code. blocking a PR should be reserved to when aka 'This code will break master lol' "

## :small_blue_diamond: Straight up Chat:

It is also very acceptable to chat with the PR author and bring up your concerns or ever have a chance.

concerns