The Art of Constructive Feedback in Code Reviews
Travis Waith-Mair
Dev Lead Community Rewards
Time for a Dad Joke
Dad Jokes are how eye-roll
What should you tell a Developer who lives in the South and doesn't use source control?
Go on and Git
Why Do We Do Code Reviews?
Identifying issues
Improving code quality
Sharing knowledge
Mentoring
Prevent Bugs
Not The Reasons Why
Why Do We Ship Code?
Solve Real Problems
Confidently Solve Real Problems
Why Do We Do Code Reviews?
Identifying issues
Improving code quality
Sharing knowledge
Mentoring
More Benefits
Prevent Bugs
What Do Code Reviews Look Like?
Types of Reviews
- Gate-keeping Code Review
- Pair Program
- Mob Program
- Code Show-n-Tell
- Request For Comment
Identifying issues
Gate-Keeping | Mob | Pairing | RFC
Improving code quality
Gate-Keeping | Mob | Pairing | Show-n-tell
Sharing knowledge
Mob | Pairing | Show-n-tell
Mentoring
Mob | Pairing
Prevent Bugs
Gate-Keeping | Pairing | Mob
You don't just have to do one or another.
Reduce distractions in the code review process when we utilize multiple reviews
Why Do We Dread Code
Reviews?
Common Complaints
"Unclear or vague comments"
"Excessive focus on style"
"Lack of context"
"Personal attacks"
"Delayed feedback"
"Not addressing edge cases"
"No Tests"
"Unrealistic expectations"
"Waiting Forever"
Humans
Common Pitfalls
Make An Agreement
Expectations are agreements made by one side without the other side's consent
- How will people know that a code review is ready?
- When should one expect to hear back after a code review is ready?
Automate what can be Automated
Linters
Auto-formatters
CI to build and run Tests
PR Templates
Style Guide
If something comes up in a code review that isn't in style guide then it should be brought up in the Style Guide not the code review.
A Style guide is an agreement
You should avoid "nit" comments.
If you can't articulate what problem is being solved is it worth commenting?
We should change this prop from `onClick` to `onClose` because it is more clear when the function is called
nit: This function could return a ternary
"Let's rename this function"
"We should do ABC pattern here"
"You should use XYZ API, it would be more safe"
Vague Comments
Let's rename this function, to something like this:
We should do ABC pattern here, like this:
You should use XYZ API, it would be more safe. We can do something like this:
Give Examples
Unclear Context
Its normal to need to find unique or unconventional solutions, but we need to document why
function getThirdPartyDataWithRetry(){
return Array.from({length:5})
.reduce(promise=>{
return promise.catch(()=>{
return thirdPartyApi()
})
}, thirdPartyApi())
}
Explain Why
/**
* API errors unexplicably, but will
* resolve after 1 or 2 times.
* This has 5 retry attempts
* just to be sure.
*/
function getThirdPartyDataWithRetry(){
return Array.from({length:5})
.reduce(promise=>{
return promise.catch(()=>{
return thirdPartyApi()
})
}, thirdPartyApi())
}
Title Text
Confidently Solve Real Problems
Aim to bring up the code a letter grade (or two)
| Grade | |
|---|---|
| A | Excellent code. Maybe a few minor thing or suggestions, but may not even be worth the time to ask for code changes. |
| B | Good code with good code coverage. Has some edge cases that needs tests and/or variable naming that could be more clear |
| C | Functions correctly with some happy path tests. Some code duplication and naming |
| D | Meets minimum functionality, but missing test and is super hard to read. Probably needs a major refactor/de-factor |
| F | It doesn't function or so unintelligible you can't be confident it will function |
Make sure to offer sincere praise
"I didn't know this API existed"
"I really like this solution. It's simple and elegant."
"This was a difficult task. Good Job."
Ping Pong is not for Code Reviews
Author
- Smaller PRs
- Ask for clarification if comments are not clear
- Jump on a call
Reviewer
- Avoid Nit picks
- Focus on bringing up a letter grade
- Do you best to send all the comments needed to pass the first time
Final thoughts
There is not a Single correct way to do a code review
We are all humans
Read your comments be for sending them.
Make sure the comments are about the code and not about the person who wrote them.
Try to use "we" instead of "you"
Remove trigger words like "just"
❌ Why don't you just refactor this function?
✅ Can we refactor this function? Like this.....
Give each other grace
QUIZ TIME
Confidently Solve Real Problems
Why Do We Do Code Reviews?
The Art of Constructive Feedback
By Justin Travis Waith-Mair
The Art of Constructive Feedback
Solid.js has an API that feels very familiar to React developers, but under the hood is completely different. Solid.js has a suite of “fine-grained reactive primitives.” It is these primitives that we will explore and learn how to use coming from the perspective of a React.js Developer.
