Looks GREAT To Me

Getting Past Bare Minimum Code Reviews

đź‘Ť

@AdrienneTacke

👋🏼 Hola J on the Beach!

đź‘Ť

@AdrienneTacke

đź‘Ť

@AdrienneTacke

L

G

T

M

@AdrienneTacke

Looks

Good

To

Me

What are we doing wrong?

How do we fix those things?

Beyond bare minimum code reviews

1

2

3

@AdrienneTacke

What are we doing wrong?

@AdrienneTacke

What are we doing wrong?

@AdrienneTacke

Reviewer

Author

What are we doing wrong?

@AdrienneTacke

Reviewer

Author

What we're doing wrong as a

@AdrienneTacke

Reviewer

Not leaving your ego at the door

What we're doing wrong as a

@AdrienneTacke

Reviewer

Focusing on the developer rather than the code

Detecting interpersonal conflict in issues and code review: cross pollinating open-and closed-source approaches

Qiu, H. S., Vasilescu, B., Kästner, C., Egelman, C., Jaspan, C., & Murphy-Hill, E. (2022, May)

Detecting interpersonal conflict in issues and code review: cross pollinating open-and closed-source approaches

Qiu, H. S., Vasilescu, B., Kästner, C., Egelman, C., Jaspan, C., & Murphy-Hill, E. (2022, May)

"the use of second-person pronouns at the beginning of the sentence is more likely to be impolite"

Detecting interpersonal conflict in issues and code review: cross pollinating open-and closed-source approaches

Qiu, H. S., Vasilescu, B., Kästner, C., Egelman, C., Jaspan, C., & Murphy-Hill, E. (2022, May)

Detecting interpersonal conflict in issues and code review: cross pollinating open-and closed-source approaches

Qiu, H. S., Vasilescu, B., Kästner, C., Egelman, C., Jaspan, C., & Murphy-Hill, E. (2022, May)

unigram

bigram

ngram

z-score

z-score

z-score

Toxic

you

people

even

do

what

is

want

your

because

why

it is

you want

that is

going to

you are

trying to

if you

to do

do not

you think

12.172

7.292

7.097

6.71

6.644

6.373

6.078

5.796

5.657

5.547

5.555

4.81

4.272

4.256

4.187

4.053

3.682

3.668

3.556

3.539

if you want

it is not

do you think

you need to

3.397

2.712

2.576

2.397

Non-toxic

tests

unit

vs

file

files

for

test

from

at

line

could you

the pull

as the

and the

of files

we can

pull request

code to

to the

instead of

-4.773

-4.858

-4.982

-5.15

-5.165

-5.574

-5.76

-5.872

-6.732

-6.782

-2.815

-2.889

-3.137

-3.143

-3.296

-3.48

-3.668

-3.856

-4.031

-5.004

the pull request

-2.276

Statistically more likely in "toxic" comments

Statistically more likely in "non-toxic" comments

unigram

bigram

ngram

z-score

z-score

z-score

Toxic

you

people

even

do

what

is

want

your

because

why

it is

you want

that is

going to

you are

trying to

if you

to do

do not

you think

12.172

7.292

7.097

6.71

6.644

6.373

6.078

5.796

5.657

5.547

5.555

4.81

4.272

4.256

4.187

4.053

3.682

3.668

3.556

3.539

if you want

it is not

do you think

you need to

3.397

2.712

2.576

2.397

Non-toxic

tests

unit

vs

file

files

for

test

from

at

line

could you

the pull

as the

and the

of files

we can

pull request

code to

to the

instead of

-4.773

-4.858

-4.982

-5.15

-5.165

-5.574

-5.76

-5.872

-6.732

-6.782

-2.815

-2.889

-3.137

-3.143

-3.296

-3.48

-3.668

-3.856

-4.031

-5.004

the pull request

-2.276

Detecting interpersonal conflict in issues and code review: cross pollinating open-and closed-source approaches

Qiu, H. S., Vasilescu, B., Kästner, C., Egelman, C., Jaspan, C., & Murphy-Hill, E. (2022, May)

What we're doing wrong as a

@AdrienneTacke

Reviewer

Skimming through reviews

@AdrienneTacke

“The developer has a bug in their code. It's their fault.”

― Reviewers

   [Especially after a quick LGTM]

What we're doing wrong as a

@AdrienneTacke

Reviewer

Writing ineffective comments

@AdrienneTacke

Writing ineffective comments

Subjective

Unclear

Lack an outcome

"Return the result in an array instead?"

"This could be better."

"There's something wrong here."

What we're doing wrong as a

@AdrienneTacke

Reviewer

Writing ineffective comments

Skimming through reviews

Focusing on the developer rather than the code

Not leaving your ego at the door

How do we fix those things?

@AdrienneTacke

What we should be doing as a

@AdrienneTacke

Reviewer

Forget your ego

What we should be doing as a

@AdrienneTacke

Reviewer

Focus on the code, not the developer

Detecting interpersonal conflict in issues and code review: cross pollinating open-and closed-source approaches

