6  Guidelines

Adapted from:

6.1 Everyone

  • Accept that many programming decisions are opinions
    • Discuss tradeoffs, which you prefer, and reach a resolution quickly.
  • Offer positive comments
    • What did you learn? What did you think was neat?
  • Ask good questions; don’t make demands
    • “What do you think about naming this function fetch_user?”
  • Good questions avoid judgment and avoid assumptions about the author’s perspective
  • Ask for clarification
    • “I didn’t understand. Can you clarify?”
  • Avoid selective ownership of code
    • “Mine”, “not mine”, “yours”
  • Avoid using terms that could be seen as referring to personal traits
    • “Dumb”, “stupid”.
    • Assume everyone is intelligent and well-meaning.
  • Be explicit
    • Remember people don’t always understand your intentions online.
  • Be humble
    • “I’m not sure - let’s look it up.”
  • Don’t use hyperbole
    • “Always”, “never”, “endlessly”, “nothing”
  • Don’t use sarcasm
  • Talk synchronously if there are too many “I didn’t understand” or “Alternative solution:” comments
    • Chat, screen-sharing, in person
    • Post a follow-up comment summarizing the discussion.
  • If you learned something new, share your appreciation
    • “I did not know about this. Thank you for sharing it.”

6.2 Having Your Code Reviewed

  • Be grateful for the reviewer’s suggestions
    • “Good call. I’ll make that change.”
  • Be aware that it can be challenging to convey emotion and intention online
    • You may want to consider using labels to convey intention and tone.
  • Explain why the code exists
    • “It’s like that because of these reasons. Would it be more clear if I rename this class/file/method/variable?”
  • Extract some changes and refactoring into future tickets/stories
  • When making visual changes, include screenshots or screencasts to show the effect of the changes
    • You may want to consider before/after screenshots or screencasts whenever applicable.
  • Seek to understand the reviewer’s perspective
  • View each comment as an opportunity to learn
  • If you’re someone who says “sorry” frequently, think about removing this from your language
  • You can ask for clarification
  • It’s OK to disagree. It’s helpful to first look for things that you agree with and explain why you disagree

6.3 Reviewing Code

  • Communicate which ideas you feel strongly about and those you don’t
  • Identify ways to simplify the code while still solving the problem
  • If discussions turn too philosophical or academic, move the discussion offline
    • In the meantime, let the author make the final decision on alternative implementations.
  • Offer alternative implementations
    • But assume the author already considered them.
    • “What do you think about using a custom validator here?”
  • Seek to understand the author’s perspective
  • Remember that you are here to provide feedback, not to be a gatekeeper
  • When suggesting changes using the “Add a suggestion” feature (look for this icon: ):
    • Communicate clearly which lines you suggest adding/removing
    • Test the suggested changes to validate it works whenever possible
      • When not possible, let the pull request author know that you did not test the suggestion
    • Provide some context to let the author know why you’re suggesting the change