
Building an Inclusive Code Review Culture - whockey
https://blog.plaid.com/building-an-inclusive-code-review-culture/
======
aequitas
"If someone has committed many crimes against the style guide in a PR, the
reviewer should point them to the style guide..."

Everything a computer can do trivially should not be done by humans. There are
plenty of tools available to validate code to all the standards you can dream
up. And most languages allow you to even automatically fix style errors or
even enforce it for compilation (thank you Go).

So instead of codifying it in a style guide document, codify it in config
files for linting and autoformat tools instead. The defaults of those tools
are often fine for what you need and exceptions can as easily be documented in
them as well.

~~~
mnarayan01

      if a = foo()
    

versus:

    
    
      if (a = foo())
    

Sometimes idiomatic patterns vary based on things which are not amiable to
mechanical correction.

~~~
quxbar
The trick is to ban even the correct but (to a human) ambiguous patterns. In
this case, I wouldn't let you do assignment on the same line as the
conditional.

~~~
squiggleblaz
With an `if` it's probably superfluous and therefore bannable without cost,
but sometimes it can make a `while` loop so much more concise - since the
alternative would be to write the same code before the loop and at the bottom
of the loop. It is the cost of inexpressive languages.

------
spenrose
Based on my experience with Mozilla's mature code review culture, this article
is excellent. If I were going to make one suggestion, it would be to emphasize
the points that author jelambs makes below: this approach to code review is
inseparable from a commitment to deep shared understanding of the code base.
It both depends on and supports that understanding. It's an investment in
quality as embodied in a particular technical approach and body of work. It's
not cheap, but what it buys you can be very valuable.

------
crunchlibrarian
Most code reviews I've participated in have devolved into the senior/loudest
person making sure that people know who they are and what everyone's place in
the hierarchy is.

Even in reviews where the reviewee had some amazing code that was kind of
groundbreaking in one area, still have to find something to gripe about
because I have 20 years on this kid. This seems to be quite common in
science/engineering, not just in code reviews.

I wish I could find that company where nobody gives a shit about their place
in the hierarchy and everyone isn't constantly trying to gun for other
people's jobs to make their linkedin profiles look more impressive. It's been
a looooong time now, still searching.

~~~
daveFNbuck
> still have to find something to gripe about because I have 20 years on this
> kid

I wish I had someone with decades more experience than me taking the time to
find areas where my code could improve. Regardless of their motivation, this
sounds like an amazing resource that you're lucky to have.

~~~
nilkn
I think their motivation still matters quite a bit. Just because someone has
20+ years of experience doesn't mean they're actually leveraging any of that
for their review. If they're nitpicking something unimportant and they're only
doing it for political reasons, then in that act they are contributing
negative value to the team, regardless of experience.

~~~
daveFNbuck
If someone is that much more experienced than you are, it can be hard for you
to tell the difference between nitpicking and something that experience has
taught them is more important than you realize. Even if it is just nitpicking,
that still means your code has nits to pick.

~~~
nilkn
Code reviews are generally public and visible to the entire team and often
even to other teams. While it's true that the reviewee may not be able to
distinguish the two cases being considered, there may be other developers on
the team or in the company who witness the review and can make the
distinction. In fact, this can cause the "toxic reviewing" practice to spread
to other experienced senior developers, who may see others with similar
positions doing the same thing and then feel they must do it too in order to
signal that they are, in fact, senior.

~~~
Drdrdrq
Interesting - I thought you would end the argument differently. In my
experience additional visibility helps curb the unwanted behaviour because
other senior devs step in and help fight it - but I guess that depends on the
general culture in the company. If that doesn't happen you have bigger
problems anyway.

~~~
nilkn
I do believe that code review culture ends up being a reflection of general
company culture. In a healthy culture, visibility tends to spread the positive
part of what's observable, whereas in a toxic culture visibility tends to
spread the negative and political parts of what's observable.

