Confirmed users, Bureaucrats and Sysops emeriti
722
edits
User:GavinSharp/Code Review: Difference between revisions
no edit summary
GavinSharp (talk | contribs) |
GavinSharp (talk | contribs) No edit summary |
||
| (4 intermediate revisions by 2 users not shown) | |||
| Line 5: | Line 5: | ||
* am I the best person to review this? | * am I the best person to review this? | ||
** is someone else better suited? | ** is someone else better suited? | ||
** should other people | ** 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! | ** can be hard to not let pride get in the way! | ||
* should we do this? | * should we do this? | ||
| Line 33: | Line 34: | ||
** consistent code style should be a means, not an end | ** 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" | ** 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? | * does the patch have performance implications? | ||
** hot paths (e.g. startup, pageload) | ** hot paths (e.g. startup, pageload) | ||
| Line 38: | Line 40: | ||
* documentation | * documentation | ||
** think of future us! think of what might be obvious now, but not later | ** 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? | * platform specific concerns? | ||
* grab bag of acronyms: l10n/rtl/a11y/1337 | * grab bag of acronyms: l10n/rtl/a11y/1337 | ||
| Line 46: | Line 50: | ||
* be friendly; particularly for first-time or infrequent contributors (doesn't always come naturally!) | * be friendly; particularly for first-time or infrequent contributors (doesn't always come naturally!) | ||
* focus on substance, not style; outcomes, not process | * 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. | * 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 | * 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. | 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. | ||