
Unlearning toxic behaviors in a code review culture - ingve
https://medium.freecodecamp.org/unlearning-toxic-behaviors-in-a-code-review-culture-b7c295452a3c
======
bgrainger
Previously posted under a different URL:
[https://news.ycombinator.com/item?id=16190648](https://news.ycombinator.com/item?id=16190648)

------
Nvorzula
Here's one that always has me stare at the screen, quietly, in disbelief.

1\. Carefully read and grok a small-to-medium sized module. 2\. Notice that
although the style is not the "official" style of that language, that it is
very consistent and legible. 3\. Contribute a small change in that exact same
style, such that no one would know that it was a different author if they did
not look at the git blame. 4\. Get a condescending comment from the first
author asking that you, "read and follow the official _BLANK_ style guide".

~~~
inertiatic
Well, this makes sense somewhat. Files will be edited incrementally to the
point that nothing remains from the original. If you follow your strategy, the
original style remains there until a large rewrite. If you make a point to
always follow the official style, then at some point you'll automagically have
a consistent codebase.

~~~
loeg
I disagree that incrementally converting style is helpful. It's best to be
consistent in a given file or module. Convert wholesale if so desired.

------
beaconstudios
I have a potentially controversial opinion on code reviews - that they have a
low enough signal-to-noise ratio that they provide minimal value, unless they
are conducted using a formal but living checklist/process. I strongly believe
that code reviews have to be goal-oriented or they become vectors for
excessive opinion airing (read: bikeshedding and holy wars).

I've spent way too much time getting feedback that I either disagree with and
end up arguing over, or feels completely pedantic or time-inconsiderate (e.g.
loosely-specified comments about code style during a tight deadline), that
could be eliminated by a few things:

1\. identifying and documenting the value that the code review is intended to
provide.

2\. constraining criticism to those that actively provide the intended value.

3\. where a decision point arises based on reviewer/author disagreement,
create a consensus decision within the team and document it in a standards doc
so that the debate never needs to be had again.

In my experience, when this process is followed well it leads to a feedback
loop that accomplishes the following:

1\. Generating a consensus-driven standards document based on the primary
values of the team.

2\. Creating a code review process that is exclusively aimed at "linting" new
code against the standard, and raising exceptions where the standard does not
provide a "correct" approach.

3\. Ensuring minimal discrepancy between code reviews coming from different
devs.

The process doesn't even need to be that formal - just 2 github wiki pages,
one for the standards and one for the code review process.

~~~
mindvirus
Totally agree that they should be goal oriented and checklist driven (as
should most processes in a company). Investing in tools like linters and
formatters can cut into a lot of the noise too.

~~~
beaconstudios
agreed, anything from the standards doc that can be automated (e.g. "use
airbnb style guide where possible") should be, to optimise review speed.

------
edmundhuber
Agree with all of this except 4. "asking judgmental questions".

While it's true that not all opinion is fact (uh oh -- 5. sarcasm) there is
nothing out of line in asking "Why didn’t you just do ___ here?" How you
approach this question has a lot to do with your own perspective. If you are
feeling defensive, it can seem like the code reviewer is calling you out. But
if you have experienced people reviewing your code, this is a question that
you will get a lot.

"Why didn’t you just do ___ here?" often simply means "I don't see why you
couldn't have done it this simpler way, can you explain"? It's often for
everyone's benefit that things are done in the simplest, most obvious way
possible, and it's not productive to take offense at being asked to reconsider
how you accomplished something.

~~~
ryanobjc
The problem with the question form "why didnt you just do ___ here?" is it's
loaded with the assumption that the other party should have known to do this
other thing instead.

A better approach is to use the less loaded "have you considered doing X
here?".

If you find yourself needing to demand others not take offense at your
questions, you might wish to try a different approach.

~~~
carussell
A "Have you considered..." question asks for a "yes" or "no".

A "Why[...]" question asks for why.

Assuming that "Why didn't you just do X?" isn't really just a disingenuous way
to say, "you're an idiot; do it this way instead", then you should ask for
what you want and ask the why question, not the yes-or-no question. In the
case that it _is_ a disingenuous question, then that's something that's
covered adequately by #5. Suggesting the avoidance of sarcasm is already
broadly applicable enough to cover using questions when you really mean to
make a statement. (Which is not specific to code reviews—it's as obnoxious in
real life.)

------
maxxxxx
Code reviews are hard. It takes a lot of time to really review something and
understand it. I think it's much better to discuss plans up front and get buy
in from other people before and while you write the code. Obviously that
requires an open and respectful culture.

~~~
s17n
It's good to discuss things upfront but I almost always realize stuff while
I'm implementing that I hadn't completely forseen.

~~~
maxxxxx
Let's just say having some continuous dialog while developing is preferable
over review after everything is done. I have that kind of relationship with a
few people and it works great.

~~~
ThrustVectoring
In a healthy environment, sure. In an unhealthily micromanaged one, you may
want to push the review to the end so that you can document that the review
process with this person took an extraordinary amount of time and delayed
rollout of a feature.

~~~
maxxxxx
You are an evil genius :-). and sadly that happens quite a bit.

------
s17n
Code review should be informal and conversational, these guidelines are the
opposite of that. I'd say the real problem here is people not having the
rapport neccesary for a real conversation.

~~~
anoncoward778
PLEASE READ THE POST. "informal and conversational" are EXACTLY what this
poster posits. The problem is that WHAT is considered "conversational" is
subjective without exact and precise examples to guide by. The poster provider
these examples in extreme detail both for Negative and positive examples.
PLEASE READ THE POST.

~~~
seattle_spring
I hope your code reviews are more humane and less condescending than this
comment.

------
jiveturkey
disagree with unhelpful #1. it is understood that “facts” are opinions. the
reviewer doesn’t need to write an essay _unless_ he gets pushback from the
author. the author needs to understand that the presentation style is just a
shorthand.

agree with #2 in theory but in practice some authors are crazy sloppy. i usu.
write “here and elsewhere” the first 2 times for a given author and after that
i break out the copy paste bazooka.

his example for #2 is also terrible. “trailing whitespace” is perfectly
sufficient review comment. you don’t need to write a novel and both parties
have to understand it isn’t rude, it’s shorthand because life doesn’t need to
be spent in code review.

likewise, disagree with unhelpful #7. as reviewer, if a comment wasn’t replied
i get it. fine. move on. I’ll track authors and learn which ones ignore
because there’s no value in arguing, or those that ignore because of not
getting it in general or not getting that a specific comment was incorrectly
thought trivial (and ou also have to consider if you played a part). jesus,
you can’t take it personally if your comments are ignored. it’s shorthand. i
don’t want to go through and read tons of “yes ok” it’s a waste of both
parties’ time.

i agree with all the rest!!

although, some of the behaviors he talks about are indicative of toxic
_culture_ and it’s just expressed in code review. more important to fix that
(or leave) if that’s the case.

~~~
repsilat
Mostly I think this is a mix of (1) "good advice for dealing with junior devs"
and (2) "good advice for dealing with people new to a technology."

For #1 don't hurt their feelings, for #2 teach them instead of expecting them
to know.

Going through the points in the article with these lenses:

 _" This component should be stateless"_ is reasonable feedback to give to
someone familiar with React. Maybe they had a good reason for not making it
stateless, maybe they made a mistake, but giving an impromptu lesson and
providing references probably isn't appropriate -- it's condescending. Again,
fine for someone new to the technology. Audience is important.

The "Avalanche of Comments"... As someone who doesn't really do it, and as
someone who has been on the receiving end of it, I always appreciated the
thoroughness. It's not judgmental, it's a checklist, and it's helpful.

"Spew emojis" hit this taxonomy too. If my code is ugly enough to warrant one
(and it sometimes is), I want to know it has crossed a line, and someone
telling me that I am a terrible person who has ruined their day (or given them
cancer) is a perfectly reasonable, light-hearted way to do so. Constructive
feedback is useful too, but for a senior dev they probably knew it was ugly
and just need telling that it was _too_ ugly and that they should try harder.
Obviously not appropriate feedback for junior devs, though.

My disagreement with #3 doesn't fit that taxonomy though, mostly I just hate
process. Most refactors are super fast, fairly mechanical jobs. Both you and
the reviewer have that code in your cache, and it will likely never be a
priority again. Take some initiative and rewrite the thing, it'll only take
20-30 minutes. Making a ticket increases debt instead of paying it down.

------
baby
Do companies usually have guidelines and maybe even training on how to do
peer/code review? With some emphasis on the empathy and the way you
communicate your thoughts? I've noticed a broad range of peer/code review in
my little career and in hindsight it was probably due to the fact that no one
really knew how to give a peer/code review.

------
freedomben
Great post. I would add a caveat that personal relationships can change the
rules a bit. When working with developers that I know very well, and that know
me (that's important too) super brief comments, and even sarcasm, can be
helpful. A friend and I were working on a nasty piece of code and we used the
barf emoji as a way of commiserating :-)

------
finnthehuman
This kind of article dances around it's point in the unproductive way typical
of the topic. Here's a condensed version of issue, rather than talking about
symptoms:

The topic at hand is harshness of language used providing feedback on another
person's work. In addition to being a developer, I've also worked in garages
where people swore like sailors and ball-busting was common. Calibrating my
voice for the context starts with understanding every critique has: the
harshness intended, the harshness communicated, and the harshness received.

This article focuses on the 2nd, leaves out the fact the internet makes it
notoriously difficult to communicate voice through text, and assess using the
common buzzword "toxic." Which, as far as I can tell based on context clues,
is intended to mean "anything harsher than my personal preference threshold."

Just as important are parts 1 and 3. The feedback "pull your head out of your
ass" can be run of the mill, or beyond the pale. It all depends on the context
and participants. Note: This is very much NOT a point about "having thick
skin," it's about understanding how sharp the comment is in the first place.

Each critique also has: the amount of ego hit to the recipient intended, and
the amount of ego hit perceived. This one is about "having a thick skin," and
where we might ask, "How harsh is too harsh, depending on [bucket of other
factors provided by the context]?"

