
Unlearning Unhelpful Behaviors in a Code Review Culture - stablemap
https://medium.com/@sandya.sankarram/unlearning-toxic-behaviors-in-a-code-review-culture-b7c295452a3c
======
nartz
Some sensible ideas - however, I'm on the fence about this one: "Unhelpful
behavior: asking engineers to solve problems they didn’t cause"

Generally, I agree with breaking out bugs or issues that are found 'along the
way' into separate subtasks or tickets. However, often what happens is a
ticket is thrown into the backlog and not addressed.

Because the developer has just touched the code, this is often the best time
to refactor as its still fresh in the mind, and this creates a culture of
constant improvement. Of course everything depends, in this case, the
complexity of the fix is the main limiting factor.

We truly believe in _Refactoring along the way_ instead of trying to break out
tech debt stories separately.

~~~
kelnos
I'm a little torn on this as well, but I think I'm in the author's camp:

1\. The person making the change to the code for their purpose may not have
the required overall understanding to execute a refactor.

2\. I believe in a One Change At A Time policy: I'd like to be able to, at
least in theory, measure the effects of any change. If you slap two changes
together, that becomes difficult or impossible.

3\. The person making the change has a purpose in making that change, and
likely has committed to some sort of schedule or timeline for getting the work
done. Forcing them to play janitor could easily derail that.

4\. The change at hand might just be a small piece in a larger body of work,
and shifting focus to a refactor would be disruptive.

Having said that, I _do_ often take the initiative in doing refactors when
making a change, if it makes sense to me to do so. But I don't think it makes
sense to push that expectation on others. Hell, it's possible/likely that the
person making the change has noticed the opportunity for a refactor, but has
decided against it for whatever reason.

I might suggest a refactor in a code review, but couched with language that
it's entirely optional, and won't block merge of the change as-is if the
author decides it's not the right call for them.

~~~
le-mark
_I believe in a One Change At A Time policy: I 'd like to be able to, at least
in theory, measure the effects of any change. If you slap two changes
together, that becomes difficult or impossible._

One change at a time has the benefit of not polluting traceability of what was
done and why, in the ticketing system. If a commit for ticket1234 was to fix a
bug in the payment system, and the commit has unrelated code fixing up user
data access, that's bad in my opinion.

I've found myself in many code bases where the only trail of breadcrumbs I've
had is ticket references in the commit messages. When these aren't reliable,
you're left with not much at all.

------
zumu
I find the use of "Toxic" to be somewhat inflammatory and hyperbolic. I would
say this would more accurately be titled as "Improving your Code Review
Interpersonal Skills".

I wholeheartedly agree with the suggestions -- they are all great ways to be
sensitive to your fellow coworkers, and I tend to favor this style. However, I
know a lot of great developers who have a more direct style of communication.
Also, I know several people who might interpret similar comments as
patronizing or passive aggressive.

~~~
mpweiher
In fact, the article seems to be more guilty of the transgressions discussed
than the transgressions discussed.

For example: "passing opinion as fact". And of course, calling these things
"toxic". In fact, I disagree. Sometimes a little tough-love is needed, and
this is the _code_ we are talking about, not the person.

I'd suggest the author look up "egoless programming". Her suggestions seem to
pander to ego-full programming, and this will mask underlying issues and allow
them to fester, causing significantly greater problems in the future.

UPDATE: And of course, something unqualified is always an opinion (for a
hilarious artistic rendition of this point, see the judge on _The Good Wife_
who requires lawyers to append “in my opinion” to everything they say)

~~~
burntsushi
Can you state specifically which of the transgressions this article mentions
that you would consider appropriate tough love, and how exactly they improve
the situation? Examples would help.

~~~
gurkendoktor
I'm not sure if this is related but maybe egoless programming is what I am
thinking about here... I felt uneasy reading the OP. I follow much of the
advice myself, and I can see how it is necessary to be diplomatic when there
are tensions in the team.

However, I much prefer it when people give me their raw and direct thoughts on
the code. "indentation wrong", "really hard to follow this code", "why does
this class exist?"; now we're two developers looking at code, and we both want
it to be as neat as possible.

If I can tell that the reviewer goes out of their way to be diplomatic (e.g.
by following the OP or by randomly praising code that is not broken), they're
obviously worried about hurting me; and suddenly we are not managing my code,
but _me_! Why? Am I on the brink of being fired? Did I ever overreact? Am I
too attached to my source code? It also becomes impossible to tell how
strongly the other side actually feels about their suggestions when everything
is posed as a "what do you think" question.

