
We are ruthless on code reviews - grey
https://techblog.workiva.com/tech-blog/we-are-ruthless-code-reviews
======
scolfax
\- If this post is satire, it's hilarious and spot-on!

\- If this is real, quit while you can, and may god have mercy on your soul.

\- The fact that it's difficult to tell if it's real or not says a lot about
our industry.

~~~
cpjk
It's not satire, it's hypothetical.

~~~
goldenkey
It reeks of humble bragging and pretentiousness. The Dark Souls analogy just
tops it off...

~~~
cpjk
Reading it again, I can see that. However, I personally am a fan of rigorous -
not ruthless - code review, the purpose and level of which has previously been
explained to a new employee.

Obviously being an asshole is out of line, and if someone is being a dick in
code review, that's a problem with the reviewer. But I think that being
rigorous about maintaining consistency and quality is important.

~~~
goldenkey
Decent positions dont usually need disclaimers. Theres a difference between
preaching, teaching, and coaching. Its not obvious that being an asshole is
out of line. You make it sound like this mystery of pure "collaboration" is
all about giving raw feedback. Raw feedback is inferior to polished feedback.
Its just as important to communicate in positive ways instead of throwing this
ruthless word around.

Honestly, your problem is that youve approached code discussion from a purely
quantitative approach - ability to convince and ability to convey arguments in
a persuasive yet unbiased way is huge. No one wants to do things by demand,
decree, or just general hate.

~~~
cpjk
Haha ok dude. I didn't write the article. And that wasn't a disclaimer; I was
empathising with your point.

~~~
goldenkey
Oh, well carry on then, my bad ;-)

------
mahyarm
If you have things in your style guide that can be done easily by a script,
they shouldn't ever be interm rules. They should be a linter auto-fix rule
that applies it to diffs automatically and NOTHING ELSE. It shouldn't even
blip at you saying it's against style, it should just fix it.

If you don't have this, you waste everyone's brain cycles with your pedantic
import ordering rule or similar on both sides and it's a red flag about the
company to me.

------
gjolund
I love working in environments like this. I can say hands down they were my
most productive years as a programmer.

Quality code review is the single most important trait of a high performing
senior programmer.

~~~
bobbytherobot
Before people take and run with this comment, this will turn into a shit show
if you don't have actual senior programmers with the needed experience. I've
seen it happen in startups where their "senior programmer" has only been
working for three years out of school for the single startup.

~~~
falcolas
This is absolutely the truth, and based on my experiences, absolutely a
problem. New graduates are promoted through the ranks at startups so quickly,
yet they have terrible interpersonal skills, and their code reeks of naivety.

I've watched an "architect" from such a company write the most tangled
spaghetti western of code for the simplest CRUD app that even his lower level
colleagues were appalled. Of course, those same colleagues then advocated a
complete re-write of the working codebase instead of incrementally fixing the
code, causing about 6 months of new work for the company before they could
start on hardening or new features...

No, experience doesn't automatically make you right, but it does increase the
likelihood.

------
estefan
People find it acceptable to write more harshly than they'd speak face-to-
face. I wonder if all 43 or whatever points would have come up if it was done
side-by-side the developer.

Also, great negative PR for this company. This sort of discussion would also
come off as much less arrogant/snarky face-to-face.

------
crdoconnor
>I don’t mean we’re mean-spirited. I just mean that we are merciless. You’ll
notice that I left the comment “Beep!” on the imports of every file you
touched. What I meant was, “ _Your imports violate our standard convention—we
order them by built-ins, then third party, and then project level_ ,” but that
was too much to type on every file.

Translation: "We have _extremely_ strong opinions on what materials to use for
our nuclear power plant's bike shed."

