Skip to content

Latest commit

 

History

History
135 lines (94 loc) · 5.63 KB

README.md

File metadata and controls

135 lines (94 loc) · 5.63 KB
Raw

Code Review

A guide for reviewing code and having your code reviewed.

Watch a presentation that covers this material from Derek Prior at RailsConf 2015.

Everyone

  • Accept that many programming decisions are opinions

    • Discuss tradeoffs, which you prefer, and reach a resolution quickly.

  • Ask good questions; don't make demands

    • "What do you think about naming this :user_id?"

  • 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.

  • Avoid diminishing words

    • "simply", "simple", "just"

  • 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

  • Keep it real

    • If emoji, animated gifs, or humor aren't you, don't force them.
    • If they are, use them with aplomb.

  • 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."

  • Avoid the "since you're at it" attitude

    • If you would like to recommend a code change unrelated to the current pull request, suggest it in the appropriate place or open a ticket for it (on Trello, JIRA, GitHub project...)

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.

  • Link to the code review from the ticket/story

  • Push commits based on earlier rounds of feedback as isolated commits to the branch

    • Do not squash until the branch is ready to merge.
    • Reviewers should be able to read individual updates based on their earlier feedback.

  • Seek to understand the reviewer's perspective

  • Try to respond to every comment

  • Wait to merge the branch until continuous integration tells you the test suite is green in the branch

    • TDDium, Travis CI, CircleCI, GitHub Actions, etc.

  • Merge once you feel confident in the code and its impact on the project

  • Final editorial control rests with the pull request author

  • Recognize the work of your teammates when you are pairing

    • Use Co-Authored-By: <name> <email> at the end of your commit message.

Reviewing Code

Understand why the change is necessary (fixes a bug, improves the user experience, refactors the existing code).

Then:

  • 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 to a regular Friday afternoon technique discussion

    • 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

  • Approve the pull request

  • Remember that you are here to provide feedback, not to be a gatekeeper

  • When suggesting changes using the "Add a suggestion" feature:

    • 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

Style Comments

Reviewers should comment on missed style guidelines. Example comment:

> Order resourceful routes alphabetically by name.

An example response to style comments:

Whoops. Good catch, thanks. Fixed in a4994ec.

If you disagree with a guideline, open an issue on the guides repo rather than debating it within the code review. In the meantime, apply the guideline. It's often helpful to set up a linter like standard to format code automatically.

This helps us have more meaningful conversations on PRs rather than debating personal style preferences.

  • Leave one comment only, for multiple stylistic offenses of the same kind
    • If there are a few occurrences of the same change needed, do not leave multiple comments for the same change, rather suggest running the linter, and/or leave one comment only, mentioning the line and elsewhere, as long as the other files are being edited in the pull request.