The Art of Constructive Feedback in Code Reviews

Travis Waith-Mair

https://www.bedrock-layout.dev

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

  1. How will people know that a code review is ready?
  2. 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?

Made with Slides.com