CodeReviewGuidelines.md

Code Review Best Practices

This guide is heavily based on Syeda Kauser Inamdar’s talk at GHC 2020, “Transform your Code Reviews for Inclusive and Productive Collaboration.”

• Benefits of code reviews
  • Find bugs in code
  • Sharing knowledge: Both author and reviewer are sharing knowledge with each other that they might not share otherwise
  • Improve code quality by having a second perspective on applying best practices to the code
• Frustrations with code reviews
  • Insufficient information
    • Author: the reviewer may not provide enough details in comments to be useful; the reviewer might make incorrect assumptions if they don’t have enough context
    • Reviewer: the author may not provide sufficient info about what they’re doing for the reviewer to give meaningful feedback
  • Hostile/unproductive comments from reviewer
  • Too much time
    • Author: unhelpful or argumentative comments from reviewers require the author to spend time defending or explaining themselves
    • Reviewer: if the reviewer doesn’t have sufficient context, they may spend a lot of time gathering context or commenting on issues that aren’t relevant
• Both reviewer and author play multiple roles: technologist, thinker, student, teacher, and human. Think about how your review addresses and respects each of these roles
• Best practices for reviewers
  • Understand context, don’t approve in rush
    • If you don’t have sufficient context, forward to someone who has more context
  • Ask clarifying questions and use questions to open discussion
    • Constructive: “why was this file added?”
    • Not constructive: “we don’t need this file”
    • Not constructive: overly general questions like commenting “why did you do this?” on the review as a whole
  • Give specific and actionable feedback
    • Specific and actionable: “Can we simplify the data passed in so we can skip introducing a new usage of the mixed type?”, provide a code example
    • General and not actionable: “I don’t think this is the right way to do this.”
  • Focus on language and tone
    • Positive tone: “I really appreciate you working on this and I think we could make it more generalizable."
    • Neutral tone: “I’m not familiar with this use case, could you share more context?”
    • Negative tone: “This seems pretty useless. I don’t know why we would need this.”
    • Negative tone: “This is weird."
  • Review the code, not the person
    • Reviewing the code: “This section could be more efficient if you refactor it by…”
    • Reviewing the person: “You’re being really inefficient."
  • Propose a solution (growth mindset)
    • Solution focused: “I’ve used a different approach when writing code like this. Here’s how I’ve done it, and here’s what the benefits are over the current implementation.”
    • Not solution focused: “You shouldn’t do it like this."
  • Give positive feedback - point out what good code looks like
    • Positive: “I really like how you used list comprehensions here, it helps make the code more readable.”
  • Clarify how critical the feedback is and whether it needs to be addressed before shipping
    • Use “nit” or “supernit” for comments that do not block shipping, such as comments that reflect personal preferences around style
    • If none of your comments are blocking shipping, approve the code review but ask the author to review your comments
    • If there are issues that need to be fixed but can wait (such as non-critical refactoring), open a ticket and attach it to the diff
• Reviewer checklist
  • Each comment should be concise, actionable, and timely
  • Adhere to shared standards and SLAs
  • Comments should use collaborative/empathic language
  • Comments should be constructive vs. overly critical/nitpicky
  • Use multiple passes to address broader issues first
    • In first pass, identify high level issues - is it solving problem, is it needed, design, etc.
    • Then nitpick in subsequent passes. Use “nit:”
  • Embrace growth mindset and keep iterating on the process
• How to improve as author/submitter
  • Start strong: provide as much context upfront as possible, especially about reasoning for your design decisions and details on how you have tested your work
  • Remember that code reviews are there to help
  • Don’t take it personally
  • Respond to vague comments by asking for actionable feedback: “what do you suggest I fix?”
  • Don’t try to resolve complex issues in the comments - sync up outside the review to make decisions and then document the discussion and outcome in the PR
  • Ensure the whole team agrees on shared standards and an SLA for comments and followups
  • For best practices and standards around style, use static code analysis and automation to ensure consistency, such as using tools like pylint, flake8, black, or isort in pre-commit hooks or in continuous integration tests
    • Automating enforcement of style guidelines also prevents reviewers from having to spend time on it and reduces back and forth
  • Include links to relevant docs, such as designs or project plans, so the reviewer can get additional context if needed
  • Minimize code review scope: smaller changes are easier to review and more focused code reviews require less context for the reviewer
• Author checklist
  • The title of the review or your commit message should be one line that concisely answers the question, “what would happen if this commit were applied?"
    • Start your commit message with an action verb (update, remove, add, fix)
  • Acknowledge every comment: even if you don’t intend to fix it, take the time to respond
  • Submit incremental changes: if a code review would cover multiple changes, split into multiple reviews
  • Check your PR as if you were a reviewer before submitting
  • Embrace growth mindset and keep iterating on the process