I'm not looking for a dissertation, but I expect people speaking on the topic
to show an understanding of the complexity at hand. Otherwise it looks like a
pack of schoolmarms trying to enforce I-know-best standards.

A piece of career advice I received can be paraphrased as: "Software
development in Silicon Valley is full of people from the participation trophy
generation that have only ever held back-office jobs. And it's San Francisco."
\-- That's all that needs to be said. From that we can derive what the
standards for behavior in code reviews are.

And if it sounds like I'm implying that the hegemony in software development
demands we treat each other with kids gloves, it's because I am.

------
pfarnsworth
One thing that I ask people on my team to do is only have one set of review
comments. What I hate is when you address a bunch of code reviews, and then
the same reviewer comes and finds more things to change. Then it's a never-
ending cycle of fix and change, fix and change, which wastes everyone's time.

Go through your code review thoroughly, and only make one set of comments. Of
course, if you happen to find a huge bug then that necessitates more reviews.
However, I think that reflects poorly on the reviewer, she should have spent
more time reviewer more carefully the first time.

~~~
zo1
I found this only to be a problem if the person who wrote the code being
reviewed is slow at writing/changing code. It shouldn't be a big deal:
Checkout, fix, re-run tests, commit, push, tick off task.

However, if the above process on the developer's side is not quick, then no
matter if the comments are batched together, the whole thing will take a long
time. This is usually a sign that the wrong person is doing this specific
module, or that they need more mentoring/training/firing.

