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
Looks GREAT To Me
Getting Past Bare Minimum Code Reviews
👍
@AdrienneTacke
👋🏼 Hi Symetri!
👍
@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
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() go 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 from the filtering behavior and 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
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
If possible, auto-assign reviewers
@AdrienneTacke
Explore informational reviewers
@AdrienneTacke
Code compliments are encouraged, within reason
@AdrienneTacke
Don't settle for so-so Code Reviews,
@AdrienneTacke
strive for
GREAT ones!
@AdrienneTacke
Thank you Symetri!
Come find me if you have more questions!
Looks GREAT To Me (Symetri Kickoff 2024)
By Adrienne Tacke
Looks GREAT To Me (Symetri Kickoff 2024)
"Unlock the secrets to effective code reviews with Adrienne Tacke. Learn how to go beyond the bare minimum and create a positive environment for reviewer and author collaboration. Discover common mistakes and how to fix them."
