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

131–140 of 309 posts

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

#131

Post by graftak »

ginja wrote: post #87
matsemann wrote: post #81

Earlier quoted context omitted.

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.

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.

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

#132

Post by [deleted] »

mmazing wrote: post #128
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…

.. and sometimes after going through that exercise you might find you don't have a good reason for what you're asking for. :)

[deleted]

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

#133

Post by MichaelMoser123 »

Smatbear has a great article on code reviews https://smartbear.com/blog/avoiding-the-politics-of-code-rev...

He has the following suggestions:

    - "Ensure that reviews are two-way. Never have people who only review and people who only get reviewed."
    - "Always focus on the code and not the person who wrote the code."
    - "Make the reviews small, frequent, and informal. Marathon group sessions in rooms make people defensive."
    - "Frame things as questions and suggestions rather than orders and accusations. Ask that others do the same."
    - "Automate as many checks as possible so that reviews don't focus on simple details."
    - You can frame the review as optional "asking for advice" instead of a gatekeeper approach of "getting the code approved"
    - Says that the potential harm of the bad approach is worse than taking up the risks, that is taking the risks that come with the policy of not requiring a code review for each and every commit.

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

#134

Post by agentultra »

I think it depends on your teams' expectations for code review. It seems like perhaps your team doesn't define what is acceptable/constructive/valued, etc from reviewers.

Personally I prefer the first one on this list: it gives the author the opportunity to refuse the suggestion. Perhaps its a frivolous request and the PR is already been open for far too long. Maybe the author would prefer to leave the duplication there until there's more justification for extracting it to a function. Who can say?

Not all feedback is necessarily good or useful.

Personally I don't bother with suggestions like this where it's more of a style preference unless there is a style guide to adhere to where "extracting this to a function" would clearly connect to a guideline.

I tend to focus on verifying the that the author did their homework: what evidence/proof did they submit that ensures me the change is correct wrt. specs/requirements/tasks? If that evidence/proof is sufficient that's all I care about: it makes it easier to manage a higher volume of review requests.

I have a pet peeve for suggestions that are "trivial" and based on, "well I would have written it this way," as I don't find them terribly useful most of the time. They rarely affect the specification of the program and their claims to readability/maintainability are often dubious and superficial. Bike shedding is a huge waste of time in the review process. At least when worded with, "Should we..." there's a chance for the author to explain whether they had already considered that or accept the suggestion as helpful and make the change.

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

#136

Post by kissgyorgy » 

    All of your examples are wrong, because you don't state WHY you think it would be better. Never give instructions without at least a short explanation.
Better tone of the above sentence, without "blame game":

    None of your examples include WHY you think it would be better to do something. I never give instructions myself without explaining at least briefly why I think my suggestion is better, because they more likely understand the problem or accept my suggestion.
Even if you think something is better only because your taste, it's still better to explain yourself, so you can agree to disagree. I usually let go small things if something is readable and functionally not different.

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

#137

Post by koolba »

Depending on the situation, one of the following three:

> Should we extract this to a separate function?

When you are not familiar with the codebase, potential reuse, and want the submitter to clarify rationale for the approach.

> This should be extracted to a separate function.

When you are the authority but you're open to a discussion. You should include the "why" aspect in this situation.

> Extract this to a separate function.

When you are the authority and there will be no discussion. If you want you can include the "why" but it's a waste of time. You are the law.

The rest are sugar coated versions of the same thing.

"I think...", "I would...", "Maybe we...", are unnecessary additions. Clearly it's what you would do as you're the one giving the review.

If people are too thin-skinned to take direct feedback then they should work on improving that before submitting more code. Life is too short to waste time picking words that try not offend when you can spend that time actually producing something of value.

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

#138

Post by StellarScience »

Based on a great article we read almost 15 years ago ("Effective Code Reviews Without the Pain": https://www.developer.com/guides/effective-code-reviews-with...), we start most code review comments with "Did you consider...?" This makes it non-confrontational and non-judgmental, as the author can easily reply that, no, they hadn't considered that and thanks for the suggestion.

Everyone in the company knows it's sort of a gimmick, and we even make fun of it a bit, but even so it still works!

"Did you consider extracting this code into a separate well-named function with clear inputs and outputs, to make the original function smaller and easier to reason about and maintain?"

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

#139

Post by gonehome »

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…

+1 the reasoning is the entire point and all of these examples are good.

Just telling people to make changes without telling why doesn't teach them anything - especially when sometimes what's being requested isn't actually better (or is at least subjective).

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

#140

Post by dominotw »

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…

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 usually subjective too. you had to explicitly mention 'security issues' which imply that you have a feeling that most of your pr comments were your subjective interpretation .

People are more open to suggestions during pair programming because people don't deliver feedback in cutting way ( with or without coffee) and its easy for get a common understanding and move forward.

Post reply on HN