Table of contents

• Introduction
• Things to consider before creating PR
• PR review etiquette

Introduction

Doing a effective PR review is tough job. Over time, i have came to a realisation that implementing some tatics and action in your PR review process will make it much more smooth and enjoyable experience. This came from both good and bad experiences and i hope will help you one way or another.

Things to consider before creating PR

“One rule, keep it small.”

We all know that “small PR = 100 issues”, “large PR = LGTM”

PR review etiquette

1. Be humble and be receptive of feedback. leave your ego out of code reviews

2. Be precise. Make sure you double check every comment you write and response you put to a PR comment. Think twice before responding to review or reviewing by saying do this instead of this.

3. Do not enforce but suggest. Always provide constructive feedback. Developers push change are made for a reason. If you request a change as a reviewer, state the intend for the requested change properly and also explain why that change was requested.

   Bad: Do not name variable like this. ...
   Bad: Do this ...
   Bad: Use this ...
   Bad: Change this to ...
   Bad: Why did you do this ?
   
   Good: Could you please let me know why was this implemented this way ?
   Good: Please update the variable name to XYA_ABC because this will increase readabili
   and maintainability of the code.
   

4. Avoid nitpicking comments that are unnecessary.

5. Avoid having long conversation. PR is to review , not communicate. If a thing needs to be discussed take it to general chat and raise with the team.

6. Stop responding to feedback with “vibe” as an answer.

   Bad: 
   
   Comment: Why did you use this method ?
   Answer: I just do it like this, i like doing it this way
   
   Comment: Why did you do it like this as we could do it this way ?
   Answer: I have been doing it this way.
   
   Good:
   
   Comment: Why did you use this method ?
   Answer: I used this method because, it increases the application effeciency and also makes
   the code readable. Implementing it this way gives us more room to optimisation
   and re-usability down the line.
   
   Comment: Why did you use this method ?
   Answer: I used this method because, implementing it this way making our function X make our code reusable for other part of the
   application.
   

7. Be strict about temporary code. If some does something that is temporary, make sure it is treated with same as normal code. Do not be lenient on temporary code.

8. Look for blast radius. Does this change fit the application flow. Will this break production.

9. Always Ask if you don’t understand the implementation.

  a. "Why did you use method A to achieve goal B?"

  b. "What gain does declaring a variable have in the instance of its usage in method Z?"  

  c. "I see you have copied/pasted some code, did you consider refactoring? You didn't refactor, what was the reasoning behind that choice?" 

10. Spot bad code practices with a appropriate explanation comment why that code is smelly. Every engineer is different. Learn to tell the difference between stylistic preference, and bad practice.

11. Keep an open mind when reviewing someone else’s code. Be receptive to their ideas, and make sure that changes you suggest are suggested for a reason. It is assumed that the checked-in code builds, but that doesn’t mean that it follows best practices. Make sure that anything you bring up can be backed up with a cited reference. If you say it doesn’t follow best practices then cite the standards document and section. If you say it isn’t a “performant” method, then have a link to a document that shows why and possibly provides metrics.

12. When your code is reviewed, consider everything the reviewer is suggesting objectively. If you’re pushing back, ask yourself honestly if you’re just being defensive or if you really believe you have a case. If you have a case, continue the dialog. Don’t get argumentative, bring ammunition such as facts and metrics.

13. Whether you’re a reviewer or a reviewee, use code reviews as an opportunity to educate. Whether it’s educating yourself or the person you’re reviewing, if there is a discrepancy then there is a chance to learn somewhere. Make good use of it.

14. Ask questions and be ready to have your questions answered truthfully. I recently made a statement that was “pseudo-true”. It wasn’t wrong, but it definitely wasn’t right. A junior developer challenged me on it, and I disagreed. My response was “I haven’t seen that behavior, but if you can find me a document on it I would love to read it.”. I spent about an hour that afternoon reading the document he sent me, and now I have a much better response (re: educated) when confronted with a similar situation.

15. Know when someone is trying to belittle your work and when someone really wants better for you. This depends on the dynamics with your your team members. Good colleague will want better for your and overall company but still will keep your experience before their ego. In short, you will get the vibe. In that case, just accept and continue or reject with proper concrete and supporting answers.

16. Provide constructive feedbacks. When someone works on something, that work is dear to them. Do not just discredit their work as it will trigger people’s emotion. Instead, provide constructive feedback using language such as “I would recommend”, “I suggest”, “If you do this way, then you could impprove your work by X”, “What if you do it this way”. If you know the person well enough then short feedbacks are good if not, use a bit mellow tone. Do not use tone like “This work is garbage” etc. Well it also depends on the team dynamics as well.