Javi Javier

Code Reviews

I’ve recently shifted my focus on improving the team aspect of making websites. I’ve thought a lot about how good teams develop norms. Are they just good habits that form by practice over time or are they explicitly stated and worked on? Having level-headed teammates who take pride in their work has helped our team a lot, but recently there have been some rough spots when it comes to code reviews. I drafted this document as a starting point for conversation and to help make our norms clearer. The initial feedback from my team was positive and I look forward to refining it as we try things out and find what works and what doesn’t.

Lots of stuff here from: http://amyciavolino.com/assets/MindfulCommunicationInCodeReviews.pdf

Pull Requests

Are

  1. Sanity check: any obvious mistakes that a linter couldn’t catch (ideally linters should catch all the obvious mistakes)

  2. Shared understanding:

    • so the team knows what changes are happening to our codebase
    • so people who work on the code in the future can understand decisions behind changes
    • and so the team can revisit critical code changes and revisit the decisions

Are not

  1. Peer Evaluations: so a check is not an A and an X is not an F
  2. Introducing new linting: we should discuss code style decisions and automate or document them outside of the ticket. Any discussions of this type should not block the ticket.
  3. Architecture discussions a. architecture decisions should ideally be front-loaded when evaluating a ticket, keep this in mind when picking up a ticket and start a discussion b. if it comes up in the middle, create a low fidelity, low effort proof of concept and push up the branch for discussion with the team c. if it comes up in the end, the preference is to ship a functional fix (meets the business/design requirements), and create a separate ticket to get back to doing a) and b)

Guidelines

For the Pull Requester: Keep the reviewer in mind

  • Keep the PR short and focused on the ticket (ex. tangential refactors are good, but should be broken out to their own PR)
  • Clean up unnecessary changes and noise (ex. commented out code, squash commits together, or cut a new branch and reapply your changes)
  • Link to the JIRA ticket
  • Provide test URLs
  • Provide screenshots
  • Provide steps if applicable (ex. npm install first, use a different environment, click X button on page)

For the Reviewer: Keep the requester in mind

  • Make feedback specific

    • comment on specific lines of code
    • provide links for context (links to documentation, links to previous code on github)
    • provide code examples where helpful (ex. show how you think a block of code could be refactored)
    • provide specific instructions on hwo to reproduce issues you ran into
  • Help the requester get to the finish line

    • keep scope in mind and write out tickets for related issues uncovered

For Both

  • Keep the labels up to date so everyone can jump in and help (ex. One More Reviewer, Ready for Code Review)
  • Follow up on Slack or in person to move the process along as automated notifications can get buried

Code Style

Minutiae/nitpicks/details should be covered in linters

(and we can adopt more detailed style guides as well)

  • ex. spaces vs tabs
  • ex. variable naming conventions

The objective is to offload these decisions to tools to avoid spending developer time on these kinds of issues

Prefer Straightforward, Boring and Verbose over Novel or Terse or Fancy

  • ex. simple ifs over fancy ternaries The objective is code that has a very low level of entry so that anyone can read and understand it and contribute

Prefer consistency with the rest of the codebase

  • in areas of ambiguity (“This piece of code I’m adding is not covered by code style guide or linter…”) strive to be consistent with the immediate surrounding code. The objective is to make it easy to read and grasp that particular area of code. Consistency helps other readers get into the flow quickly and understand what the code is doing

Test Coverage (Realistically)

  1. Code that has been fixed/patched several times should have tests with tests geared towards preventing those bugs or documenting those feature changes
  2. New complex code, to help catch bugs as you create them, and as a way of others understanding what the code intends to do