User:GavinSharp/Code Review

Effective Code Review

Preliminary questions

Things to ask yourself when you receive a review request.

• am I the best person to review this?
  • is someone else better suited?
  • should other people at least be notified/involved?
  • is my current queue too big?
  • can be hard to not let pride get in the way!
• should we do this?
  • sometimes this is very easy to answer, other times it's the hardest part
  • can often rely on others in better positions to make that judgement (e.g. ux)
• is this the best way to do this?
  • easy to forget this step before getting into specifics
  • creativity and knowledge of common techniques/approaches is key - don't let the patch author's approach constrain your imagination
• is the patch too big, or doing too much at once?

Specifics

• have we addressed this problem before?
  • is there A Long History? (HG/CVS log/blame, archaeology, Netscape-era bugs!)
  • building relevant context is really important
• does the patch have tests?
  • what is the test coverage like? could it be more complete?
• is the patch sufficient?
  • check that all manifestations of the bug are fixed - this is where domain knowledge is most useful
• is the patch complete?
  • obvious: refactorings - did you forget to update a caller? stuff like this should be the test's job, but we don't always have 100% coverage
  • code removal patches: check not only that the removals themselves are OK, but also that you removed everything
  • …but be careful not to scope creep!
• does the patch impact extensions/other products/gecko consumers?
  • e.g.: toolkit vs. browser
  • defining a new interface? SR can help (but this is just a more formalized subcategory of "ask for help")
• code style: care, but don't
  • consistent code style should be a means, not an end
  • rather than asking "does this match our coding style guidelines", ask "is this code hard to read"
    • if you can't appreciate the distinction between those two questions, try harder :)
• does the patch have performance implications?
  • hot paths (e.g. startup, pageload)
  • sync IO?
• documentation
  • think of future us! think of what might be obvious now, but not later
  • add relevant references; keep in mind permanence of those references
  • extra comments never hurt anyone (but don't write a novel!)
• platform specific concerns?
• grab bag of acronyms: l10n/rtl/a11y/1337
  • bring in domain experts when there's any doubt at all

Key Points

• don't be reluctant to redirect requests, or ask for additional help
• be friendly; particularly for first-time or infrequent contributors (doesn't always come naturally!)
• focus on substance, not style; outcomes, not process
• be open-minded; avoid the "NIH" mindset
• ask and encourage others to ask for feedback early. review shouldn't be about "big questions" - those should use a feedback request, or comments, or IRC, etc.
• when you get the urge to push back, think hard about whether it's worth it. put yourself in the other person's shoes, try to step back and look at the bigger picture, always look on the bright side of life, and most importantly: avoid using too many clichés

Code reviewing is (perhaps obviously) all about judgement calls; the cost of accepting an imperfect patch can sometimes be outweighed by the cost of asking someone who has invested a lot of time in a patch to revisit it.