------
mhotchen
Another I've noticed is not reading the ticket. I've seen a lot of reviewers
not take the time to look any deeper than the code at face value so the
feedback they provide is low hanging fruit stuff. My favorite feedback is when
someone helps me understand the ticket better, or spots potential risks that I
hadn't seen.

------
pfarnsworth
One other thing the code reviewee needs to do is develop a thick skin.
Reviewers never be assholes, because it's unproductive and makes your
coworkers not want to work with you.

But reviewees should also learn how to take criticism well, and develop the
thick skin to shrug off when the reviewer actually is being an asshole. You
can't go through life never expecting to encounter an asshole or someone who
likes to bully people. Better to know how to shrug off shitty behavior rather
than become obsessed with how your feelings were hurt.

~~~
ubernostrum
"Grow a thicker skin" is, usually, identical to "let's just let people be
assholes to you and say it's your fault if you complain about it".

So here's a thought: if you see someone being an asshole, and you're thinking
about blaming anyone _other_ than the asshole, don't. Instead, work on making
it untenable to be an asshole.

~~~
pfarnsworth
It's never okay to be an asshole. End of story.

But you will also encounter hundreds of assholes in every aspect of your life.
If every time you encounter an asshole, you break down and cry and can't go on
with your day, the only one that suffers is you, not the asshole.

Also, fearing hurting someone's feelings becomes the lowest common
denominator, and leads to everyone walking on eggshells.

Case in point, a fresh grad had her code reviewed by the team, and it was
completely professional and non-judgemental. She burst into tears at her desk
because she had never gone through this and took the criticism personally.

Were we too harsh, or was she too sensitive? If people burst into tears
because of professional code reviews, do we lower the bar as a team to account
for her sensitivity, or does she raise her bar and thicken her skin? And who
determines what that line is?

~~~
ubernostrum
_you will also encounter hundreds of assholes in every aspect of your life_

Which is the same as "I'm not going to do anything about this asshole, because
there'll always be assholes".

Given that approach on your part, one wonders what it really was that caused
the "too sensitive" new member of your team to react.

(hint: your new team member wasn't "too sensitive", you've probably just been
tolerating assholes on your team -- because "what can you do?" \-- for so long
you've forgotten what assholes they are)

~~~
pfarnsworth
No it's not the same.

And hint, the reviews were perfectly fine and professional.

