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

201–210 of 309 posts

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

#201

Post by BeetleB »

adhesive_wombat wrote: post #196
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…

> Why did you do it this way instead of X? Argh, yes, that's one line that might make me just walk away from a PR as a new/junior/casual contributor. You, the reviewer, are an expert in the system. Likely you are the or one of the most expert people in the entire world on this exact thing. You know X exists and why to use it. As you should, because you put it there. You also should know that people who aren't experts…

> You, the reviewer, are an expert in the system. ... You also should know that people who aren't experts (like me) don't know about it, simply because they didn't use it, in this PR, when they should have.

While this happens, I find that it's very common that even a junior developer has some insights the expert reviewer doesn't by virtue of the fact that he/she has spent more time on this specific problem than the reviewer has. As a reviewer, it's always better to assume you are not the expert.

> Of course, if I'm also supposed to be an expert, e.g. a co-maintainer, then it's different. I should know X. Which means either it's a brain fart, or I actually do have a reason. In which case I should have commented on the code, because if another contributor can't tell the intention in the PR, then they can't tell in a year when no one can remember why it went that way.

I disagree, and your example is a good case of not considering all the possibilities.

Yes, in certain cases, X may be the obviously better approach, and a comment would be a good idea. In many/most cases, there are N approaches, and you happened to pick one. If your approach has advantages over X, I don't see a need to justify it as a comment in the code because you then should put comments to justify it over the (N - 2) other solutions.

At the end of the day, the burden is on the reviewer to specify why he prefers X. Only if he presents a case ("X is better because of reason Y") should the developer feel obliged to justify the decision.

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

#202

Post by tayo42 »

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…

Crying? Did you leave some Linus Torvalds level feedback, like tell them to find a new career or something?

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

#203

Post by woojoo666 »

thewebcount wrote: post #187
hcarvalhoalves wrote: post #41

My rules for reviews (which influence the language used) are: * Criticise the code, not the author ("There's a bug here" vs. "You inserted a bug here"); * Reach common agreement that people shouldn't cling to code written ("sunk cost fallacy"). Code is liability, and it's okay to throw things away and restart from scratch, the value is in the learning; * Don't use questions that invite discussion if you're not inviti…

> Avoid bike-shedding about style/formatting/etc. If this is important where you work, automate with tools; I'd love some suggestions for how to get people to avoid this type of bike-shedding. While I'd love for us to use some tools to automate this, we have a huge codebase with existing code that doesn't follow the rules, and it's not in my ability to implement said tools. (Big company, lots of overhead, will affect…

Set up the linters to only lint code that was changed. This way legacy code doesn't need to be fixed all at once, but will slowly get fixed over time

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

#205

Post by convolvatron »

gonehome wrote: post #139
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).

my rule is to weigh whether or not its worth making a comment very carefully. nothing without a clear justification. never say 'I would have done it this other way' - unless its a good friend who you often discuss style issues with. comment on things that you feel would make a substantial difference on schedule, maintainability, or function - and provide a clear reasoning with which to argue against.

wasting your peers time trying to enforce your style isn't just non-productive, it sours everyone on the whole exercise.

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

#206

Post by colonwqbang »

ajross wrote: post #149
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'd switch the tone based on [whether you want a discussion or are insisting on a change] I think that's a great example of where "tone" is absolutely not sufficient, especially in the modern world where many readers aren't going to share your first language and won't absorb the nuance. If there's a situation where your disagreement is absolute, you need to say that factually, ideally with a recipe for how to resol…

It's sometimes refreshing to have people just write what they want me to do, clearly and without embellishment. Even if it's sometimes nitpicking, I know that if I just fix it I can then merge my patch. I don't have to wait for a second look from them because their comments were so direct "write this instead".

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

#207

Post by Deradon »

justinram11 wrote: post #9

[Suggestion] Use conventional comments[1] to flag how important the comment is. Most of my comments end up being "suggestions", but then when I put a [blocking] on it, it clearly communicates that I think this should be fixed before merging in. 1: https://conventionalcomments.org/

Offtopic but related:

Conventional commits: https://www.conventionalcommits.org/en/v1.0.0/

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

#208

Post by Morgawr »

hcarvalhoalves wrote: post #41

My rules for reviews (which influence the language used) are: * Criticise the code, not the author ("There's a bug here" vs. "You inserted a bug here"); * Reach common agreement that people shouldn't cling to code written ("sunk cost fallacy"). Code is liability, and it's okay to throw things away and restart from scratch, the value is in the learning; * Don't use questions that invite discussion if you're not inviti…

> * 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 am reviewing changes that have obvious coding standard mistakes I will write comments like a "linter" would. One of the most common examples:

> Use descriptive ("Uses") rather than imperative ("Use") style for function docstrings. See:

I think this type of "giving orders" in reviews is fine as long as it's clear this is an obvious misuse/misunderstanding/miss of the readability guidelines. For any other kind of comment I'll phrase it differently ("Is there a reason why you did X? I think doing Y might be more appropriate because , what do you think?", etc) but for stuff that linter should've caught I don't think it's necessary to be verbose and "sweeten" the comments too much.

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

#209

Post by adhesive_wombat »

BeetleB wrote: post #201
adhesive_wombat wrote: post #196

Earlier quoted context omitted.

> Why did you do it this way instead of X? Argh, yes, that's one line that might make me just walk away from a PR as a new/junior/casual contributor. You, the reviewer, are an expert in the system. Likely you are the or one of the most expert people in the entire world on this exact thing. You know X exists and why to use it. As you should, because you put it there. You also should know that people who aren't experts…

> You, the reviewer, are an expert in the system. ... You also should know that people who aren't experts (like me) don't know about it, simply because they didn't use it, in this PR, when they should have. While this happens, I find that it's very common that even a junior developer has some insights the expert reviewer doesn't by virtue of the fact that he/she has spent more time on this specific problem than the r…

> In many/most cases, there are N approaches, and you happened to pick one.

I'm not sure that's always true. A lot of the time, while there might be, in a vacuum, many ways, in the context of a specific project (or part of), there's usually one "most right" way to do it, considering local style, idioms, norms and conventions. Admittedly, this might be more true in some languages and applications.

Sometimes there are multiple ways. Usually this means that one or more disparate legacy ways are extant, but there's a preferred way to move towards.

Having PRs storm off in "exciting" new directions, technically correct or not, is usually a bad idea once that merges and is now everyone else's problem.

If all alternatives are equally valid logically, it's likely they have practical implications that differ. Perhaps one might consider a std::set better than a std:: vector in the abstract for your task, but maybe you know something important about the expected number of items or memory access patterns. This is when you should comment if that's not obvious to someone who isn't literally just done writing the change.

> At the end of the day, the burden is on the reviewer to specify why he prefers X.

If a reviewer is going to pull me up on truly equivalent things that have no impact on correctness or maintainability, then I'm probably not going to voluntarily be a co-maintainer with them. If I've consented to be a co-maintainer, then I have either mutual respect for the others, or I'm being paid enough to make it worth it. And indeed, I have refused maintainership offers in projects with ":eyeroll: why didn't you just magically intuit X" cultures.

Post reply on HN