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
Code Review: It's for Me and You!
By Tim Bond
Code Review: It's for Me and You!
Longhorn November 4, 2023
