| z, ? | toggle help (this) |
| space, → | next slide |
| shift-space, ← | previous slide |
| d | toggle debug mode |
| ## <ret> | go to slide # |
| c, t | table of contents (vi) |
| f | toggle footer |
| r | reload slides |
| n | toggle notes |
| p | run preshow |
loading presentation...
Prostyle code review
Lennon Day-Reynolds
Prostyle (adj.)
- "Done in a professional and high-quality manner"
- Antonym: "jank", "janky"
About me
- ~14 years in industry
- Currently an eng manager at Twitter
- >500 engineers in a few repositories
- Rapid growth, but need for stability
tl;dr
- If you aren't doing any sort of code review yet, go home and start
What is a code review?
- Manual reading of source code
- Teammates and collaborators, not author(s)
- Record of comments + approval
- Ideally applied to all new code
What is a code review? (cont.)
- Pre-push (review vs. audit)
- Async/distributed, not face-to-face
Why
- Fallibility
- No safety net
- Bus factor
- Scary legal stuff
Extra motivation
- Show off what you're doing
- Ship-it whisky!
Four critical questions
- Where's the evidence?
- Does the code work?
- What about style?
- Is it efficient?
Where's the evidence?
- Test coverage
- API docs/comments
- Staging link/build
- Example output/logs
- Screenshots
Syntax, lint, and static analysis
- Build/test output is great
- Lint, jshint, checkstyle, etc. are more mixed
- Be careful: can easily add friction
- (More on coding style later)
Does the code work?
- Read patch, description, commit logs, comments
- Step through it in your head
- Look for common bugs (exceptions, off-by-one, typos)
- Think about edge cases
What about style?
- Basic format/structure
- Naming
- Conciseness (a.k.a. DRY)
- Watch out for bikeshedding
Style guides
Style guides
- Start from/reference a standard guide
- Capture team conventions
- It's a living document
- Reference them liberally in reviews
Is it efficient?
- Obvious is more important than fast
- Performance is its own change (w/new review)
- Don't optimize w/o profiler data
- Worry about order-of-magnitude issues
- Slow resources first: RPC, DB, filesystem
In practice
- How many/often?
- Treat like email
- Be responsible and respectful
In practice (cont.)
- What about huge diffs?
- Separate blockers from nice-to-haves
Email+diff
GitHub
Review Board
Rietveld/Gerrit
Phabricator
Many, many more
- Kiln
- Crucible
- ...but try a free/Free option first!
