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

291–300 of 309 posts

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

#291

Post by librish »

withinboredom wrote: post #272
librish wrote: post #219

Earlier quoted context omitted.

I think you're receiving pushback in the comments because it comes off as though you think it's OK to be rude in the morning. Additionally, if you made someone cry I think most of us draw comparisons to similar situations we've seen where, generally, the person who thinks they're just being blunt is actually very offensive and usually the most sensitive to critique.

I don’t think it’s “ok” per se, but it’s something I’ve had to live with, as well as my family. I can honestly say that it’s not something I do intentionally, it’s as though my “social filter” is booting up and I just say the absolute first thing that pops into my head, with the wrong inflection to top it off. I’ve said some pretty fucked up things in the first 30 seconds of waking up. I own them, apologize for them,…

“Express this as X for better performance” doesn't make people cry. So either

A) Your co-worker is more sensitive than anyone I've ever met B) You are expressing yourself much harsher than you think you are

Again, I don't know you but pattern-matching based on my experiences it's almost always B.

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

#292

Post by Morgawr »

devnulll wrote: post #262
Morgawr wrote: post #208

Earlier quoted context omitted.

> * Don't give orders ("Do this", "Do that") to your colleagues, since they are not your employees, and it doesn't help promote learning & growth by not exposing the reasoning behind the demand; I think this depends a lot. I do "readability" reviews at my company to make sure people apply good coding standards and adhere to the coding guidelines that we have. A lot of these are very "mechanical" in nature and when I…

> I do "readability" reviews at my company to make sure > people apply good coding standards and adhere to the > coding guidelines that we have. Consider avoiding the entire formatting arguments as part of the code review process. There are excellent linters for every language that can run as part of the check-in process. GitHub + Actions can automate every aspect of this using all turkey stuff. Automating where poss…

> Consider avoiding the entire formatting arguments as part of the code review process.

That is not going to help me if people send code to review that has formatting problems. We catch a lot of stuff using automated tools, but there's some stuff that you cannot catch. Some people declare every variable as global rather than private, and then I have to follow up with "This variable/constant should be private, any reason you marked it as external?" etc.

You can't just automate everything, and we also have our own specific code formatting guidelines that a lot of tools don't automatically detect by default. I have raised some of these points to our toolchain and engineer productivity teams but it's not my job to dig deep into our tooling and automation so until there's better support in our internal tools (we don't use github + actions) to detect that, I'll keep pointing out any formatting mishap that gets through our linters.

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

#293

Post by wutbrodo »

CrimpCity wrote: post #287
wutbrodo wrote: post #268

Earlier quoted context omitted.

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

Yea, it's weird. I don't think I've actually really encountered someone smart and experienced who has thus problem. They tend to be secure enough in their own intelligence that honest consideration of their mistakes and gaps doesn't send them spiraling into insecurity and ego-protective backlashes.

I can only speak to my limited dataset, but it's a lot more correlated with self-esteem than actual skills IME.

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

#294

Post by gwbas1c »

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'm curious, what happened in the long run?

Was this developer generally unprofessionally sensitive? What happened to your manager?

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

#295

Post by Kaze404 »

graftak wrote: post #274
kqr wrote: post #172

Earlier quoted context omitted.

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.

To me ’…don’t you agree?’ is the sort of rhetoric that parents use to make their kids internalise their beliefs. If someone disagrees I expect them to speak up, if they’re not so inclined I’m interested to know why, so we can address/resolve that and move on.

The comment says "do you agree?", not "don't you agree?". I agree that the latter sounds condescending, but the former sounds reasonable to me.

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

#296

Post by greenie_beans »

hnruss wrote: post #258
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…

sounds kinda nice...where do you work...?

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

#297

Post by menaerus »

hnruss wrote: post #258
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…

What's the point of reviews if author can unconditionally merge the PR without addressing the comments?

I'd feel very low to none incentive to spend time first understanding the problem and then reviewing the solution to the problem if I knew that my opinion can be disregarded just like that. Seems like a waste of time from reviewer POV.

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

#298

Post by agentultra »

rjdamore wrote: post #217
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…

People are sensitive. Directness is underappreciated. Inability to take critique is killing code quality. Try going to an art school. Critique is how we get better. Check your ego at the door and you'll go far as a programmer

They have courses on constructive critique in art school!

I think software developers would do well to take such a course.

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

#299

Post by withinboredom »

gwbas1c wrote: post #294
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'm curious, what happened in the long run? Was this developer generally unprofessionally sensitive? What happened to your manager?

I left the company within the next few months, the manager is still doing well. I haven’t kept in touch with the other dev.

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

#300

Post by gwbas1c »

withinboredom wrote: post #299
gwbas1c wrote: post #294

Earlier quoted context omitted.

I'm curious, what happened in the long run? Was this developer generally unprofessionally sensitive? What happened to your manager?

I left the company within the next few months, the manager is still doing well. I haven’t kept in touch with the other dev.

Good move.

I had a similar overly-sensitive colleague. (She was support, and would get flustered at my short responses like "please file a ticket" or "logs.")

She left the day after I met with her and explained common sense. (Customers expect you to know how to use the product; don't bypass the ticketing system by emailing me directly; follow the documented process to escalate to engineering; don't just copy and paste customer emails without doing your own investigation...)

I was having a lot of trouble with getting my manager to intervene with her, so in this case her departure was a godsend. We ended up firing my manager a little later, and things ran so smoothly afterwards that I realized he was the real source of the problems.

Post reply on HN