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

81–90 of 309 posts

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

#81

Post by matsemann »

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 everyone's happy.

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

#82

Post by nickjj »

I think setting team goals are important before anything. For example if the goal is to create the most maintainable and best version of the code in the long term I think this opens up doors for feedback not being taken personally as long as the person giving the feedback isn't dropping lines like "why didn't you just do xyz instead?".

I tend to provide more open ended questions like "what do you think about ..." where "..." could be a few options to take. In my mind I've most likely picked one based on personal preference but I like proposing a few choices to spark some discussion amongst team mates and give everyone a chance to contribute. Often times throwing out a few options will result in someone coming up with another option that only became apparent after they've seen a few alternatives (which is a good thing).

It also lets more folks make decisions and that's a very important thing. If you keep making every decision then folks won't feel like they can contribute anything because "why bother". I remember an old TNG episode (S3E21) around this with "Broccoli" when he was trying to integrate on the engineering team but Jordi kept alienating him. Picard dropped some really good life advice in that episode!

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

#83

Post by no-dr-onboard »

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 share praise creates a climate where feedback is viewed as a gift rather than an attack.)

    - Kindness (It costs nothing to be kind, even if you do not believe someone deserves it.)

    - It's impossible to know everything (You can't know how your words are interpreted without asking.)

    - Short toes (GitLab is a place where others can feel comfortable with others contributing to their domains of expertise.)
Specific to your situation, I'd focus on what others have said as well. If they *need* to perform something as part of their job role, remove the suggestion and place an imperative. Ex: "This needs to be a separate function" vs "You should. . .".

> Communicate assuming others have low context. We provide as much context as possible to avoid confusion.

It also helps to provide context, especially if you have a particular fix in mind. "See this PR/MR for someone who encountered a similar situation recently."

Hope this helps.

1. https://about.gitlab.com/company/culture/all-remote/effectiv...

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

#85

Post by EuanReid »

Obviously tone is hard in text, and it's worse in multilingual teams. What's been very effective for my current team is adding explicit context as a comment prefix with standardised terms: - Nit for "it doesn't actually matter" - Suggestion for "this approach might be better, what do you think?" - Question for "help me understand this" - Requested Change for "this is actually a blocker"

We include this expectation in our working agreement, stick to it rigorously for reviews where reviewer and reviewee haven't established a strong rapport yet, and use it as appropriate beyond that. So far we've had no confusion about tone in reviews.

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

#86

Post by JamesSwift »

Default to short, polite statements, but make it clearly suggestive if its optional.

e.g. "Extract this to a separate function please" if you aren't asking. "Should this be a separate function" otherwise.

This also assumes that theres a certain protocol to PRs in terms of veto power. I've always followed the rule that the writer must make all requested updates by reviewer unless explicitly challenged. In other words, you can't silently not change something they mentioned, but you can say things like "thats out of scope for this update" or "I think this design is better because XYZ" and the writer is able to mark as resolved at that point.

Sometimes this ends up with a conversation that needs to happen where the reviewer disagrees with the writer, but honestly it rarely does at least on the teams I've been on. It would really have to be a contentious point to escalate to that level rather than just being a difference of opinion.

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

#87

Post by ginja »

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…

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.

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

#88

Post by lhnz »

"How would you feel about extracting this into a separate function? It might be better because the logic seems quite self-contained, and we can then apply the extra business logic immediately after this with a few if-elses."

I try to give critiques in this way, as it gives the person that wrote the code ample opportunity to defend their approach, but also gives explanation of why your suggestion might be better.

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

#89

Post by comprev »

ianmcgowan wrote: post #21
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…

You sound like a Dutch person. Working with different cultures I've had to adapt to varying levels of directness but like to think it's helped me become more up-front. I've learned to appreciate people that say what they're thinking, so you can avoid the 2nd and 3rd order analysis about intent and thoughts about how other people will receive things.

I found the Dutch directness a breath of fresh air when working in NL.

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

#90

Post by broast »

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 agree with this. Don't give demands, give reasoning and examples.
Post reply on HN