[https://en.wikipedia.org/wiki/Law_of_triviality](https://en.wikipedia.org/wiki/Law_of_triviality)

There's seems to be an inverse relationship between how much effort a team
spends on surface details of code (e.g. does it adhere to linting
standards/are the lines < 80 characters/imports ordered?) and how much time
and effort a team spends on deeper architectural issues (loose coupling, de-
duplication, un-reinventing the wheel, transforming imperative to declarative
code, etc.)

~~~
Ensorceled
Until the first time you spend a couple of hours looking for a weird bug in
Python because somebody broke this rule...

~~~
crdoconnor
Which rule?

The rule that imports must be ordered according to the whims of the team lead?
Or the rule that lines must be under 80 characters?

There are a few linting rules which it pays major dividends to pay attention
to (e.g. variables that are initialized but never used), but most of them are
superficial.

~~~
Ensorceled
You commented on the import ordering so clearly that's what I'm commenting on.

Not following the PEP standard for python import ordering can cause issues
like circular imports or monkey patching not working properly.

------
auggierose
It always astonished me how delicate coders in the UK are (I am from Germany,
working in the UK currently). During a team meeting at a former company, my
"ruthlessness" was brought up as a negativism. I countered that (or so I
thought) with the following question to the rest of the coder team: "Do you
want me to point out problems I perceive with the code / approach / whatever,
or do you want me to hold back in order to be considerate of someone". To my
surprise, in the presence of management, the unanimous answer was to show
consideration!

I think people who work together should be able to openly communicate with
each other about technical issues. Sadly, most people are (either self-
perceived or for real) not good enough to do that without bruised egos.

~~~
brandmeyer
There is another side to this. I've seen a ton of technical criticism get
delivered with emotionally laden commentary. X is "totally broken", "crap",
"not even wrong", "just wrong", "terrible", etc. There is a way to deliver
criticism while maintaining an environment of common professional respect.
Many people don't even try. With the caveat that I'm painting with a
culturally biased brush, I've found Germans (and Finns!) to be particularly
abrasive in this regards.

~~~
cpks
And conversely, I've seen people from cultures with different communications
styles substantially and unfairly discriminated in the industry for not
matching 2016 American business culture. I've found Germans (and Finns!) to be
particularly discriminated in this regard, primarily because those are highly
technical cultures, but with different communication styles. However, anyone
who grew up in the inner city, in African American culture, in Irish Catholic
culture, etc. is likewise stigmatized and labeled as rude due to what is a
cultural communications difference.

------
kvcrawford
I miss working in an environment like this. I was actually surprised to find
so much negative feedback in the comments.

I think a detail getting glossed over is that the tone needs to be calibrated
to the relationships between team members. If team members joke around with
each other, send silly gifs, get lunch or coffee together, and are generally
on good terms with one another...then it's known that you don't take PR
comments personally, they're just feedback. And the more critical the
feedback, the better developer you become.

~~~
Dommorak
I can tell you that this is the kind of environment I work in and the first
thing I teach a new teammate is that the team is only concerned with
consistent high quality code. I think it is a good lesson for most developers
and software engineers to separate their personal feelings from their code. It
can help you be less defensive of your approach and take in multiple
viewpoints to find the best possible solution for the task at hand.

------
parennoob
The whole "we are so cool because we are _ruthless_. _Merciless_ , I tell you!
Once you get through our code review your code will be perfect." attitude of
this article is a bit surprising for one ranked so highly on HN. Do people
really prefer this kind of attitude? Bender GIFs and snarky comments can
become quickly tiring.

I personally would strongly prefer if they put it in milder technical terms,
"We are very conservative with deviations from our preferred code style and
conventions". I suppose that wouldn't be as click-baity, but for me (and I
suspect a lot of other people) it would be easier to appreciate, since we
understand that being conservative before accepting commits leads to a well-
groomed codebase.

~~~
savanaly
Every single time I do a code review I find myself noticing a lot of little
style mistakes (and, not to be overconfident, I could be mistaken myself about
that) and don't leave a comment because I don't want to be that guy that no
one wants to review their code and gives one a major headache because I care
more about the consistency than does the person I'm reviewing.

In other words, I might restrain myself from criticism out of my compassion
for my coworkers. The definition of "ruthless" according to Google is "having
or showing no pity or compassion for others," so it's a fine term to describe
how I'm not-- and how this company is determined to be.

~~~
mkozlows
As a team, decide how much you care. Some teams won't care much at all, and
will follow a "hey as long as it works" attitude. Other teams will be deeply
interested in making their code as good as it could be, and will want comments
about doing things more cleanly or elegantly and will view PR review as a
chance to learn and improve.

As long as everyone's on the same page, and sharing the same attitude, that
drive to improve will come across as a positive thing rather than a jerk move.
But if you're the one person with that attitude on a team of good-enoughers,
yeah, not so much.

------
jkot
For me declaration of "ruthlessness" is a red flag, similar to ninja buzzword.

Strictness in one direction does not mean company is strict at all fronts.
Code reviews can be used without automated tests or version control.

In this case the review process seems like a "busy work". Checks bellow could
be easily integrated into build script or CI. Such code should not even made
it into code review. (I am not arguing about the rule itself, just how it
should be enforced)

> _You’ll notice that I left the comment “Beep!” on the imports of every file
> you touched. What I meant was, “Your imports violate our standard
> convention—we order them by built-ins, then third party, and then project
> level,” but that was too much to type on every file._

------
siegecraft
God, how insufferable. "Our code reviews are so ruthless because our clients
(and by extension, us) are so much better than everyone else!" But apparently
we can't write a bot or a pre-commit hook to re-order imports so instead we'll
write some non-sensical in-house jargon (because typing out the full
explanation is too much effort, even though we are the best of the best
because our clients are so excellent and demanding). But don't worry, we'll
write a 2 page email to explain this jargon.