Qiu, H. S., Vasilescu, B., Kästner, C., Egelman, C., Jaspan, C., & Murphy-Hill, E. (2022, May)

@AdrienneTacke

"we see many software engineering-related n-grams, e.g., "tests" and "the pull", among non-toxic comments but almost none among toxic comments"

@AdrienneTacke

Detecting interpersonal conflict in issues and code review: cross pollinating open-and closed-source approaches

Qiu, H. S., Vasilescu, B., Kästner, C., Egelman, C., Jaspan, C., & Murphy-Hill, E. (2022, May)

Detecting interpersonal conflict in issues and code review: cross pollinating open-and closed-source approaches

Qiu, H. S., Vasilescu, B., Kästner, C., Egelman, C., Jaspan, C., & Murphy-Hill, E. (2022, May)

unigram

bigram

ngram

z-score

z-score

z-score

Toxic

you

people

even

do

what

is

want

your

because

why

it is

you want

that is

going to

you are

trying to

if you

to do

do not

you think

12.172

7.292

7.097

6.71

6.644

6.373

6.078

5.796

5.657

5.547

5.555

4.81

4.272

4.256

4.187

4.053

3.682

3.668

3.556

3.539

if you want

it is not

do you think

you need to

3.397

2.712

2.576

2.397

tests

-4.773

could you

-2.815

unit

-4.858

the pull

-2.889

vs

-4.982

as the

-3.137

file

-5.15

and the

-3.143

Non-toxic

files

-5.165

of files

-3.296

for

-5.574

we can

-3.48

test

-5.76

pull request

-3.668

from

-5.872

code to

-3.856

at

-6.732

to the

-4.031

line

-6.782

instead of

-5.004

the pull request

-2.276

What we should be doing as a

@AdrienneTacke

Reviewer

Proper, thorough reviews

Review code in 25 - 45 minute focused bursts

@AdrienneTacke

Don't be afraid to send back PRs that are too large to review

@AdrienneTacke

“How did I miss that?”

― Reviewers

   [What they really should be saying]

“The developer has a bug in their code. It's their fault.”

“The developer has a bug in their code. It's their fault.”

What we should be doing as a

@AdrienneTacke

Reviewer

Write effective comments

Effective comments are objective, as specific as possible, and have a focused outcome.

@AdrienneTacke

As reviewers, we need to be objective.

@AdrienneTacke

As reviewers, we need to be objective.

@AdrienneTacke

Suggest with facts.

"Before asking for a change, ask yourself why you want to make that change."

Target the code, as presented to you.

Code Review Comments: Language Matters, Vasiliki Efstathiou and Diomidis Spinellis (2018)

As reviewers, we need to be specific.

@AdrienneTacke

As reviewers, we need to be specific.

@AdrienneTacke

Be clear if something needs to be done

