PR_review_notes.md

REVIEWING CODE:

• If possible, before the review, talk to the requester for 5 minutes to get a bit of a preview of the code. This helps you get a mental understanding of what you are reviewing.

• Teach, don't just tell. Reviewing a PR shouldn't JUST be about code safety, but also an opportunity to mentor as well.

• Don't forget to praise concise/readable/efficient/elegant code.

• Remember that you are reviewing another human's code. Ask, don't tell.

Be nice on your phrasing:

• This way creators won't be scared to innovate and will think of "what's best" instead of doing what the reviewer tells them.

• When reviewing code, you are going to be making suggestions for what to change, but also make sure to state WHY it should change.

• When phrasing code, try to refer to the mistakes of the "code" rather than the "person". An example of this would be:

  • "The logic on line #25 isn't clear" compared to: "Your logic on line #25 ins't clear."

• There is more than one solution. Having someone re-do their entire code so it fits your solution isn't the purpose of a code review.

• Weigh the cost of just approving the changes. If it's something that's not super critical, then lean towards the tradeoff of letting not 100% superb code get merged over destroying a relationship.

• When reviewing code, make sure to be wary of how many comments you are leaving. If you are leaving 50+ comments, stop. Give the author a chance to change and make corrections before you continue. Remember that as an author, it's daunting to see 50+ comments in a code review.

• It is frustrating when your teammates withhold reviews. The implementation doesn't have to be perfect. If it isn't up to standard, we should think of a more iterative way to help the author bring the code quality up (either through pair programming, or otherwise).

  • If no improvement is being made after multiple round of reviews, then you should withhold your review and consider a different approach to help the author up their code quality.

• If there is something near/around the code you've identified as something that should change, try to suggest this in another ticket. Suggesting other changes, unrelated to the scope of the ticket, could help blow up the scope of the ticket as well as affect their timelines.

  • A rule of thumb: If the changelist doesn't touch the line, it's out of scope. source
  • If it's an easy fix, then it's okay to break this rule. Otherwise just leave it as a comment and see if they are willing to log it as a follow-up or a backlog ticket.

When you are reviewing a pull request, check your ego at the door and be open minded to considering new solutions.

• When you're done, you should be able to answer two following questions for yourself: "What goal does this code accomplish?" "How does it accomplish this goal?"

  If you cannot answer both questions, you don't fully understand the changes!

• Don't just gloss over test code. Read them and try to answer:

  • Are the test titles adequately descriptive?
  • Are key scenarios captured?
  • Are enough edge cases covered for comfort?
  • What parts of the application are exercised by a single test? Too much? Too little?
  • Are the tests written with good assertions? Can they ever fail? Will they fail too often?
  • If a test fails, would it be easy track down the error?
  • If new frontend behavior has been added, has it been added to the manual test script? Browser automation tests?

• Try to review the tests first. This way -- you'll get a better idea of what the new application code intends to do.

• When reviewing application code, keep a list of what could go wrong.

• Think about the inputs/parameters. What happens at extreme values?

• Think about conditions that should always be true -- these are invariants. Attempt to have tests to verify this.

• Good code reviews will point out:

1. Hard to understand code
2. Unclear names
3. Unhandled edge cases

• If the writer ends up attempting to push a solution that isn't correct, be nice and reject them nicely. Maybe start with: "Looks like you put some good work into this, thanks! However, we're actually going in the direction of 'XXXX' because of YYY. I'd suggest doing ZZZZ approach instead."

  • Main focus is to do it courteously. This is important because we want to show that we respect each other as developers even though we disagree.

• Find the file or files that are the "main" part of the PR. Look at these first. This will help give context to all of the smaller parts, and generally could accelerate the review.

  • If there's too much, then ask the developer which section he/she should review first.

Complexity

• Are functions too complex? Are classes too complex? Can it be understood quickly by code readers?
• Explanation written only in PRs aren't helpful for future readers.

Code Design

• Functions and classes should exist for a reason. When the reason is not clear, this is an indication that the code needs to be rewritten or supported with comments or tests.

Rejections

• If you do reject/make a suggestion, make sure to explain why. A good explanation demonstrates both an understanding of the developer's reply and additional information about why the change is being requested.
• As for upsetting developers -- most won't get upset if you suggest changes in a polite/courteous way.

Style

• Comments

General

• Your goal as a reviewer is to unblock the developer or enable them to take some sort of further action quickly without sacrificing health to do so.

• Take breaks in between reviewing code. You won't be as efficient after XX lines or YY hours.

• Per Google's standard, "One business day" is the maximum time it should take to respond to a code review.

• However, if you are in the flow of writing code, then don't interrupt yourself to review some code.

• If you are too busy to get a PR reviewed, let the developer know. Try and suggest other developers for the writer to reach out to. Some response to the writer is better than none.

• Code reviews help push/move the team forward. Faster feedback iterations will help push your team to ship/build product faster.

• When code reviews are slow:

  • velocity of the team decreases.
  • frustration ensues upon the developer as he/she has to wait "days" for the blocker to respond so he/she can properly proceed.
  • code health can be impacted. Developer may opt to hide details to get PR in state that will get merged by end of sprint.

Better code reviews look at the change in the context of the larger system, as well as check that changes are easy to maintain.