------
eddieroger
I guess that's one way of doing it, but you're dependent on the quality of
said ruthless reviewers, and delivery of the message is separate from the
message itself. Personally, I'm more of a "kill them with kindness" guy, so
while my reviews would come off as soft by the article's definition, I'd argue
that they're just as effective, and my teammates still want to have a beer
with me at the end of the day, instead of walking away thinking I have a stick
up my ass, which an email about ruthlessness wouldn't necessarily negate.

------
draw_down
If it's anything like where I work, some people come in for more ruthlessness
than others. Some are more equal than others, if you will. New people or those
deemed unworthy will receive the full brunt of scrutiny, to the point of bare-
faced pedantry. Those who are golden are given the benefit of the doubt. You
can probably tell which one I am. It gets really old to see this time and
again, and to see the team pat themselves on the back for being "ruthless".

~~~
noamsml
Playing devil's advocate here: The only way to really learn the full ruleset
of a team is by undergoing a couple of code reviews in which every issue is
pointed out. Once your teammates see you've understood the prevailing style,
they don't have to work as hard at code reviews.

That said, I usually hate petty/nitpicky code reviews. I try to focus only on
issues that I think affect readability, maintainability or functionality.

~~~
Niksko
Makes sense, but there are ways of being even petty and nitpicky without being
condescending or rude. State facts, don't throw insults, don't demean or
belittle.

~~~
coredog64
One good rule I was given by a tech lead is to not use the word "you" in the
PR comments. PR comments are a discussion on the failure of the code, not of
the individual. Avoiding "you" allows for some separation between the two.

~~~
cachemiss
I always use "we", and I almost always phrase things as a question.

Example: "So we are doing X here, which I think will probably do Y, which
could have adverse affects Z, are we sure we want to do this?"

------
jorjordandan
This reminds me of people who say, "People don't like me because I'm so
honest." No, people don't like you because you lack basic empathy.

~~~
mfringel
Amen. Bonus points if they try to dress it up with some honorific like
"Truthful Speaker".

------
nmeofthestate
Code reviews are pretty heavy going on the project I'm working on at the
moment. The code isn't noticeably better for it though!

~~~
ybobinator
You gotta argue your side if what they're saying is going to make shit awful.
IMO the best way to do it is to try their approach out and show them in a few
lines that you don't think it's helping. Doing whatever people say just
because is bad and so is just saying no. You gotta work with them.

~~~
nmeofthestate
Don't worry - there's been plenty of long fruitless debates along the way.

------
joesmo
Yet despite being assholes (ruthless in their parlance) who obviously have not
on-boarded said employee or even told him what is expected of his code, I
doubt their software has any less bugs than the software from shops who are
not assholes (ruthless in their parlance). Sounds like a great reason to avoid
working here and other places that have the same mentality. If you want your
coworkers to actually do their work, it's a really good idea not to be an
asshole because it's incredibly easy to get away with not doing shit in tech
while seeming to do so and after awhile, most people will switch to that mode
when berated long enough. I can't blame them. Employers deserve no loyalty in
the US these days because they don't give any.

~~~
hvidgaard
In this articles terms, I'm ruthless when doing code reviews. I do it because

* We have a code style to follow, and if we don't we might as well not have it.

* It ups the game from the developers, they double check their code before checkin, and believe it or not, they catch more errors this way.

* It have created an environment where we can openly talk about interative improvements.

------
louden
'We are _rigorous_ on code reviews' would be a better approach. We don't have
to be jerks to make sure that code is up to standards.

------
cafard
"One thing to keep in mind is that our customer base is unique. Our customers
work for many of the most successful companies in their respective industries.
They are where they are because they are committed to excellence and have
great attention to detail."

An awful lot of companies can describe their customers in such terms.
Microsoft can, Google can, Pepsi and Bic can.

------
kabdib
Satire, yes?

If not, GTFO while you can.

------
jbrooksuk
As a PHP "shop" this is why we use StyleCI [1].

Our code isn't that critical, the quality and maintainability are. We don't
waste time on "code reviews" that only check the style of code, that can be
automated away.

Yes, I make StyleCI, but we use it in the place I work too (unrelated).

[1] [https://styleci.io](https://styleci.io)

------
aelaguiz
Based on this I can only assume that workiva builds spaceships or life support
systems. You know, something where absolute perfection is far more important
than having a fulfilling and constructive workplace environment.

------
gdulli
It's probably a joke but it's backfiring because most comments are taking it
seriously and thinking the place actually works like this.

------
dlandis
Ugh, comes off as condescending, immature, and insufferable.

I do think strict code review and high standards are the way to go, but when
implementing them it becomes all the more important to have appropriate and
professional interpersonal communication and documented policies.

------
MisterBastahrd
If you've got time to dream up ways to be witty in order to be snarky to a co-
worker, then you've got too much time on your hands at work and clearly need
to fill it with other responsibilities.

~~~
savanaly
I would hazard a guess that this letter was to a hypothetical employee, rather
than for a real situation. And its purpose was to illustrate the type of code
review new employees can expect and are expected to give, in a whimsical way.
I quite liked it.

