Looks GREAT To Me

Getting Past Bare Minimum Code Reviews

👍

@AdrienneTacke

👍

@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

Code reviews and DORA metrics

@AdrienneTacke

What are we doing wrong?

@AdrienneTacke

What are we doing wrong?

@AdrienneTacke

Reviewer

Author

What are we doing wrong?

@AdrienneTacke

Reviewer

Author

What are we doing wrong as a

@AdrienneTacke

Reviewer

Not leaving your ego at the door

What are we 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)

What are we doing wrong as a

@AdrienneTacke

Reviewer

Skimming through reviews

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

― Reviewers

[Especially after a quick LGTM]

“How did I miss that?”

― Reviewers

[What they really should be saying]

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

What are we 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."

"I think there's something wrong here."

What are we 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

What are we doing wrong?

@AdrienneTacke

Reviewer

Author

What are we doing wrong as an

@AdrienneTacke

Not being your own first reviewer

Author

What are we doing wrong as an

@AdrienneTacke

Not using any automation during development

Author

@AdrienneTacke

Not using any automation during development

Linting

Formatting

Static Analysis

Testing

What are we 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 are we doing wrong as an

@AdrienneTacke

Treating PRs as a burden

Author

@AdrienneTacke

What are we doing wrong as an

@AdrienneTacke

Tying your worth to your code

Author

What are we doing wrong as an

@AdrienneTacke

Tying your worth to your code

Author

Not being your own first reviewer

Treating PRs as a burden

Making your PR unmanageable

Not using any automation during development

How do we fix those things?

@AdrienneTacke

What are we 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

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)

When you write a comment, ask yourself:

Do you mention the person a bit more than the code?
Is the overall feeling combative, commanding, or have passive-aggressive undertones?
Does the comment add value? Will it drive productive discussion?
Can any parts of your comment be lost in translation, misconstrued, or not apply?
Does the comment contribute to or counteract developer happiness?

@AdrienneTacke

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

Do what's necessary to obtain the full context of the code changes

@AdrienneTacke

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

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

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

Please 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() should be through the library rather than a standalone declaration.

Request

Rationale

Result

Split a feature and create a separate PR

@AdrienneTacke

Could you separate the sorting behavior into its own PR?

I am having trouble reviewing both the sorting and filtering behavior in a single PR. By moving sorting into its own PR and leaving this PR focused on filtering, I can review each PR more clearly. Also, if anything breaks after these two PRs are merged and deployed, we can isolate where the problem actually lies to a smaller portion of the codebase.

Filtering behavior and sorting behavior are separated and isolated into their own respective PRs.

Request

Rationale

Result

Request a more meaningful variable name

@AdrienneTacke

Please 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

What are we doing wrong as a

@AdrienneTacke

Tying your worth to your code

Author

Not being your own first reviewer

Treating PRs as a burden

Making your PR unmanageable

Not using any automation during development

What we should be doing as an

@AdrienneTacke

Author

Be your own first reviewer

What we should be doing as an

@AdrienneTacke

Author

Make PRs manageable

@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

Let the robots take over during development!

What we should be doing as an

@AdrienneTacke

Author

Prime your PRs; it's your job

“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]

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

Prime your PRs; it's your job

Let the robots take over during development!

Make PRs manageable

Be your own first reviewer

Beyond bare minimum code reviews

@AdrienneTacke

Create a Team Working Agreement

@AdrienneTacke

If possible, use PR Templates

@AdrienneTacke

If possible, auto-assign reviewers

@AdrienneTacke

Explore informational reviewers

@AdrienneTacke

Code compliments are encouraged, within reason

@AdrienneTacke

Code Reviews & DORA Metrics

@AdrienneTacke

@AdrienneTacke

Deployment frequency

How often do you deploy to prod?

Lead time for changes

How long from code commit to code in prod?

Time to restore service

How quickly can you fix a production fire?

Change failure rate

What percentage of deployed changes cause prod fires?

@AdrienneTacke

Deployment frequency

How often do you deploy to prod?

Lead time for changes

How long from code commit to code in prod?

Time to restore service

How quickly can you fix a production fire?

Change failure rate

What percentage of deployed changes cause prod fires?

@AdrienneTacke

Deployment frequency

How often do you deploy to prod?

Lead time for changes

How long from code commit to code in prod?

Time to restore service

How quickly can you fix a production fire?

Change failure rate

What percentage of deployed changes cause prod fires?

@AdrienneTacke

Deployment frequency

How often do you deploy to prod?

Lead time for changes

How long from code commit to code in prod?

Time to restore service

How quickly can you fix a production fire?

Change failure rate

What percentage of deployed changes cause prod fires?

@AdrienneTacke

Deployment frequency

How often do you deploy to prod?

Lead time for changes

How long from code commit to code in prod?

Time to restore service

How quickly can you fix a production fire?

Change failure rate

What percentage of deployed changes cause prod fires?

@AdrienneTacke

Deployment frequency

How often do you deploy to prod?

Lead time for changes

How long from code commit to code in prod?

Time to restore service

How quickly can you fix a production fire?

Change failure rate

What percentage of deployed changes cause prod fires?

@AdrienneTacke

Deployment frequency

How often do you deploy to prod?

Lead time for changes

How long from code commit to code in prod?

Time to restore service

How quickly can you fix a production fire?

Change failure rate

What percentage of deployed changes cause prod fires?

@AdrienneTacke

Deployment frequency

How often do you deploy to prod?

Lead time for changes

How long from code commit to code in prod?

Time to restore service

How quickly can you fix a production fire?

Change failure rate

What percentage of deployed changes cause prod fires?

Don't let these metrics convince you to cut corners on code reviews.

@AdrienneTacke

Hvala vam/Salamat, Infobip Shift!

If you liked this talk, you'll love my book. Check it out!

Currently writing, with 5 out of 11 chapters available.

“”

― Yamamoto Tsunetomo

[Hagakure: The Book of the Samurai]

Looks GREAT To Me

By Adrienne Tacke

Looks GREAT To Me

Adrienne Tacke PRO

🇵🇭 Software Engineer & Sr. Developer 🥑 @ Cisco • Published Author • LinkedIn Learning Instructor • Twitch Streamer 🕹 Most important: I spend way too much money on desserts and endless hours playing Borderlands