Comment Signal Use For
needs change: - Smaller changes and fixes that can be resolved in a single commit.
- Small updates to current PR.
needs rework (let's discuss offline): - Larger changes that require major rework or refactoring.
- Typically, these initiate an offline discussion as there’s a lot to discuss.
nitpick: - Purely subjective commentary that does not affect the code.
- If you must add a nitpick, clearly identify it as such. But, try not to include nitpicks at all!

As reviewers, we need to be specific.

@AdrienneTacke

Be clear if something needs to be done

As reviewers, we need to be specific.

@AdrienneTacke

Pinpoint what you're talking about

Only highlight relevant code you are discussing

Explicitly call out method/variable/class/etc. names

If necessary, call out line numbers to be more succinct

As reviewers, we need to provide a focused outcome.

@AdrienneTacke

Code review comments: language matters

Efstathiou, V., & Spinellis, D. (2018, May)

@AdrienneTacke

Code review comments: language matters

Efstathiou, V., & Spinellis, D. (2018, May)

@AdrienneTacke

"Apparently, the useful comments contain mostly verbs that denote some kind of transformation..."

Triple-R Pattern

@AdrienneTacke

©

Triple-R Pattern

@AdrienneTacke

A sentence explaining what you’d like the author to do.

A sentence or two explaining why the request is warranted. Include references or links to justify and support your request.

A measurable end state the author can compare their change to. This could be a metric to reach, a screenshot of an intended state, or quite literally, what the code should look like after the change.

Request

Rationale

Result

Moving a method

@AdrienneTacke

Can we move the AuthenticateUser() method into our AuthenticationUtilities library.
Similar methods are in the AuthenticationUtilities library (like ReauthenticateUser() and AuthenticateThirdPartyUser()). AuthenticateUser() is also called more than a few times, which warrants its place in the library. 

Lastly, our Team Working Agreement advises us to place any authentication behavior within the AuthenticationUtilities library.
After this change, calls to AuthenticateUser() go through the library rather than a standalone declaration.

Request

Rationale

Result

Request a more meaningful variable name

@AdrienneTacke

Can we rename the variable "item" to something that is less ambiguous.
The variable name "item" is vague and provides little context within its scope. Specifically, the notion of discounts being applied to this object is lost, making the function a bit unclear. 
Variable name "item" is changed to something more meaningful; suggestions include "discountEligibleItem" or "discountEligibleProduct".

Request

Rationale

Result

What we should be doing as a

@AdrienneTacke

Reviewer

Forget your ego

Focus on the code, not the developer

Proper, thorough reviews

Write effective comments

We know what we're doing đź‘Ť

@AdrienneTacke

Reviewer

Author

Now, what are we doing wrong?

@AdrienneTacke

Reviewer

Author

What we're doing wrong as an

@AdrienneTacke

Not being your own first reviewer

Author

What we're doing wrong as an

@AdrienneTacke

Not using any automation during development

Author

@AdrienneTacke

Not using any automation during development

use(rule);

let lint = "linting";

forMATt-ing

for (var i = 0; i < 10; i--) {
console.log("Static Analysis");
}

❌ Testing

Linting

Formatting

Static Analysis

Testing âś…

What we're doing wrong as an

@AdrienneTacke

Making your PRs unmanageable

Author

@AdrienneTacke

Making your PRs unmanageable

Multiple parts of a large feature are in a single PR

Changes

aren't

atomic.

Too many files

What we're doing wrong as an

@AdrienneTacke

Treating PRs as mystery novels

Author

0

@AdrienneTacke

From "Looks Good To Me: Constructive Code Reviews" by Adrienne Braganza

What we're doing wrong as an

@AdrienneTacke

Tying your worth to your code

Author

What we're doing wrong as an

@AdrienneTacke

Tying your worth to your code

Author

Not being your own first reviewer

Treating PRs as mystery novels

Not using any automation during development

Making your PRs unmanageable

How do we fix those things?

@AdrienneTacke

What we should be doing as an

@AdrienneTacke

Author

Be your own first reviewer

What we should be doing as an

@AdrienneTacke

Author

Let the robots take over during development!

What we should be doing as an

@AdrienneTacke

Author

Make PRs manageable

(AKA make them smaller)

@AdrienneTacke

Manageable PRs

Focus on an atomic change or feature

Include only what's necessary for the code changes to work

Split multiple parts into multiple reviews

Likely can be reviewed in 10 - 20 minutes

Typically under 500 lines of code*

Typically less than 20 files changed*

*Based on study by Dragan Stepanović

*Based on study by Microsoft

What we should be doing as an

@AdrienneTacke

Author

Solve the mystery; let your PRs tell the story

“When one is writing a letter, he should think that the recipient will make it into a hanging scroll.”

― Yamamoto Tsunetomo

   [Hagakure: The Book of the Samurai]

From "Looks Good To Me: Constructive Code Reviews" by Adrienne Braganza

@AdrienneTacke

What we should be doing as an

@AdrienneTacke

Author

Remind yourself: you are NOT your code

What we should be doing as an

@AdrienneTacke

Author

Remind yourself: you are NOT your code

Solve the mystery; let your PRs tell the story

Let the robots take over during development!

Make PRs manageable

Be your own first reviewer

We all know what we're doing đź‘Ť

@AdrienneTacke

Reviewer

Author

Beyond bare minimum code reviews

@AdrienneTacke

Create a Team Working Agreement

@AdrienneTacke

If possible, use PR Templates

@AdrienneTacke

# Bug Fix PR Template
—--

## What’s happening?
Provide a sentence or two to describe the bug.

## Reproduction Steps
Outline the steps you took to reproduce the bug. Be sure not to forget any steps and that they are in the right order!

1.
2.
3.

## Root Cause
What was the root cause of the bug? How did you identify it?

## Fix
Explain the fix in a sentence or two. Then, go into detail about **why** this fix solves the problem.

## Testing Strategy
How was the fix tested to make sure the bug is truly resolved? 

## Impact
Are there any implications this fix may have on other parts of the application? 

## Regression Risk
Are you confident this fix does not introduce new issues? If you think this fix may have some regression risk, what steps have been taken to mitigate it?


## Bug Fix Checklist

- [ ] Issue # is linked
- [ ] Fix has been tested locally and works
- [ ] New unit tests have been added to prevent future regressions
- [ ] (If applicable) Relevant documentation has been updated


## Notes
Is there any other context or information to share in order to understand the fix or its impact?

## Thanks for (hopefully) squashing this bug!

Example: Bug Fix PR Template

If possible, auto-assign reviewers

@AdrienneTacke

Example: Auto-assign reviewers via reviewer policy in Azure DevOps

Explore informational reviewers

@AdrienneTacke

Code compliments are encouraged, within reason

@AdrienneTacke

Don't settle for

Code Reviews,

@AdrienneTacke

strive for GREAT ones!

¡Muchas gracias

J on the Beach!

Come find me if you have more questions!

(or if you know a good coffee/bakery spot in Málaga!)

or "X", whatever

@AdrienneTacke

If you liked this talk, you'll love my book. 

Check it out!