Code Review: It's for Me and You!

Cascadia PHP October 26, 2024

Tim Bond

👀

Who am I?

  • Senior Software Engineer
  • Frontend developer
    (when I have to be)
  • Seattle is my home
  • Cyclocross racer

Opinions ahead

Who currently does code reviews?

Why do code reviews?

  • Knowledge Sharing
  • Collaboration
  • Mentorship
  • Catching bugs*

Scope

Develop

Review

Deploy

Which code gets reviewed?

  • New code
  • Removed code
  • Changed line of code
  • Nobody commits directly to main.  Ever

Author's responsibilities

  • Clear Description

    • Summarize changes to set context for reviewers

  • Contextual Information

    • Include rationale for changes

  • Documentation Updates

Author's responsibilities

  • Link(s) to relevant items

    • Open/closed issues

    • Design documents

    • Give reviewers background information so they don't have to search for it

  • Describe why you're changing things, and less on what or how

Not Helpful

This PR fixes bugs

Helpful

This PR addresses issue #123 and resolves an error in which some users couldn't log in even when their account is active.
Previously the login method could throw an exception when user was activated manually.

Reviewer

  • Ask yourself...

Is this how we want to solve this problem?

$numbers = [1, 2, 3];
array_map(function($n) {
    global $total;
    $total += $n;
}, $numbers);

❌ Optimal way to solve this

✔️ Works

What to do as a reviewer

  • Focus on learning

  • Provide constructive feedback

  • Encourage collaboration

  • Look at the big picture

What to avoid as a reviewer

  • Any checks that can be automated

    • Linter

    • Tests

    • Issue number in the commit message

  • Bug detection

  • Personal preferences

  • Rubber stamps

The reviewer isn't looking to approve the pull request, they are looking for any reasons why they can't approve it

Who should review?

  • Everyone on the team!

Who should approve?

  • Someone with knowledge on the codebase and/or area(s) being changed (code owner)

  • Round robin

  • Balanced

  • First come, first served

The First Come, First Served Approach

  • Each dev posts to a Slack room for review

    • Use a specific mention group

    • Anyone with capacity will review

Issues

  • Must get two approvals
  • Not waiting for CI

  • Influence with words like "quick" or "easy"

  • Shopping to friends for an approval

Commenting on code or PR

  • Remain professional

  • No micromanagement

  • Give the person the benefit of the doubt

  • Ask questions: let the author work their way to the conclusion

  • Be clear when it's an optional change

The future is AI!

  • Good at summarizing the PR

  • Can find small issues
  • May not find issues in a larger context

Adding Reviews to a Team

  • Set expectations upfront

    • Every line

    • Every developer

  • Automate the automatable

  • Remember that this isn't "wasted" time; it's time you would have spent anyways getting everyone on the same page

Conclusion

  • Code reviews are a chance for everybody to learn what, why, and how changes are being made

  • Instills confidence within the team by ensuring that multiple developers collaboratively assess and agree on every line of code deployed to production

Questions