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

171–180 of 309 posts

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

#171

Post by evsasse »

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…

Yeah, that is great.

Learned something similar on a previous job. Prefix it with "#must", "#should", "#could", "#would".

We've also extended it with some other things that you may want to comment, but are not necessarily actionable. "#wont", "#idea", "#tip", "#question", "#risk", are pretty much self-explanatory.

"#fun": an ugly workaround that could've worked; some funny situation that the change could cause; ...

"#domain-knowledge"/"#debrief": really nice if you are working with people that are more junior, or not that familiar with this codebase in particular. May help other reviewers understand better why the author made certain decisions.

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

#172

Post by kqr »

graftak wrote: post #131
ginja wrote: post #87

Earlier quoted context omitted.

Yeah I remember being a junior and feeling like blunt comments were kind of rude. So I make an effort to be nicer to new developers - sometimes simply adding “what do you think?”/“do you agree?” is enough to make those command style sentences sound much less blunt, and also encourages them to ask questions instead of blindly following my suggestion if they don’t fully understand it.

To me those questions could come off as condescending. I say could because I wouldn’t think that’s your intent, but the tone is there.

That depends entirely on whether you add the question as a formality or if you are genuinely interested in why the other person might disagree.

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

#174

Post by fknorangesite »

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

Earlier quoted context omitted.

> 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…

Considering this story is over a decade old, and I was in my late teens ... I'd hope that I've grown quite a bit even though you're using the present tense like you know me.

The way you told the story it comes off as much more recent.

> I'd hope that I've grown quite a bit

I hope so too, because "I hadn't had my coffee yet har har" is a pretty thin excuse.

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

#175

Post by 3minus1 »

pclmulqdq wrote: post #39

What do you mean to say when you want to review code? Personally, I have roughly three distinct levels of things that I comment on in code reviews: 1. Does not need response, but is my personal opinion about how the code probably would look nicer. In this case, I would do #2. If you just say "no," that's fine. I can revisit the issue later. Some people think these comments don't actually belong in a code review, and…

+1 to the comment about using "nit:". I also don't think it's fair to block code changes for style/quality/whatever that is the same or better then the existing code base. If it's not making the existing codebase worse on average than don't block.

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

#176

Post by daveevad »

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…

> socially retarded

I used this term all the time growing up and I'm trying to stop because it's definitely offensive to some people. Have a good day.

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

#177

Post by withinboredom »

fknorangesite wrote: post #174
withinboredom wrote: post #167

Earlier quoted context omitted.

Considering this story is over a decade old, and I was in my late teens ... I'd hope that I've grown quite a bit even though you're using the present tense like you know me.

The way you told the story it comes off as much more recent. > I'd hope that I've grown quite a bit I hope so too, because "I hadn't had my coffee yet har har" is a pretty thin excuse.

> because "I hadn't had my coffee yet har har" is a pretty thin excuse.

I'm pretty blunt/rude in the mornings, within half an hour of waking up, even these days. Back then, I did not know that about myself -- or rather, just how rude I came across.

Most people I interact with never see me like that, but it generally takes me 10-15 minutes to get some coffee going. Hence why I said "before coffee" since I assume nearly everyone is a little weird within the first 30 minutes of waking up, which also tends to be before a morning coffee.

People who wake up "ready to go" seem to be pretty rare. Granted, my number of people I've had the opportunity to be around when they wake up is only a few hundred people (basic training, etc).

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

#179

Post by Galxeagle »

In addition to other comments saying to add reasoning, I’m a big fan of the ‘MoSCoW’ method (must/should/could/would) to remove ambiguity around if things are a question vs a command vs a thought. ‘You must do x’ sets a clear condition before you approve a PR. ‘I would have done y’ shares perspective and experience without blocking when you think you might be just bike shedding.

I’ll usually link something like this page to the repo and align on the meanings of each phrase.

https://en.m.wikipedia.org/wiki/MoSCoW_method

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

#180

Post by eropple »

matsemann wrote: post #81
vsareto wrote: post #29

There's basically two categories here: - you have an opinion on how things ought to be done and want a dialog - you see some code that's wrong or violates an agreed-upon rule and so it should be changed with no discussion I'd switch the tone based on what you're addressing. Giving a rationale when you share an opinion or point out a mistake also softens the tone (for the better IMO). >* Should we extract this to a se…

I also think it depends a bit on the other person and the rapport one has with them. At least I try to remember back to when I was a junior, and how "personal" the feedback felt when I was just starting. So when reviewing for others in the same situation I make extra sure it doesn't come off the wrong way by adjusting my tone. But for someone I've worked with for years we're often a bit shorter and to the point, and…

The best way I've found to introduce junior developers to code reviews is to pair on it. Just go side-by-side down the diff and hop into an editor when necessary. It also helps teach a developer how to review code and what to look for--which has knock-on benefits of its own.
Post reply on HN