Code Review

(Alex Gaynor, Python Software Foundation)

• reviewing diffs vs. entire body of code: The latter takes the "why" out of the process
• "raise the bus factor"
• ensure readability: counterbalances self-optimization we do when we write code
• catch bugs: least important purpose of code reviews
• encourage healthy engineering culture: mechanism for giving and getting regular feedback

Ground rules for effective code review

• don't make people do a machine's work (tests, style guide issues, merge conflicts should be handled by the machine). Automate! Sometimes psychologically easier to get this feedback from a computer than a human
• code review applies to everybody (juniors, seniors, etc.)
• do code reviews before merges: less-likely to have conversations post-merge
• code review applies to every change, every time: Better than creating adhoc rules on when code reviews apply.

Get started

Tools:

• Phabricator
• Github
• Gerritt

Requirements:

• leave comments in-line
• track history

How to be a good patch author

• describe what the patch does
• keep it small (200-400 lines of code max!): code reviews are less effective after that. Split up large patches by functions that can stand on their own
• be collaborative: Have conversation, don't just do what the reviewer says

How to be a good reviewer

In sequence:

• what's the intent?
• design: architecture and design of the patch. Is it in the right place?
• implementation: tests included? API changes? documentation?
• grammar: naming, syntax, small stuff to make the patch more readable

Types of review elements

• TODO: must be fixed before patch can be approved
• questions: may not require a change, just asking for more information or clarification (may just require a comment in the code)
• suggestions for follow-ups: Other places in the code this patch may improve

Anti-patterns

• manual processes. Again: Automate!
• irregularity. Needs to be a core part of the process. Becomes less about learning and more about "beating down." Creates a toxic environment.

Positive patterns

• share early, share often (even when incomplete). Provides extra context to help explain the problem. Helps everyone keep track of what's being worked on. Also helps build up code to complete patch.
• document the steps (what does your team expect in a good code review)
• lend a helping hand: Not all about being critical, but help them get better. What are they doing right? What needs work?
• Checklists: What needs to be right for this patch to be accepted? Keep track of the questions being asked.

As part of engineering culture

• makes it easier to ship good code
• helps people get better at what they do
• makes senior/core developers "less special"

Questions:

• how to make the most of senior's reviewing time
• pairing vs. code reviews