Hacker News Reader

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.

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.

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.

The best way I've found to introduce junior developers to code reviews is to pair on it. Just go side-by-side down the diff and hop into an editor when necessary. It also helps teach a developer how to review code and what to look for--which has knock-on benefits of its own.

My company used to do this as default (pre-wfh and other changes), now no-one really thinks of it as a possibility which is a shame.

I was junior for most of that time and I think "use a totally different approach" or "change these 10 things" can sting a lot less that way. Your mistakes aren't all in black and white for the world to see, people can tell when you get something and don't end up patronising you, but you can also blur the lines a bit between "hey, what do you think of this, not sure" and "I think this code is 100% ready for merge and officially request permission to do so" - like, I know there are going to be comments, pretending otherwise will just make it worse.

Most of the time you'd get a review immediately too, less complicated structure then so one approval was usually enough, and it made sense for that to be high priority in my mind - this task is one step away from being done, I don't mind being interrupted.

The only thing is when I was the reviewer, it maybe went a bit too far one-sided learning experience, sometimes comments would get brushed under the carpet without much explanation/there was an assumption I'd approve.

Another thing that people don't often consider is that while some really appreciate being given a pointer to the rule as a justification (these are in a wiki / code style doc usually), many people will chafe at the "pedantry". In general, those second group need a bit thicker skin, but it is something to be considered.

There are different categories of things you might see:

1. This is wrong. I can tell from reading your code that it doesn't do what the description of this PR or the name of a test or some other indicator shows its supposed to do. This should be communicated in a tone of "you must not merge this".

2. This violates our agreed-upon style or best practices. The strictness of enforcement is part of that agreement and company culture. At my current company, this would be communicated along the lines of "this is not how we prefer to do this, so unless you have a good reason why our standard method is wrong, change it"

3. This is confusing to me, or I have a suggestion for improvement. This should be communicated as a suggestion or general feedback. If you're getting miffed about people not taking the suggestions, maybe it's actually in a category above, or maybe you need to adjust your own assumptions about how much stylistic consistency to insist on, since it sounds like you don't have a consensus.

4. You are new, either to software engineering or at least to this company, and your style is inconsistent with how things are done in this code. This is the same as #3 but should be communicated more strongly and depending on your company normals may be effectively a requirement.

> 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 resolve that included:

"I can't approve this scheme, please do X instead."

"This API isn't OK to use here, you have to..."

"Under no circumstances will I ever approve of a feature that does this."

You can phrase those as nicely as you want, but it's imperative that your disagreement be spelled out explicitly.

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

>>* This could be extracted to a separate function.

> This one's the least useful. You could do a lot of things with code. It doesn't resemble the command or inviting tones of the other examples here.

It's clearly a politely phrased command, and to think otherwise is intentionally missing the point.

What about clarifications? In a sense when you're not sure if this is good code, and would need to get more details. I sometimes just ask directly ("Why is this there?"/"What does this do?"), but other times I just don't bother and let others review.

This is really useful. Thanks.

<sarcasm> I like your suggestions... I would add that they are more effective if you keep a background noise with whips cracking sounds and random screams </sarcasm>