loading...

Politeness or Bluntness in Code Review? Settling the Matter Once and for All

Erik Dietrich on March 21, 2019

These days I make a living producing content (or, increasingly, running a content operation).  As a result, social media and blogs have become more... [Read Full]
markdown guide
 

nice post. I am an Italian living in Germany. I can tell that I never really like the "italian way" of telling things: this subtleness where you always try to be kind, friendly, funny often ends up in utter hypocrisy and more often in sarcasm. Of course, the first weeks in Germany surprised me. They are really blunt and direct, but I immediately liked it. If something is wrong - and I am not talking about coding style - but implementation details ( memory leaks, performance issues - sometimes code readability - why can't you just say it directly - without false compliments and the sandwich stuff.
Don't get me wrong. Being blunt does not mean being offensive or aggressive. I understand that there are cultural differences - as an Italian I gesture a lot - and I am probably louder as well - therefore when discussing code with a Japanese colleague I inevitably look and sound more "passionate" about my stance. ;-)
Kindness and politeness is key. everywhere. but it should not hinder the message being transferred. If I say "I really like the effort you put into this bla bla bla, it might be my impression but this could be done differently, and i would have probably have written this" - not only i need more time than writing "It would be better to do this, please refactor/change it" but the reviewee might just think - well it's his opinion. i will keep it in my way.

At work we do code reviews within the pull/merge requests in gitlab, so normally we just write a couple of sentences in the points we want to have fixed / changed. kindly but firmly.
What i always do though, is pointing out with positive a positive comment some parts of the code which i like or find particularly clever. They are not required during the PullRequest - but i guess that can make the code review less "demotivating" ( in the end we are just pointing out mistakes - that's what MR are made for)
Something that i like to point out is also - use linters so that the code review has nothing to do with style and formatting.

 

"I really like the effort you put into this bla bla bla, it might be my impression but this could be done differently, and i would have probably have written this"

I'd distinguish between the "like the effort" bit and the "might be my impression" bit -- the former is harmless but the latter waters down your stance. And ideally, there'd be some mention of why the proposed alternative is better.

But I agree with your larger point. It's nice when people get the point. Belle parole non pascon i gatti...

that can make the code review less "demotivating"

I actually really enjoy seeing what I've missed, even if I might feel dumb later. It's like seeing a magic trick get revealed. But I've seen folks dread code review who were nevertheless very good devs. Everyone's politeness meter is calibrated differently, I guess.

 

Belle parole non pascon i gatti... ( Fine words butter no parsnips )

despite being italian i never heard that (it's probably regional :-) very very nice!

lol. my father's from Avellino province but no one in my family says it either. I saw it online and it seemed cute and half-relevant :P

 

Different Cultures Land in Really, Really Different Places on the Subtlety vs. Directness Spectrum

I agree. In particular I think that supplicating-by-default can be very problematic when folks aren't expecting it.

"nice or blunt" is ... staking a claim to your power or lack thereof.

Ideally, among knowledge workers, "power" (though I prefer "standing") follows from correctness. If that doesn't happen during code review, the only good option is to leave for a place where it does. Maybe it's naive, but I think a focus on correctness is how you break out of the politics.

At best bluntness should be tempered by:

  • how sure you are
  • how important the thing is
  • assuming the other person has good intentions
  • knowing that learning a thing before someone does doesn't make you smart

I try to be right and nice, in that order. And I try to know when I'm not sure, or when it's a matter of taste.

Folks complain about the tenor of Stack Overflow, but the only time I see people get chewed out there is when they're very confidently wrong. IMO such reprisals are useful for maintaining a culture of accuracy.

Linus Torvalds

Unfortunately no one tells stories about when someone isn't a dick. We've got a big pantheon and I'd argue that most of its members are fairly unassuming. I've never read about Kay, or Steele, or Hickey telling someone they're "so stupid it's a wonder they figured out how to breastfeed." (That's the cleaned up version.)

the right strategy is one that you tune according to your audience.

That was my first thought. If you know the person, use the style of communication they prefer. But be nice. And be right. Ideally, explain why you think you're right, and have the integrity to also say why you could be wrong.

 

I'm trying to resist the urge to write a novel on the subject of office politics, since it's both something I've talked a lot about on my blog for years, and, in its current corporate incarnation, the thing that eventually drove me out of employment and into working for myself. At any rate, I'd say the fatal flaw experienced by most groups aiming for "correctness as currency" would arise from issues where correctness is either unknowable or else non-existent due to subjectivity.

So, at any rate, I don't take what you're saying as naive at all, but rather the seed of a good mode for interaction. Because if correct is (currently) unknowable, that should result in extremely productive discussions of "this call seems subjective, so how could we run an experiment and measure it?"

For me, that approach tends to eliminate a lot of discord, in the polite vs. blunt arena, but in terms of human interaction in general. "Is this a good blog post" is a question that can quickly become a terminally stupid shouting match between respondents. But, what if you say, "good as measured by what -- time on the page, reader engagement, etc" you can then start to establish outcome-oriented metrics and have more productive conversations.

 

Great post. I personally feel that bluntness saves time and it's definitely necessarily to be critical. Adding a few pleases/polite terms helps to calm down the tone of post which allows messages to get across without sounding too bossy/offensive...:)

 

My book Programming Without Anxiety will address code review. Some important bits here:

Explain the generic idea as well

Whenever pointing out a specific thing, describe the generic underlying idea as well. By adding context, the author doesn't feel scolded, rather understands the intent towards a broader rationale. (And might dispute the rationale, not the specific comment).

For example, "Make this method final." can be extended with "Generally, prefer making methods in this class final, as we want to avoid accidental overriding. We had bad bugs about that in the past.".

Single comment per category

Add only a single comment about a single category of issue, and have an agreement that authors should fix all relevant issues without asking.

For example, don't write "Remove trailing whitespace here" on 20 lines. Add only on one line, maybe saying "Remove trailing whitespace - other lines too" initially.

Style guide

Have a public style guide, so authors diverge less. Keep it updated with common mistakes.

Ask for correction first

If a piece of code looks quirky or impenetrable, don't tear it to pieces. Rather start by commenting "I'm confused by the control flow here. Could you make it cleaner or comment?" or "This looks complex, please simplify or add more tests.". Often the author knows a piece of code is quirky, and a small trigger nudges them to fix it up.

If the code still comes back complicated, then make specific improvement comments.

If you like these and want more, subscribe to (or better, prepurchase) my book!

 

I have become disgusted with how people take criticism over the past few years. It gets worse and worse, to the point that simply saying:

Your site loads painfully slow due to the 100+ separate files causing a lot of handshaking overhead. The over-use of frameworks has resulted in major accessibility failings due to a relative lack of semantics, gibberish use of numbered headings, and a lack of scripting off graceful degradation. These problems are only further exacerbated by the use of fixed metric (px sized) fonts and layout, colour contrasts that fail to meet accessibility norms, and a general mismatch of task complexity rooted in "false simplicity". Many things users would do on your pages are actually harder to do, because you made it look simpler.

(actual post)

Some places -- I call them dens of suck-ups and sycophants -- that's now treated as a ban-worthy post as if I'd used every four letter word in the book mated to Carlin's "Seven Dirty Words". Some will even say "that's not constructive" -- what the bloody blue blazes is "more constructive" than providing a list of everything that's WRONG?!?. What are we supposed to do? Slap the rose coloured glasses on their heads and whisper soothing-syrup words in their ears as they drive off the cliff?

... and why? To maintain the status quo 3i of web development: Ignorance, Incompetence, and Ineptitude.

Mind you, I don't use "ignorant" as an insult. It just means you don't know. It's only when you're told and continue to willfully do things badly that it becomes the other two.

There's this "wah wah, somebuddy usededed teh hursh wurdz" attitude that's flushing this entire industry down the toilet, allowing every two-bit dirtbag scam artists peddle their snake oil, and silencing anyone who dares speak out against it.

How DARE anyone get upset by sleazy scams. How DARE anyone get upset by poseurs not qualified to work with the underlying languages like the know-nothings who created and maintain bootcrap. How DARE anyone make waves, rock the boat, or be "confrontational" over bad practices, nubes being led down the garden path to failure, or any of the dozens of other dishonest bald faced LIES that make up the majority of "new" development practices of the past ten to fifteen years.

No, that might "upset" someone. Heavens to Betsy, nots thatz.

But really that's what this "It's not how you said it" or "if you can't say anything nice" rubbish boils down to, controlling the narrative and forcing a language of control. It's part of the "bandwagon" process of propaganda, and is why those singing the praises of so much of the outright incompetence in this industry follow up quickly with transfer, glittering generalities, plain folks, name calling (and not the crass direct type I just used; But to understand that, you have to recognize that "easier" is name calling....), testimonial, and of course card stacking. That all seven? Yup, all seven propaganda techniques. Learn them, recognize them!

Can't argue the facts, so attack how they were presented whilst screaming "wah wah, is not" like a petulant seven year old. The bread and butter of "modern" discourse. Nothing more than lame excuses to silence anyone who doesn't let themselves be hammered into that round hole.

Though I freely admit the closer I get to the big five-oh, the less and less I give a flying purple fish about what people think about how I say things. My internal censor was killed off twenty years ago, but now I'm just so frustrated with the endless streams of lame excuses for lamer development practices that the kid gloves are off.

 
 

While I agree that a mismatch between subtle vs direct communication styles can cause a multitude of problems*, I think you may be mixing in an orthogonal behavior axis: cooperative vs combative.

For example, let's assume a review for a bit of code that needs work. I tend towards blunt/cooperative, so unsurprisingly I'm not really good at picking up when someone's being subtle/combative, so these examples will probably be a bit exaggerated. Hopefully, they'll be close enough to get the idea across.

blunt/combative

This is really ugly, what made you think using null was a good idea? Get rid of of nulls and learn to code language_name, or go back to other_language.

Characterized by abusive language, this is the one we generally think of when talking about abusive behavior in reviews. Linus Torvalds is an infamous example of this communication style.

subtle/combative

consider rereading our style guide

The best description I came up with for this one is "passive-aggressive sniping".

blunt/cooperative

Please remove these nulls, there are better ways of solving this. Option is a common tool for dealing with missing values, and would look something like this:...

This one is the one I tend towards by default, so I'm most familiar with it's strengths and weaknesses. Basically: direct, polite, and very clear about what is expected.

Often this involves including code snippets, on the grounds that, if it's worth bringing up, it's worth taking the time to suggest something concrete.

If you tend towards this style, be careful to use open-ended phrasing and acknowledge the limits of your certainty frequently, or you'll run the risk of sounding like a pushy know-it-all to folks who prefer subtle communication.

subtle/cooperative

We tend to avoid nulls, as they have been the cause of several hard to trace bugs. Option may be a good alternative.

Polite and (if you prefer direct communication) a bit on the vague side, but still providing enough information for the reviewee to fix the issue.


* This can cause trouble even when everyone on the team comes from the same area. For example, in the USA men tend to be socialized to favor direct communication, and women tend to be socialized to favor subtle communication. This causes about as much trouble as you might expect, if people don't actively work to mitigate the mismatch.

 

I blogged about my take on code reviews a little while ago, so here's the obligatory link to my 2 cents mcod3.wordpress.com/2019/04/24/pee...

 

Code review should be "Lead by (code) Example" rather than blabbering lot of concepts or jagrons .
Code speaks a lot , Words doesn't.

 

Great post, well explained! I'm working with UK folks and I can say that they're really good in giving "Code Review" feedbacks. Very clear and direct to the point. I also agree that the review should include a summary that contains those where you did great and the reasons why the certain codes needs refactoring.

code of conduct - report abuse