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

281–290 of 309 posts

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

#281

Post by dhzhzjsbevs »

sebastien_b wrote: post #124
IncRnd wrote: post #78

Review: a formal assessment or examination of something with the possibility or intention of instituting change if necessary. None of your examples provide feedback as to why you want a change. That may be why you have been led to ask this question. Consider: * There is a potential overflow in this code. The library function xyz already does this and can log when the app is in debugging mode. * This portion duplicate…

Agree with adding additional reasoning, regardless of the actual tone. I’ve received some feedback along the lines of “LOL Wut?” on a comment in code, which I really didn’t know how to take or respond to (suffice to say its meaning was vague, and generally unhelpful).

Sometimes you catch people in low energy or bad moods. It doesn't excuse the behaviour but try not to take it personal.

My trick is to write the wtf in pending review then go and look later in the day if it's still valid. Often I just needed to eat something and I'll be over it.

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

#283

Post by dhzhzjsbevs »

somethoughts wrote: post #158
IncRnd wrote: post #78

Review: a formal assessment or examination of something with the possibility or intention of instituting change if necessary. None of your examples provide feedback as to why you want a change. That may be why you have been led to ask this question. Consider: * There is a potential overflow in this code. The library function xyz already does this and can log when the app is in debugging mode. * This portion duplicate…

Where possible it can also be useful to point out if there are existing guidelines/checklists the submitter should be referencing to understand these preferences of the project. The more the contributors can code review themselves prior to submitting a PR the better. And if such a resource doesn't exist or that topic is not in the current guidelines/checklists - offering to take up the task of creating or adding an t…

I ignore style check nitpick comments.

Fix your linter and autoformatter.

You don't pay me to lint code.

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

#284

Post by redwolf22 »

sebastien_b wrote: post #124
IncRnd wrote: post #78

Review: a formal assessment or examination of something with the possibility or intention of instituting change if necessary. None of your examples provide feedback as to why you want a change. That may be why you have been led to ask this question. Consider: * There is a potential overflow in this code. The library function xyz already does this and can log when the app is in debugging mode. * This portion duplicate…

Agree with adding additional reasoning, regardless of the actual tone. I’ve received some feedback along the lines of “LOL Wut?” on a comment in code, which I really didn’t know how to take or respond to (suffice to say its meaning was vague, and generally unhelpful).

I would talk it out with the person. Such comments aren't helpful at all, unless the mistake made on that line is obvious.

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

#285

Post by kubanczyk »

duped wrote: post #240
oldjavacoder wrote: post #228

Earlier quoted context omitted.

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.

On the contrary, HN is the place to debate the usage of words. Precise word choice is how to effectively get your point across. Words should have more definitions than the obvious two ("something universally good" and "something universally stupid, annoying, embarrassing, and smelling like fart") and it's not a shame to talk about subtleties of their definitions.

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

#287

Post by CrimpCity »

wutbrodo wrote: post #268
CrimpCity wrote: post #259

Earlier quoted context omitted.

Wow that's crazy! Was it cultural issues for example foreign born coworker who doesn't understand how respect in the USA works in text form? That's an interesting point about high skill/experience/knowledge inequality leading to unequal code output. It seems that the less skilled person wants you to sign off on the code in which case they turn their brain off a little and defer to you. I think approaching the matter…

> Was it cultural issues for example foreign born coworker who doesn't understand how respect in the USA works in text form? No, this was a white guy born in America. I think it was just a self-esteem thing: he didn't feel worthy of being treated as a peer, and wasn't emotionally healthy enough to handle the confrontation of one's mistakes that good engineering requires. > That's an interesting point about high skill…

Thanks for the considerate response!

I think the no-ego environment does a lot of work towards being more effective and increasing self-esteem. It's just so conducive to learning! But learning can be difficult especially if someone who's smart has never really been challenged as I can imagine being the case if you're doing advanced research where everyone has so much deep expertise.

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

#288

Post by SilasX »

osigurdson wrote: post #264
SilasX wrote: post #257

Earlier quoted context omitted.

That’s the point: the replies are saying that the content is more of an issue than the tone, and so OP should focus on the hints for the former.

How do we know the OP had any issues coming up with valuable and insightful comments? Tone can have an impact - being particularly rude is not likely to result in good outcomes (but maybe it could in some situations). Why aren't we exploring what was originally asked?

Because all of the versions have the same problem regardless of tone. If the assumptions are in error and the OP had been leaving off the justifications suggested, they can reply and say so! (But, as it turns out, didn’t.)

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

#289

Post by dominotw »

withinboredom wrote: post #169
dominotw wrote: post #140

Earlier quoted context omitted.

Is this person working on his own. Why can't people pair and give each other at the moment feedback vs doing code review. Person is already invested in his code by PR review time and doesn't really want to do PR ping pong for their task. What if you come back with more comments after they address your comments. Will they ever get to finish their task or forever be hostage to you subjective comments. Most of it is usu…

> Why can't people pair and give each other at the moment feedback vs doing code review. That was something we discussed at various points. We never tried it when I worked there. > you had to explicitly mention 'security issues' which imply that you have a feeling that most of your pr comments were your subjective interpretation . Aren't almost all code review comments? I can't think of a single one except ones that…

> but the relationship between the reviewee and the reviewer is more important than any of that and how you review code will affect that relationship whether you want it to or not.

yes I agree with this 100%. I've gotten nasty reviews from people i like and it didn't bother me at all.

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

#290

Post by mleonhard »

notyourwork wrote: post #182
hcarvalhoalves wrote: post #41

My rules for reviews (which influence the language used) are: * Criticise the code, not the author ("There's a bug here" vs. "You inserted a bug here"); * Reach common agreement that people shouldn't cling to code written ("sunk cost fallacy"). Code is liability, and it's okay to throw things away and restart from scratch, the value is in the learning; * Don't use questions that invite discussion if you're not inviti…

I agree in general, though I find the second on clinging to code difficult when people counter with timelines. It can be difficult to express to juniors how to reason about taking a bit more time to do it "right".

Even when you express it clearly, many juniors won't understand and will feel frustrated. Some things can't be taught and must be experienced. Every junior developer must work on a crufty legacy codebase to develop understanding and habits for writing maintainable code.

Large companies pick experienced internal transfers for shiny new projects and hire externally for unglamorous legacy systems. Since all new grads are external hires, they end up starting out on legacy systems.

I recommend that junior devs start out at Amazon. They will learn a lot of important things in a year or two: the pain of technical debt, the value of automated tests, the pain of production issues when you're on call, that working overtime is not rewarded and does not solve job insecurity, and that switching companies is a great way to get more experience and earn more money.

Post reply on HN