NERD Summit - March 18, 2023

Michael Lynch (mtlynch.io)

https://decks.mtlynch.io/nerds-2023/

Improve code reviews as the author

  • You’re half of the review
  • You have a significant impact on the outcome
  • Too few developers think about it

Goal

  • Become so good at code reviews that…

Your code reviewer will fall in love with you

Literally

But I don’t want my reviewer to fall in love with me

They’re going to fall in love with you

Deal with it

Why improve your code reviews?

  • Helps
    • Your reviewer
    • Your team
    • You

Learn faster

  • Directs your reviewer’s attention to important areas
  • Elicits more constructive feedback from your reviewer

Make others better

  • Sets an example for your colleagues
  • Makes your job easier when they send code to you

Minimize team conflicts

  • Code reviews a common source of friction
  • Conscientious approach minimizes arguments

What is a code review?

  • Consists of author and reviewer
  • Author sends a changelist to reviewer

What is a code review?

  • Review happens in rounds
    • Author sends changelist
    • Reviewer gives feedback

What is a code review?

  • Review ends when reviewer approves the changelist

The golden rule

Value your reviewer’s time

Value your reviewer’s time

  • Easiest thing to do is a lazy review
  • A quality review is a gift
  • Reward them for putting in the time
  • Developer time is a scarce resource

Review your own code first

Review your own code first

  • Come back to the code with fresh eyes
    • Review your code after taking a few hours away
  • Put yourself in reviewer’s shoes
    • Imagine you don’t have context as author

Review your own code first

  • Use a diff view
  • You’ll overlook changes in your editor

Review your own code first

  • Your reviewer should not see:
    • Debug code you forgot to delete
    • Extra files you accidentally included
    • Unresolved merge conflicts

Review your own code first

Which is easier?

  1. Reviewer identifies the bug
  2. Reviewer writes a note explaining the bug to you
  3. You read the note
  4. You find the relevant code
  5. You make the fix
  6. You send the revised code to reviewer
  7. Reviewer verifies the fix

Review your own code first

Which is easier?

  1. You identify the bug
  2. You fix the bug

Write a clear changelist description

  • Everything the reviewer needs should be in the code or the description

Write a clear changelist description

  • Other people might read it beyond the reviewer
    • Teammates not on the review
    • People from other teams
    • You, in a year

Write a clear changelist description

  • Explain the context around the change
    • The why not the how

Write a clear changelist description

Example bad changelist description

Move user.js

This change moves user.js to the src/controllers/auth directory.

Write a clear changelist description

Example good changelist description

Move user.js to src/controllers/auth

When we created user.js, most of its callers lived in src/lib/user.

We did a lot of restructing as part of the 4.2.x release, so now all of user.js’s clients live in src/controllers/auth, so this change moves user.js to be closer to the majority of its clients.

Automate the easy stuff

  • Review time is valuable
  • Don’t use human reviewers to:
    • Format code
    • Identify style violations
    • Identify build failures

Automate the easy stuff

Automate the easy stuff

  • Shift heavy lifting to computers with:
    • Continuous integration (CI)
    • git pre-commit hooks
    • Integrate linters/formatters in your editor

Separate functional and non-functional changes

Separate functional and non-functional changes

  • How it happens
    • Author makes a small change
    • Editor is configured to auto-format on change
    • Author doesn’t notice / care

Separate functional and non-functional changes

Changelist Review difficulty
One-line change Easy
Whitespace-only change Easy
One-line functional change within 200 lines of whitespace changes Painful
Separate functional and non-functional changes

Communicate your responses explicitly

  1. Author sends changelist
  2. Reviewer gives notes
  3. Author pushes new commits

What happens next?

Communicate your responses explicitly

Communicate your responses explicitly

Answer questions with the code itself

Answer questions with the code itself

Answer questions with the code itself

  • If your reviewer has this question, others will too

Answer questions with the code itself

  • Prevent future readers from having the question
    • Refactor code to improve clarity
    • Add code comments for things you can’t express with naming/structure

Respond graciously to critiques

  • It's about the code not about you
  • Stay objective even if your reviewer is not
  • Resist defensiveness

Respond graciously to critiques

Respond graciously to critiques

  • Reviewer catching subtle mistakes is a good sign
    • You’ve eliminated the easy stuff

Be patient when your reviewer is wrong

Be patient when your reviewer is wrong

  • A misunderstand of the code: still a red flag

Be patient when your reviewer is wrong

There are two ways of constructing a software design. One way is to make it so simple that there are obviously no deficiencies. And the other way is to make it so complicated that there are no obvious deficiencies.

-Tony Hoare

Be patient when your reviewer is wrong

  • Can you prevent future readers from misunderstanding?
    • Refactor the code
    • Add comments to clarify behavior
    • Add tests

Artfully solicit missing information

Reviewer: This function is confusing.

Artfully solicit missing information

Example bad response (defensive)

Reviewer: This function is confusing.

Author: What, exactly, is confusing about it?

Artfully solicit missing information

Example good response (agreeable)

Reviewer: This function is confusing.

Author: What changes would be helpful?

Artfully solicit missing information

  • Take a stab at it
    • Find something to improve about your code

Award all ties to your reviewer

Award all ties to your reviewer

Reviewer: I think this 8-line function would be better as two 5-line functions.

Author: I think it’s clearer as a single function.

Who’s right?

Award all ties to your reviewer

  • Reviewer has better perspective reading the code fresh
  • If both participants have equal evidence, defer to reviewer

Review

  1. Review your own code first
  2. Write a clear changelist description
  3. Automate the easy stuff
  4. Separate functional and non-functional changes
  5. Communicate your responses explicitly
  6. Answer questions with the code itself
  7. Respond graciously to critiques
  8. Be patient when your reviewer is wrong
  9. Artfully solicit missing information
  10. Award all ties to your reviewer

The golden rule

Value your reviewer’s time

Thanks!