It could be a cultural thing too? I'm German and American politeness often
makes me paranoid because every flowery statement could be criticism in
disguise.

~~~
jdmichal
I think that all your example comments are entirely appropriate. And, in the
case of, "Why does this class exist?", I think it's extremely important that
such questions are asked. In my opinion, knowledge transfer is one of the
primary functions of code review in the first place. And that question is as
much about knowledge transfer as it is about the code. Maybe there's a
consideration that led you to create that class, and this is a chance for that
other person to learn it. And maybe you will learn something back about
another potential solution.

I think the real negative that needs to be avoided is using code reviews to
attempt to force people to do things exactly the way you would do them. I
think many of the articles points kind of go towards this, but indirectly.
Diversity of thought and approach is a good thing, and learning to live code
that's not done in the fashion that _I think_ is ideal was a lesson that took
quite a while for me.

EDIT: And, to be clear, "Why does this class exist?" is a potential lead-in to
such a problematic viewpoint. The problem happens when, after explanation of
the class' existence, the response turns into, "You should remove this class."
That's where it transitions away from knowledge transfer and into forceful
monoculture of approach.

------
ironjunkie
The most toxic behavior is being overly protective or offended about your
code. This goes both ways: Both as the review-seeker (that opened a PR) and
the reviewer (That most of the time got his original code modified by the PR).