And there's definitely a circularity here. In a healthy culture, code reviews
tend not to be overly political in the first place, so it would stand out like
a sore thumb if someone was causing a lot of unnecessary drama with their
reviews. Likewise, in a toxic culture, code reviews may go from healthy to
toxic and dramatic because developers have been incentivized to compete with
one another in counterproductive ways.

~~~
KKKKkkkk1
As an engineer, how do you address this sort of mutually-reinforcing jerkery?
I expect that complaining to the manager will not be effective because the
culprits are not explicitly hostile.

~~~
Drdrdrq
Easiest way? Complain, look for another job, quit. There is no way you can
change the culture from bottom up. Best you can achieve is make a bubble with
your close coworkers where you cover each other - but even that is temporary
and can be broken by management anytime. Sorry. But there are many companies
with better culture out there...

------
zhangela
How do you all make sure the code review requests get load balanced across the
team, as opposed to 2-3 people getting all the PR requests?

~~~
notyourwork
Not everyone is the right person for a specific code review. Even on small
teams someone may be more of a subject matter expert in one facet of your
system or code. Therefore load balancing isn’t really the right way to look at
it. The idea should be to seek feedback inclusively and welcomed from all
teammates. As a submitter your job is to make sure you get the right feedback
if there is someone more knowledgeable of a topic. This is why standup is a
good place to discuss a code review where you can identify need for time and
also identify right people.

~~~
zhangela
Agreed it shouldn’t be an even distribution of review requests across the
board. That said, if a few people are getting all the reviews, it’s difficult
for them to make progress on their project work, and for other engineers to
learn code review best practices and get familiar with the codebase.

~~~
notyourwork
It’s a balance but you need to balance team priorities. If you aren’t shipping
code because of your time on code reviews you need to better manage your time.
Call it out during stand up (assuming your team has them) and make it clear
you don’t have time or code reviews today because you need to focus on your
deliverable. If the review is more important your team can identify this
collectively. However, individuals should never feel you describe, if so it’s
a lack of coordination across team.

------
KKKKkkkk1
How common is it to get unsolicited reviews on your PRs? In my team, I have
some colleagues who will routinely pipe up when they see a PR being uploaded
by an "outsider" and will write a litany of complaints in order to block
progress and establish their dominance. (I should mention that these
colleagues work in a branch office, so this is their only way to assert
themselves.)

~~~
knocte
> in order to block progress

This is your subjective interpretation of the facts. Learn how they complain
(in the end, they only can complain about technical facts: learn them, and be
correct next time), and you will not receive complaints anymore.

I'm guessing you're new to code reviews right?

------
tdsamardzhiev
The real problem here are people with bad attitude and poor communication
skills. Bullying other people is a punishable offense in real life, and I
don't see why code reviews should be any different. Have some basic rules
regarding communication. Issue a public warning for first-time offenders. Give
them the boot if they do it again. There, problem solved.

~~~
sqrt17
You sound like you didn't read the article. There's nothing about political
correctness or try-hard inclusivity in that article but many (or indeed all)
the things that common sense would dictate as a base of working code reviews.

~~~
tdsamardzhiev
No, I didn't read the article. But I did read the entire comment section, if
that counts :)

------
egyptiankarim
Neat to see that they started by interviewing a diverse subset of their
engineers; that type of user-centric research is invaluable. Moreover, I
wonder what of their methodology could be mapped to a live analysis of the
code review conversation threads they're having via GitHub.

More than anything else, though, I'm curious where teams like this still go to
buy those amazing Apple Cinema displays?!

------
brian-armstrong
Not focusing on style violations is foolish. Imagine someone submitted an
English paper littered with grammatical errors and typos. It would be
difficult to find the intended meaning and bigger picture. Style violations
are the same, they have to be fixed before a deeper review can even take
place.

------
Confusion
Good article, but I don't understand why the word 'inclusive' is in the title.

