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

251–260 of 309 posts

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

#251

Post by wutbrodo »

withinboredom wrote: post #177
fknorangesite wrote: post #174

Earlier quoted context omitted.

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

> People who wake up "ready to go" seem to be pretty rare.

I know this is a complete tangent, but do you find this to be true specifically of habitual coffee drinkers? I have a pretty horrid history with a sleep disorder, and while I'm a little slow after waking up, I don't think my mood is especially worse. I wonder if it's simply because I don't have a stimulant addiction like most people do.

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

#252

Post by wizofaus »

None of the above - I'd explain why I think the way the code was written was problematic (e.g. function logic can't easily be readily comprehended in one go, consider how you could break this down). But to a large degree my "tone" and level of detail would depend on the seniority of the author.

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

#253

Post by contingencies »

If you really insist on style review, how about defining a metric which allows the autonomous analysis of the code and autonomous recommendation？ Then you can put the rationale in a wiki and maintain stats on how often everyone gets caught. If the thing is so stylistic you can't spec it as code, why waste people's time reviewing？

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

#254

Post by eek2121 »

I try to be polite, yet precise. Remember that the author likely put a ton of work into their code. I usually let them know that if they want to collaborate further, we can huddle on slack. I push others to do the same for me and have done so when needed.

I guess I will say this: Code review isn't about who can write the best code. Nobody can write the best code. Rather, code reviews help push the team to ensure that only the best code gets pushed.

I guess the only other advice I will give is to never criticize code without suggesting an alternative approach. Present that approach in the pull request and not privately. Accept criticism on the behalf of the code writer as well as others. Understand that no single developer will have the perfect solution to a problem, even when comparing juniors to seniors. I've been in this business for more than 20 years, and I still get surprised by fresh blood. If you aren't surprised, you aren't growing as an individual developer.

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

#256

Post by xbar »

BeetleB wrote: post #159
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…

I've long wanted to write a blog post on applying what I learned from effective communications books to code reviews. Your comment mirrored something I wrote in another thread about the problems with the Socratic method in general[1]: "If you have a concern, then express the concern openly before asking your question. This will make it clear to the recipient what your intent is, and they will not have to guess." The…

Seeing an open source project's reviewer begin with "Why did you do it this way instead of X?" is the the most shocking shutdown I have seen this year.

Buddy, you let that issue languish for a year. How about let my junior dev friend have a go without being a nitwit reviewer?

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

#257

Post by SilasX »

osigurdson wrote: post #249
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…

The original post seemed to be concerned about tone, not content.

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.

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

#258

Post by hnruss »

lxe wrote: post #215

Here's a trick: always approve the code review, unless it's an actual bug, a security risk, or obvious quality issue. Then you can use whatever tone you want without much damage. "approved, but I think this should be in a separate function"

This is similar to how my team operates.

On my team, the PR author is ultimately responsible for their work. All review comments are considered suggestions. The Approve button is used only to indicate completion of the review. If a reviewer has a problem with the merged work, they can open a new case describing the problem.

We train all developers to have sufficient competency to work on any part of the code. If some code is too complex for a developer to understand, it is simplified. Documentation is prioritized and included as part of the work effort.

We have worked together this way for about 7 years, with high developer satisfaction and low turnover. The quality of the product has steadily improved during that time.

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

#259

Post by CrimpCity »

wutbrodo wrote: post #246
resonious wrote: post #242

We have no guidelines where I work. People have different styles. I think the only one I particularly dislike is * Please extract this to a separate function. And I think my favorite is * Should we extract this to a separate function? Asking the person for their opinion seems more productive than commanding them to do something. If you word it like a command, then you're basically stating that you know better than th…

Fwiw, I used to use respect-by-default until I encountered (multiple times) people who were too emotionally unhealthy and insecure about their abilities to handle it. I literally had someone write me an impassioned email claiming that questions about why he made a design choice "served only to humiliate him, not an interrogative purpose", and that "when he did something wrong, I should just tell him what to do instea…

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 as a peer suggests you expect this person to learn from you and so you are giving them room to grow, explore and ask questions but this means they need to commit more time to solving problems because clearly if you just did the work for them then you'd arrive at a solution much quicker. Sometimes it feels like we are under so much pressure to produce that there isn't time to really master something just let your coworker flex their strength and the problem goes away.

Thinking back on your experience would you say that the extra friction comes from time constraints?

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

#260

Post by BeetleB »

xbar wrote: post #256
BeetleB wrote: post #159

Earlier quoted context omitted.

I've long wanted to write a blog post on applying what I learned from effective communications books to code reviews. Your comment mirrored something I wrote in another thread about the problems with the Socratic method in general[1]: "If you have a concern, then express the concern openly before asking your question. This will make it clear to the recipient what your intent is, and they will not have to guess." The…

Seeing an open source project's reviewer begin with "Why did you do it this way instead of X?" is the the most shocking shutdown I have seen this year. Buddy, you let that issue languish for a year. How about let my junior dev friend have a go without being a nitwit reviewer?

> Seeing an open source project's reviewer begin with "Why did you do it this way instead of X?" is the the most shocking shutdown I have seen this year.

Thing is, in most cases it's not meant to be nasty at all. Their intent usually is to point out they think there's a better way, and are either trying to say you should do it that way, or are trying to invite you to give your reasons. The problem is they're phrasing it in a manner where there are multiple interpretations to their intent.

Given that commenters responded negatively to it - beyond what I had originally meant when I highlighted it - makes it a good example of how not to communicate.

Post reply on HN