Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Ask HN: What tone to use in code review suggestions?

news.ycombinator.com

Post reply on HN
Print view View original

231–240 of 309 posts

Re: Ask HN: What tone to use in code review suggestions?

#231

Post by oldjavacoder »

59nadir wrote: post #160
withinboredom wrote: post #3

I feel like this is often overlooked. One morning, I did a code review before having any coffee. I basically used the latter two throughout the whole thing. I came into the office to find my coworker literally crying. Later in the day, someone else said it was the best code review they’d ever read… and asked me to come to their team… There was so much drama from that code review. That code got merged as-is to appease…

> I was literally just my pre-coffee blunt self. I have a co-worker who's socially retarded as well and I can say that the vast majority of his comments that tend to rub people the wrong way are just badly phrased versions of legitimate opinions (that he sometimes should just keep to himself because no one asked). He's a better reviewer than you seem to be, though; probably because he can take the time to type out be…

Using "socially retarded" is a huge red flag to me, about you.

Re: Ask HN: What tone to use in code review suggestions?

#232

Post by AussieWog93 »

I don't think there's a clear answer to this question.

Stereotypical corporate Americans of course would use the first two, but in Australia using language that tiptoes around peoples' feelings too much (e.g. making sure to point out positives even when someone clearly fucked up) can make it look like you don't trust them or think they're incompetent.

The only other time we'd hear that level of "compassion" is when we have way too much to drink and are acting like a knob.

I've heard Europeans make similar comments about American corporate norms confusing things.

Re: Ask HN: What tone to use in code review suggestions?

#233

Post by oldjavacoder »

simonbarker87 wrote: post #162

I’ve introduced Must, Should, Could at every company I’ve been at (except the one where I picked it up myself) and it works wonders. Prefix every suggestion with M, S or C and then just write the suggested change as a plain statement. The prefix handles the severity and importance without you having to worry about tone. Coulds can be ignored by the coder author with no explanation as to why they are ignoring but if t…

The word "must" should only be used in the Military. Anywhere else, it is profusely out of place. In its stead, I recommend the term "requirement" and then cite an actual reference by the PO or someone officially representing the business requirements.

Re: Ask HN: What tone to use in code review suggestions?

#235

Post by oldjavacoder »

MattGaiser wrote: post #141

As someone on the junior end, the tone is less relevant here than the content, and there is not a lot of content. Why do you want it extracted? Will we re-use it? Do you just not like blocks of code bigger than X (been given this reason many times)? Do you just feel the need to comment (a lot of devs do and I appreciate it when they are upfront about that rationale and I have done this myself to show that I am paying…

I like SonarQube for removing ambiguity. If SonarQube is set to fail the build at lower than a certain percentage of coverage or too low of a grade in Maintenance then it gets fixed before the PR even gets filed.

Assuming the code is processed by SonarQube for regular branch builds, which it should be.

Re: Ask HN: What tone to use in code review suggestions?

#237

Post by jpace121 »

oldjavacoder wrote: post #233
simonbarker87 wrote: post #162

I’ve introduced Must, Should, Could at every company I’ve been at (except the one where I picked it up myself) and it works wonders. Prefix every suggestion with M, S or C and then just write the suggested change as a plain statement. The prefix handles the severity and importance without you having to worry about tone. Coulds can be ignored by the coder author with no explanation as to why they are ignoring but if t…

The word "must" should only be used in the Military. Anywhere else, it is profusely out of place. In its stead, I recommend the term "requirement" and then cite an actual reference by the PO or someone officially representing the business requirements.

Can you explain why? I’ve definitely seen must used in non military contexts. It also seems really strict to limit musts to things specifically laid out in requirements, unless I’m just used to dealing with really loose requirements. My gut is that you’d run into two categories of shoulds, one which is a much bigger deal than the other.

Re: Ask HN: What tone to use in code review suggestions?

#238

Post by hackernewds »

mason55 wrote: post #223
thewebcount wrote: post #187

Earlier quoted context omitted.

> Avoid bike-shedding about style/formatting/etc. If this is important where you work, automate with tools; I'd love some suggestions for how to get people to avoid this type of bike-shedding. While I'd love for us to use some tools to automate this, we have a huge codebase with existing code that doesn't follow the rules, and it's not in my ability to implement said tools. (Big company, lots of overhead, will affect…

Everyone is talking about how to get the style guide integrated into your CI. But it sounds to me like your concern is that other reviewers frequently point out your style guide violations. You could set up some local lint rules just for yourself. You'd need to set it up to only run against code that you changed. And if you don't have an automated style guide you'd have to come up with your own. But it seems like onc…

In addendum, if a couple of style guidelines don't apply to a couple of lines of code for a couple of people, would it have any meaning at all?

Re: Ask HN: What tone to use in code review suggestions?

#240

Post by duped »

oldjavacoder wrote: post #228
no-dr-onboard wrote: post #83

I do code reviews as the meat of my job and have personally benefited from GitLab's handbook article on this topic [1] - No ego (Don't defend a point to win an argument or double-down on a mistake.) - Assume positive intent (If a message feels like a slight, assume positive intent while asking for clarification.) - Get to know each other (Building a rapport enables trust.) - Say thanks (Taking every opportunity to sh…

I agree with "Don't defend a point to win an argument or double-down on a mistake.". I disagree with the juxtaposition that ego is bad, i.e. "no ego". Ego is defined by Oxford as "a person's sense of self-esteem or self-importance.", I agree. Ego isn't bad. To use it in a context directly indicating it's bad is wrong. If the world agrees to do away with the word ego then it must be replaced by something that also mea…

It feels unnecessarily pedantic to debate the usage of a word in a common English idiom, especially with an outdated definition from Oxford.

It would be clearer to just say, "don't be an asshole" but that comes off as crass in professional communications.

Post reply on HN