As a general rule I think there is too much emotion in what should be 100%
technically driven. And again, this goes both ways. Being judgemental in a
review (like on some of the link's slides) or being offended by a technical
comment on the other side. Too much sugar coating is not helping anyone

~~~
kelnos
> _The most toxic behavior is being overly protective or offended about your
> code._

I disagree with "most", but this is absolutely often an issue. Often people
take criticism of their code as criticism of themselves and their abilities,
which is always counterproductive.

> _As a general rule I think there is too much emotion in what should be 100%
> technically driven._

In my experience, things that are 100% technically driven are vanishingly
rare. Most decisions come down to balancing trade offs and opinions, after
examining the actual facts and evidence at hand.

Regardless, people have emotions and don't always act fully rationally,
despite concerted efforts to do so. That's just a fact of human nature, and
we'd do well to design our professional processes with that in mind.

~~~
thedz
> I disagree with "most", but this is absolutely often an issue. Often people
> take criticism of their code as criticism of themselves and their abilities,
> which is always counterproductive.

I think it's easy to fall into "this comment is a value judgement on my
skill/person, and not my code" if code review feedback is targeted at the
person and not the code.

Hence the discouraged "why did you do this" form vs the more helpful "this
might be better because" format.

------
darkerside
I find that focusing on negative behaviors and "what not to do" can have some
undesired side effects. Namely, discouraging people from communicating because
they don't want to hurt someone's feelings or undervalue their own ideas. If
you want to leave a comment, there's probably a great reason! Shutting
yourself down does nobody any favors. I liked that there were a few ideas
about helpful behaviors at the bottom, but they were less detailed and didn't
include examples in the same way.

Over-communicate, notice positives, and build a culture of trust, where
feedback is seen as the gift that it is. Those are the secrets to great code
reviews... IMO

~~~
TimPC
I think it’s far better to develop a culture where code reviews are opinions
and suggestions and if you disagree you can reply to a comment and discuss
further. Using aggressive tone is not helpful for this, but documentation
needed to correct any mistake is unhelpful as well. If it’s hard to ask for
additional information or hard to disagree with a review comment that isn’t
going to be solved by increasing the quality of review comments. In fact I’m
more likely to be upset if I make a mistake in my code and someone assumes
it’s intentional, explains why it’s a mistake, and then provides documentation
to explain further. That feels like in bold, triple underline, all-caps text
to me. From my perspective a better comment might be “Let’s discuss further as
I prefer alternative.” Highlighted on appropriate line of code with a this
made issue specific. A lot of the time the reaction will be ‘oh, oops, so do
I.’ The other times you spend the time on a discussion rather than carefully
crafting a written statement (which is far more time consuming).

------
robotpony
Most of these points all relate to having empathy for the developer and being
kind, and focusing on the point of code reviews (which is to improve quality
and reduce ongoing costs).

Example:

> Unhelpful behavior: overwhelming with an avalanche of comments

Code that isn't consistent to standards absolutely needs to be fixed. And
while it's a lot of work to keep code up to standard, the author is correct
that the nitpicking approach is unproductive.

A better approach is to review code like this, see that it falls short, and
suggest realistic (and in turn kind) ways to improve things. Picking at each
missed space is clearly counter productive, and there are more standard and
helpful solutions.

Why not suggest adding:

\- lint tools \- requiring code meets standards \- auto formatting tools

I don't think things like minutia fit in code reviews either, but the small
stuff does matter.

I feel the same about the other suggestions, they can be boiled down to: don't
be an ass; be kind; be constructive. Just imagine that you're reviewing your
own code from 5 years ago.

~~~
Domenic_S
She did suggest those things further down the article:

> _Helpful Behavior: automate what can be_

> _Reviewing issues that can be caught by linters, git hooks, or automated
> tests are unhelpful because they often result in an avalanche of comments
> and come off as nitpicking. People are not particularly good at catching
> these issues, hence why automation tools exist._

------
whiddershins
I feel that this could be recontexualized for the person who is receiving
feedback.

If you don’t take things pwrsonally, and you take things one at a time,
there’s no reason to let yourself feel overwhelmed or attacked in many
situations where at first you might be inclined to do so.

Specifically I disagree with point #2. I absolute love it when testing/QC
labels every single instance of a minor mistake. It saves me from having to go
through and find each place where it happened, and it lets me check off a
little Asana task as complete each time I make a minor correction.

Feedback like “it looks like you made this mistake a number of times” can be
just as condescending, with the added frustration that I now need to go try to
find everywhere I might have made the mistake.

When they do the work of naming each instance, they are saving me time.

So I’m wondering how much of this is about putting the burden of not feeling
attacked on the reviewer, and neglecting the role of the reviewee?

------
lifeisstillgood
> Why didn’t you just do ___ here? as opposed to "i think xxx is a better
> approach because"

This is such a good and deep point - being forced to defend your decisions is
one thing - using a form such as "the whole world knows not to do this why
don't you" is another.

i would argue that there is value in developing a longer for review comments -
it's probably viable in fact - such that the comment is non judgemental.

~~~
burntsushi
As a good rule of thumb, I've managed to set a trigger in my brain that goes
off whenever I use the word "just." I've been a recipient of that word too
many times to not know better. It's amazing how much "just" can cover up.

It is perhaps mostly orthogonal to the specific phrasing used here, and
certainly the word "just" isn't in and of itself bad. But my feeling is that
my technical writing has improved dramatically if I carefully consider each
use of it. It often exposes a perspective gap that I have with my target
audience.

------
misterbowfinger
These are good guidelines, I generally agree with all them (and I've messed up
tons of times as well). It's important to note that you should write comments
for the developer, whoever that is. That means understanding their
perspective, their experience, and what they want.

Some developers really won't take offense to many of the "bad comments"
suggested, and could even feel patronized if you take some of the guidelines -
for example, over explaining something could be overkill for someone who just
made a mistake.

I've also found that taking a lighthearted approach to code reviews really
lightens up a stressful interaction. Simple jokes, comments like "dead code!
woo!", and whatnot make a difference.

And lastly - don't forget positive comments!! Celebrate people's work when you
can!!

------
thedz
Something that's been on my mind is the value of code reviews as a line-by-
line exercise, vs the value of a code review as a round table.

I find line by line reviews useful for small PRs of limited scope (bug fixes,
etc), but find it a difficult space to talk about things like approach,
architecture, etc.

For the latter, I've found round tables where the dev in question have the
time and space to explain their reasoning in a synchronous real-time manner to
be valuable.

------
austincheney
While that is certainly excellent good advise it is centered on online code
reviews, such as those in a git pull request. Although this advise is
excellent, these aren't hard problems to solve. Particularly since if a
submitter is out of line a collaborator or moderator can easily shut them down
or close the pull request. Case closed (over simplification).

Far more challenging are offline code reviews where you have to deal with
people face to face and contend with people's emotions. If everybody were
mature and confident emotion wouldn't be a serious factor, but this isn't the
case in the corporate world.

People are frequently guarded, offensive, or deceptive most usually without
any awareness of their emotional communication. It takes a special kind of
soft skill to get people to open up, be relaxed, and attain greater
disclosure. This can be particularly challenging among young people who may be
more emotional, face greater technical hurdles, and lack the communication
experience to contend with emotional pressure.

~~~
kelnos
I have the exact opposite opinion and experience.

I think these _are_ hard problems to solve. We haven't solved them yet; the
need for this sort of article is empirical evidence of that. Toxic review
behavior continues to be a problem.

I think face-to-face code reviews are much easier, because when you're sitting
across from a real human, you pay much more attention to the social aspects of
the interaction than when you're hiding behind a screen. You can also adjust
your manner, tone, and word choice in real time based on the verbal and non-
verbal cues you get from being physically present with the person you're
talking to.

~~~
austincheney
I completely disagree. Listening to people is paramount, but the subject of
the conversation is the code in review. If you are paying more attention to
the communication than the code you have lost focus and are reviewing
communication opposed to code.

I have been through this when people get hostile or defensive. They make their
communication more important than the code.

I don't have to deal with any of that online. As a project maintainer if
somebody is being hostile I have several ways to deal with them from a command
perspective. If I am a contributor and the project maintainer is being hostile
then I simply won't contribute to that project.

Also, online there is the convenience of time. You can take as long as you
need to draft a proper well thought out response. Speed of thought in offline
communication can cloud judgement and impose emotional distractions.

~~~
kelnos
If you believe that how you communicate is irrelevant, then I don't think
we're going to have a productive discussion here. Because it _does_ matter, as
much as you or anyone (or even I) think it shouldn't.

~~~
austincheney
I believe I described communication as paramount.

------
gurkendoktor
> Avoid asking judgmental questions like:

> Why didn’t you just do ___ here?”

> Asking such questions implies that a perceived simple solution should have
> been obvious. [...] Instead, provide a recommendation [...].

> You can do ___, which has the benefit of ____.

I don't think that's safe either. If I use a char[] instead of the more
obvious String for whatever technical reason, a comment like "You can use a
String." would sound incredibly patronizing. The trouble is that the reviewer
now has to guess what happened - did I intentionally choose a more convoluted
solution, or was I honestly not aware of the straightforward way to do it?
I've had reviewers who erred on the side of underestimating my knowledge and
found it super awkward.

What helps as a reviewee is to _always_ add a code comment explaining _why_
you are not using the obvious tool for the job; both the reviewer and the next
developer will thank you.

------
Chaebixi
> Even if a developer is extending or modifying a messy part of the code that
> is rife with bad practices, don’t ask the developer to fix them in that pull
> request just because their changeset happens to touch the messy code.

I don't think that's good advice at all. If you touch messy code have a go at
fixing it. If you don't you'll just end up with increasing tech debt.

~~~
CrystalLangUser
Not necessarily. _If_ said refactoring requires very little work, then sure.
But, as it often is the case, refactoring often takes a fair bit of work and
probably more than expected. Thus, it is out of scope of the original
PR/branch.

If the PR is for adding a feature, then focus on that. Refactoring is better
served by having its own dedicated focus and branches, respectively.

~~~
hinkley
People who keep getting deflected from fixing deeply troubling problems with
the code tend to become unpleasant to be around after a while.

Edit: people who enjoy the status quo like to argue about doing it later and
it can be exasperating and also a challenge to filter the people making a
helpful objective decision from an obstructionist one.

As someone else said, it’s best to dig into a problem when you’re already
there instead of having to come back.

~~~
megaman22
There's little more irritating then pointing out something is going to bite
you in the ass if not fixed, have it declared out of scope, and them be blamed
when, as you predicted, it comes around to bite you in the ass. Especially
when it makes it look like you're some amateur-hour bozo to a third party.

------
eldavido
I wrote a long article on this topic today - wouldn't normally post in a
comment but it's responsive to a lot of this author's points:
[https://blog.shortbar.com/tech-debt-in-the-
cloud-a6cdcbea184...](https://blog.shortbar.com/tech-debt-in-the-
cloud-a6cdcbea1848)

------
amw-zero
I don't agree with making tickets to come back and fix things. Lots of times
changing a line in a file points out how smelly it is. If it can be fixed
there, I say give it a shot.

Sometimes "I'll open a ticket" is code for "I'd love to do this, but I'm never
going to."

~~~
hinkley
Translated again: “I’d love to be perceived as willing to do this, but I’m
never going to.”

I got a big dose of this on a project. Requirements got hung up for weeks and
the do it later crowd simply had no idea what to work on. To me this means all
the stuff they “didn’t have time for” was stuff they couldn’t be bothered
with.

If you don’t tell me what to work on I can think of ten different things that
are about to break.

------
xrd
Can we have meta-PRs for code comments? "Please review these
changes/improvements I made to your comments."

------
michaeljbishop
This post is excellent. Bravo! The best places I’ve worked had this kind of
culture.

