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

> 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 possible avoids all the arguments, actually enforces standards, and saves time for everyone. Using "Standard" linting rules (aka: The defaults) also help promote good practices for everyone.

How do we know the OP had any issues coming up with valuable and insightful comments? Tone can have an impact - being particularly rude is not likely to result in good outcomes (but maybe it could in some situations). Why aren't we exploring what was originally asked?

All of the above are fine. If both parties know that the review is about the code and not the author then tone is more or less irrelevant.

I’d prefer something like this: “I think we should extract this logic/code/etc into a separate function for better testing, less code duplication, and better maintainability.”

You only have to say that a few times before you can use the short hand: “Extract this into a function.” because you stated reasons before.

> 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/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 actually mean this less negatively. In my current job, I work with extremely talented and intelligent colleagues on an applied research system that also requires some heavy-duty engineering. Writing high-quality code is only one of the many skills required to be effective, and many of them handily beat me at any number of skills. There's a no-ego recognition that my ability to write and review clean, safe code massively outstrips theirs, in the same way that I recognize coworkers with (eg) ML or even execution expertise that outstrips my own.

In this context, I have relationships where both of us are extremely clear that my review of their code is not a meeting of peers, but a (relative) domain expert reviewing someone else's work. Again, these colleagues are intelligent and driven, so they are hoping to grow from these reviews and will absolutely push back if they disagree or don't understand a suggestion. But there's a baseline of mutual respect and a clear-eyed understanding of the asymmetry in expertise.

This means that I can use assertive statements to communicate confidence that my view is correct without offending ("This docstring implies that the blonker is a plumbus. Clarify that it's a blorp"). Conversely, I'll use less assertive language for suggestions that are ambiguous or subjective: "Hm, I think it might be cleaner if we move the foobar into the bazqux. Wdyt?"

I don't think any of this implies that they are turning off their brain and deferring to me; I think it's just a shared prior that my suggestions are likely to be correct and that they will agree with them on sight. I do my best to add brief reasoning to shorten the number of roundtrips due to disagreements or gaps in understanding, and I'll occasionally say, "If you still disagree, I don't feel too strongly about this".

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

IMO it comes purely from the aforementioned unhandled self-esteem issues. It's not been generally applicable to my current colleagues, whom all have my deepest respect. It's a relief and a reduction in mental load to be able to communicate directly, but using flowery language doesn't actually cost much walltime.

Where I work there is a readability review like mason55's (probably the same company). Most of the comments I see are not about formatting. They're more like "Take parameters by std::string_view instead of const std::string&" or "Use a reference member instead of a pointer member if it'll never be null."

Applying a linter to a giant company-wide monorepo can be difficult because of decades-old code that predates certain style rules, or some teams that might have created their own rules. Even if the linter only applies to diffs, a small change to a line can trigger a lint warning on that line, and having a ton of those can be annoying.