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

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 duplicates the same set of processes as over there. Refactor these into a single function, and we'll be good to go.

* While I don't have feedback yet on this function, it's too long for me to follow. It would be easier to read and for future maintenance if you refactor this part into a function.

* Since we're in closedown we can only take certain types of changes. If you refactor this into a separate function in the library, this change can be accepted.

* Or, I hate to be the process person, but the internal guidelines for this team call for all code to be structured the same way. Refactor this part into a separate function and I'll approve the PR.

There are lots of ways to provide feedback. I suggest stating the problem with the code and providing a solution. If that's the only possible solution to get past your review, state and don't ask. You can also give a carrot with "do this and I'll approve the merge."

How would you speak when sitting next to the person face-to-face? What tone do you want your boss to use when providing a performance review during a 1on1?

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 worst comment I see in code reviews (and sadly, all too often) is:

"Why did you do it this way?"

I have no idea why I'm being asked this. The answer is "Because it solved the problem."

Even this is problematic:

"Why did you do it this way instead of X?"

Possible (unhelpful) answers:

"Because I didn't think of X." (I still don't know if you want me to change the code and why).

"Because it solved the problem."

Your examples are good ones on how to ask this question "This could have been solved via X, which has the benefits Y and Z compared to your approach. I suggest changing this to use X, unless your approach has advantages that I'm unaware of."

Probably about half the times I get something like this: Yes, my method did have advantages the reviewer is not aware of, and we then have a discussion.

[1] https://news.ycombinator.com/item?id=31889518

> 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 (like me) don't know about it, simply because they didn't use it, in this PR, when they should have. Why don't they know it? Probably because you haven't used it consistently in your own code, or it's not documented. This newbie has cobbled this PR together from what sense I can make of this project. Probably 90% is guessed from code I found in there already.

What wouldn't wind me up?

"I think a better way to do this is X. It's better because Y. Or have I missed a specific reason for X?"

Note two things: 1) explanation to a noob of reason Y, which may well be valuable, not only to the noob, but also in the record of the project in general. 2) The indication that the noob might at least have had a logical approach, and they're not an idiot, just a noob.

Afterwards, if this seems like something the noob should have known from the codebase, consider that you, the maintainer, have failed to make it clear.

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.

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

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

> "Why did you do it this way?"

While the question is literally (and without any other thought) what I have sometimes in mind, usually I ask something like:

I feel I might be missing some insight or context, could you walk me through your rationale?

Because if I don't get some code change I may very well be missing something. And in any case by talking we're only going to improve our mutual understanding, both of the codebase and of each other's thought process.

Just wanted to chime in and say that I still remember the first code reviews I received and while I didn't take any of the comments personally, they certainly didn't feel great to receive. (I should add that I don't begrudge the reviewers, they were nice people).

These example notes are wonderful. They feel like an editor's notes, not a graded exam. Something you'd get from a colleague who is collaborating with you and not some infallible black-box oracle.

It's your standard Socratic Method[1] that's devised to help you (the learner) arrive at solutions on your own terms. Just don't push it too deep - others may become a little too annoyed, and, well, read up on Socrates to learn what happens next

[1] https://en.wikipedia.org/wiki/Socratic_method

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

Agree with adding additional reasoning, regardless of the actual tone.

I’ve received some feedback along the lines of “LOL Wut?” on a comment in code, which I really didn’t know how to take or respond to (suffice to say its meaning was vague, and generally unhelpful).

These suggestions hit the nail on the head. Additionally, the reviewer also becomes a better dev because they’re forced to really think about and articulate why they think the code smells.

Even if my coworker’s code doesn’t feel right to me, I’ll sometimes forgo commenting if I can’t articulate why. My goal is to avoid bugs and deliver features, not to force my code style and opinions on others.

Where possible it can also be useful to point out if there are existing guidelines/checklists the submitter should be referencing to understand these preferences of the project. The more the contributors can code review themselves prior to submitting a PR the better.

And if such a resource doesn't exist or that topic is not in the current guidelines/checklists - offering to take up the task of creating or adding an topic to the list of things to check before submitting a PR can be helpful.

Eliminating nitpicky PR comments by having pretty verbose coding guidelines as well as pretty strict settings for automatic code linters/syntax checkers/static type validators means the PR feedback cycle is a lot more focused.

And for architectural discussion - draft PRs can be useful early on for feedback on design choices for more challenging features can be helpful.

This!

Walking on eggshells because every comment chain devolves into religious battles about style? Find a linter or style checker that can automate comments/fixes. Talk about those checks in your team meeting, get consensus (who cares what it is, just consensus), and make the tools enforce it. Or suggest your manager do this.

If discussing actual code problems in code reviews rankles people, just escalate to your manager.

If you need a special code review human language lexicon because people can faint or throw punches, it's a management problem. Because that's the kind of thing that makes other people, not the troublemakers, leave the company.

I agree with the tone of your suggestions, but your suggestions still take quite some time to read. They can easily be shortened without losing any information. For example, no need to to apologize for being the process person (being overly apologetic decreases the value of apologies anyway).

Also for me it feels like you're seeing code reviews as a senior/junior thing. It's more than that, no?

I agree with this. Don't give demands, give reasoning and examples.

These work and sound great, but stating them without a "please" can come off as a little brusque. Please add a "please" before the orders, and I'll accept the advice.

Great post! One quibble with your last point — your guide is probably most helpful to the devs that are too direct and blunt, and expect others to be that way to them.

Haha! That's an excellent way to state the issue.

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

Yep: the operative word being “because”, absent in all the OP’s sentences.

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 inviting a discussion, instead expose the reason you believe something should be different ("Should this be like X?" vs. "I think this should be like X for reasons A, B, C."). This avoids wasting time and reviews that turn into long discussions.

* 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;

* Avoid bike-shedding about style/formatting/etc. If this is important where you work, automate with tools;

* Agree previously on what constitutes a non-passable review, if there's such a process.

> 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 a lot of people, don't have a lot of time, not my department, etc.)

One of the things I hate is having a different comment for every single place where I misplaced a star or ampersand. My natural tendency is to do it one way, but our style guidelines prefer it the other way, so it's something I miss frequently. Having 30 comments in a review that are all "move the star to the other side" over and over again just makes me hate the reviewer. (Particularly if they don't provide any useful feedback.) A single comment on the first one that says something like, "This doesn't match the style guidelines, and I see several others that don't match. Please fix them all," would be highly preferable. And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy!

Everyone is talking about how to get the style guide integrated into your CI. But it sounds to me like your concern is that other reviewers frequently point out your style guide violations.

You could set up some local lint rules just for yourself. You'd need to set it up to only run against code that you changed. And if you don't have an automated style guide you'd have to come up with your own. But it seems like once you get over the hump you could just slowly add rules as you get tired of forgetting about them/people pointing them out to you.

> And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy!

This is a concerning attitude. If the team has agreed on a set of style guides to follow then you should follow them, argue to change them, or move on.

If you don't have time to adhere to the style guide now, why do you think you will have more time to go back and fix it later?

Tools are easy. You add it to the CI/CD pipeline, it fails if it doesn’t meet the guideline, people have to run the tool to get the merge in. On first run it’s painful, so if you’re not starting a project, use an autoformatter so it’s not all manual work. If you really want, you can even make your CI pipeline push commits with autofixes

Of course if you don’t have a CI pipeline that’s easy to integrate steps like this, you’ve got major issues to deal with elsewhere…

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

Sorry, but formatting is on you. If you don't like the comments, don't commit inconsistent code. Set up a local auto-formatter with the right rules. If enough of the existing code you're working on doesn't conform, set the formatter to only run manually and get in the habit of running it before committing.

Are you sure it's not in your ability? Whenever I introduced something like that people were in general indifferent-to-happy, as long as you did the majority of the job and they could somewhat easily run those tools (e.g. anything written in Java is more or less a lost cause because people cling to their IDEs)

> * 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: <url-explaining-docstring-guidelines>

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 <reasons>, 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.

I agree in general, though I find the second on clinging to code difficult when people counter with timelines. It can be difficult to express to juniors how to reason about taking a bit more time to do it "right".

It depends on so many things. One sliver of input I’ll add to this discussion is that I like applying the Socratic method for some cases where the goal is not just to write correct code but to help them learn how to examine code.

“What might happen if the input is a null value?”

Sometimes they realize things on their own after one or two questions. Sometimes many. But it’s always so neutral and keeps them in the driver’s seat.

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 separate function?

>* Could you extract this to a separate function?

These are essentially the same: opening a dialog over an opinion. I'd suggest this when there's not an obvious flaw or rule violation or you have a gut-feeling about how code should be and want the author's input.

>* I would extract this to a separate function.

This is almost a command but isn't clearly one. It should be followed by some rationale, at least.

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

>* This should be extracted to a separate function.

>* Extract this to a separate function.

These are commands and are practically the same tone and best when catching mistakes. Unless obvious, a rationale should be given like a demonstrable flaw in logic or inconsistent abstraction, etc. There should be a few sentences explaining this. If there isn't a clear violation, I'd prefer the dialog invitation tones.

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.

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.

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.

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

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

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

I’ve introduced Must, Should, Could at every company I’ve been at (except the one where I picked it up myself) and it works wonders.

Prefix every suggestion with M, S or C and then just write the suggested change as a plain statement. The prefix handles the severity and importance without you having to worry about tone.

Coulds can be ignored by the coder author with no explanation as to why they are ignoring but if they have time or want to implement the change then they can. Shoulds can only be ignored with a reason and the reviewer has to agree or the change should be made. Musts have to be implemented and should be used rarely. The only person who can over rule a must is a department head, lead or CTO type person.

This massively reduces friction in the CR process.

Article here: https://allthecode.co/blog/post/how-to-get-better-at-code-re...

Yeah, that is great.

Learned something similar on a previous job. Prefix it with "#must", "#should", "#could", "#would".

We've also extended it with some other things that you may want to comment, but are not necessarily actionable. "#wont", "#idea", "#tip", "#question", "#risk", are pretty much self-explanatory.

"#fun": an ugly workaround that could've worked; some funny situation that the change could cause; ...

"#domain-knowledge"/"#debrief": really nice if you are working with people that are more junior, or not that familiar with this codebase in particular. May help other reviewers understand better why the author made certain decisions.

We do something very similar:

* NICE: Optional change. PR is approved.

* SHOULD: Highly suggested change. PR is neither approved nor rejected.

* MUST: Must be fixed. PR is rejected.

This also helps the PR author to know which of my comments have to be addressed to resolve my rejection.

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 the author without a single one of my statements addressed (including security considerations), and no one else would review it. My manager was in a tight spot, felt sorry for him.

I was literally just my pre-coffee blunt self. I usually use the first few versions in your list, which is probably why it hit my coworker so hard. We chatted and made up, but our relationship was weird after that.

So, I’d say, just be consistent. Changing tones unexpectedly might affect your coworkers and politics in interesting ways that you might not expect.

> I was literally just my pre-coffee blunt self.

I have a co-worker who's socially retarded as well and I can say that the vast majority of his comments that tend to rub people the wrong way are just badly phrased versions of legitimate opinions (that he sometimes should just keep to himself because no one asked). He's a better reviewer than you seem to be, though; probably because he can take the time to type out better versions of things he'd normally just blurt out.

Edited to add:

It's a massive chore to have a teammate that causes stuff like this and I don't envy your teammates or lead (even worse if you're the lead, obviously).

> socially retarded

I used this term all the time growing up and I'm trying to stop because it's definitely offensive to some people. Have a good day.

Considering this story is over a decade old, and I was in my late teens ... I'd hope that I've grown quite a bit even though you're using the present tense like you know me.

The way you told the story it comes off as much more recent.

> I'd hope that I've grown quite a bit

I hope so too, because "I hadn't had my coffee yet har har" is a pretty thin excuse.

> because "I hadn't had my coffee yet har har" is a pretty thin excuse.

I'm pretty blunt/rude in the mornings, within half an hour of waking up, even these days. Back then, I did not know that about myself -- or rather, just how rude I came across.

Most people I interact with never see me like that, but it generally takes me 10-15 minutes to get some coffee going. Hence why I said "before coffee" since I assume nearly everyone is a little weird within the first 30 minutes of waking up, which also tends to be before a morning coffee.

People who wake up "ready to go" seem to be pretty rare. Granted, my number of people I've had the opportunity to be around when they wake up is only a few hundred people (basic training, etc).

I think you're receiving pushback in the comments because it comes off as though you think it's OK to be rude in the morning.

Additionally, if you made someone cry I think most of us draw comparisons to similar situations we've seen where, generally, the person who thinks they're just being blunt is actually very offensive and usually the most sensitive to critique.

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

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.

Is it ironic I moved to the Netherlands and feel right at home here? (this story is from when I lived in the US)

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

People are sensitive. Directness is underappreciated. Inability to take critique is killing code quality. Try going to an art school. Critique is how we get better. Check your ego at the door and you'll go far as a programmer

Yeah, I think code reviews strike deep for whatever reason. I don't think being blunt works here even if you can be blunt elsewhere.

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.

> Why can't people pair and give each other at the moment feedback vs doing code review.

That was something we discussed at various points. We never tried it when I worked there.

> you had to explicitly mention 'security issues' which imply that you have a feeling that most of your pr comments were your subjective interpretation .

Aren't almost all code review comments? I can't think of a single one except ones that point out literal bugs. Most are "do it this way because I think it will be more maintainable" or "do it this way because that is how we do it here." In this case, I mentioned "security issues" because they were literal bugs that introduced new attack surfaces that didn't previously exist. When I read comments on my code, I make sure to read it without any inflection. Some people have really bizarre code review styles, and some are totally straightforward. Some people are lenient on whether or not there actually needs to be a change, while some people will not accept code until changes are made, no matter how little the change request is.

Knowing your reviewer is about as important as how you review code, which is why I suggested being consistent. If you're always straightforward, and then suddenly not, the reviewee is going to wonder if something is going on. Vice-versa, if you're suddenly straightforward, people are going to be offended or feel like you are taking something out on them. In my case, reviewing code within 3 minutes of waking up was a terrible idea and I have never done it since because I'm "grumpy-mc-grumpy-pants" for at least 10-15 minutes after waking up and having some coffee.

I feel like a lot of the conversation in this thread is about how to review code, but the relationship between the reviewee and the reviewer is more important than any of that and how you review code will affect that relationship whether you want it to or not.

Wow not excusing hurting people's feelings, but if you are literally crying you have become too identified with your code and possibly your job. Or else, you just cry too easily and that's a problem when working with others also.

Personally it all depends on the rest of the comments in the review, if you use one style all the time then you lose the ability to indicate which changes are the ones you feel are more important than others.

I'd use the more question style comments for the minor changes, and save the stronger comment style for those changes that are really important.

So I'd word a comment about changing a variable name to something slightly better as a question. e.g. Would accounts be a better name for this array?

While a comment addressing a security concern would be a direct statement. e.g. Use a parameterised query here, as it'll be harder for someone to exploit.

One thing to note is that when making the strong statements I like to back them up with a reason, that way it helps me to think through the comment itself, helps a more junior developer understand why they should change it and helps more senior developers to not take the comment personally.

The other side to this though is that as senior developers we need to set an example for our more junior colleagues on how to take review comments. I always try to take them in a good mood, make the changes when I agree with them and have a constructive conversation about those that I don't agree with.

[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/

Using conventional comments also forces me to rethink if my comment is really blocking or is just an optional suggestion and I can adjust the tone accordingly.

+1 for use of Conventional Comments. It's a simple lift for a noticeable improvement in clarity, and guidance on what to do next.

I use conventional comments everyday. It just helps me communicate clearly and concisely.

praise: I came here to suggest conventional comments as well!

thought: reading this style of sentence would get annoying quickly.

I don't get the point of these TBH. The usefulness of conventional commits is that they can be easily aggregated to get an idea of the types of changes that were made.

Code review comments are purely meant for human communication, so requiring a specific structure is both annoying to write and to read.

I tend to use one, at most two prefixes. "Nit", and rarely "blocker". Though I'd rather specify if something is a blocker once I make my case about what needs to be changed.

Every other comment is assumed to be a suggestion, and depending or not I block the PR by "requesting changes", it's up to the author to either accept my suggestion or provide reasons not to.

Any other strict guideline about how to write prose is silly to me. Especially the decorations section. C'mon now.

I wouldn't personally use any of these, but I'm a bit of a code review heretic.

I view (peer) code reviews mostly as an opportunity to discuss and familiarize your team with your code, and I think they should conform to the "social rules" that prevail in the rest of the world. If a peer in the real world was sharing something with me, I would very rarely command or strongly suggest they change this or that thing about it. Instead I would strive to create a collegial atmosphere by being receptive and supportive 90% of the time so that 10% of the time a suggestion I offered would be received gratefully and gracefully.

None of this can happen in a github pull request.

There was an alternative that I used to see before the PR monoculture took over that I liked. It could look like a weekly "code review" in-person meeting where a random developer would put together a presentation about a new module they'd written and project it on a wall. Mostly the other devs would listen and learn, and any suggestions they made would be strictly optional. The dev might leave feeling a bit embarrassed about something - but never feel "under the thumb" of a peer who was forcing an issue.

My tone normally is "I believe this should be extracted to a separate function, and here's WHY".

The WHY is to typically refer to some best practice, coding standards, something as neutral as possible.

I use the combination "believe" and "should" as it's friendly but still has some hint of authority. Another tip is to not just post negative review comments, I also comment on things that went well.

When I see somebody repeatedly make the same mistake, or very severe mistakes, I schedule a session to discuss the topic, as there's likely a knowledge gap or misunderstanding. Invest in people like that and make it clear that this improves life for both themselves and the reviewer. Win-win.

People can be very diverse, of course. Still, if you need to sugar coat feedback too much and are afraid of a possible backlash or drama, that's unhealthy. The exchange of good faith professional feedback is part of business.

What do you mean to say when you want to review code?

Personally, I have roughly three distinct levels of things that I comment on in code reviews:

1. Does not need response, but is my personal opinion about how the code probably would look nicer. In this case, I would do #2. If you just say "no," that's fine. I can revisit the issue later. Some people think these comments don't actually belong in a code review, and I think it depends a lot on your relationship with the code and the person asking for review.

2. Needs response, but is potentially open for discussion instead of a change to the code. I would do #4 here.

3. You must make this change or I will not accept. I would personally do #6. There is no need to use "non-confrontational" language with many people when you mean to be confrontational. There is a hard line about what you will and won't accept. If it is a major comment, you need to explain your rationale here out of courtesy to the person sending you code.

Most things that fall into the third category are around code style conventions and minor bugs. More substantive changes, like changes to an architecture, often fall into the second category.

Edit: Also, when I nitpick your code for one reason or another (which generally only happens when I am enforcing a company style guide), I will often say "Nit:" and then use the sixth style. If you are new to the team, I will quote the guide to you. Most people don't feel so bad about the nitpicks when you admit that you are nitpicking, and that it is for a reason.

Yeah I agree w/ you. I think there are three major problems w/ code review:

- reviewer ego

- author ego

- reviewer ambiguity

"Solving" ego is involved, but easy: you have to encourage trust amongst your team, and that can't happen in code review--it's too late.

Reviewer ambiguity (being non-confrontational and wishy-washy when you really mean "this is an XSS vuln, you gotta change it") is an attempt to solve ego, but always fails.

I'm not saying be a jerk, you have to act with compassion and empathy always, which is a marathon and not a sprint. But you're not gonna fix ego/trust/respect issues at code review time, no matter how deferential you are.

> Edit: Also, when I nitpick your code for one reason or another (which generally only happens when I am enforcing a company style guide), I will often say "Nit:" and then use the sixth style. If you are new to the team, I will quote the guide to you. Most people don't feel so bad about the nitpicks when you admit that you are nitpicking, and that it is for a reason.

This. We do it on our team as well. Especially when it is smaller things like spelling, indentation, etc, I might have quite a few nits in one PR, so it makes it a little bit easier on the receiving end when you see a dozen comments but they're all marked as nit.

If there's a significant number of nitpicks, I'll generate an issue to implement/fix/improve linting tools.

+1 to the comment about using "nit:". I also don't think it's fair to block code changes for style/quality/whatever that is the same or better then the existing code base. If it's not making the existing codebase worse on average than don't block.

I prefix my nitpicks with the same, but with the 3rd style.

For non-critical stylistic or refactoring I prefix with "Consider".

> Consider extracting this to a separate function to make this more re-usable.

This gives the developer the option to disregard the comment. Essentially I am trusting that they will consider it. It shows that I trust their judgment instead of ordering them.

For stuff that is more important but not critical I prefix with "I'd recommend".

I'd recommend doing [current approach] [migration statement] [your suggestion]. It will [benefit].

> I'd recommend testing UI components in a similar way that a user would interact with it. It will make your tests more resilient and prevent other developers from breaking your code.

> I'd recommend moving this logic into it's own function, it will allow this code to be used by future developers and allow you to test it easier.

This both declares the recommendation and explains why it was recommended.

For stuff that is critical, I might just write it more directly:

> We need to extract this into it's own function so it doesn't lead to/cause X.

+1 to phrasing blocking/mandatory change requests as "We" rather than "You"

Make observations but don't try to be an arbiter: Say "This function is long." but don't say "This function is too long."

You can soften that language by using phrasing like: "This function has grown long."

Explaining how to resolve a problem if the solution is obvious can be perceived as rude. Avoid. Don't tell other programmers how they have to solve a problem either - it's their job to figure that out - make suggestions instead.

If you want to make a suggestion, explain how it addresses the problem and if possible put an emphasis on improving things for the sake of others: "Extracting this to a separate function would make it easier to grasp."

Brief, helpful, and you're not appearing like you want to hold anyone's hand or micromanage them.

A good way to package possibly negative feedback is to formulate it as an I-message, or an observation and go from there:

"I find this method to be overwhelmingly long and it's hard to keep track through all of it. However, it looks like this, this and this are rather isolated steps, they could be extracted to make the overall structure easier to see".

"To me, this, this and this look like duplicated code. Do you expect them to evolve differently, or could they be extracted into a common function / module?"

And if the code goes against architectural or style guidelines, it also softens the feedback to start with that reason and go to the suggesting from there. "This class was designed to be independent of that module to be testable. Please extract your code into the foobar-interface and inject it"

My usual approach is to point out the problem I'm trying to solve rather than suggest a specific solution. I also like to phrase as a question about practicality. Sometimes the answer to that question is "No", and I want to make sure that even someone very junior is comfortable giving me that answer.

- This code looks pretty similar to this other code; is there a way it can be deduplicated?

- I don't understand this code; I think it's because there are too many conceptual steps. Is there some way to structure it for better readability?

- Is there a way to unit test this specific behavior?

Sometimes this style doesn't work (e.g. correcting a comment that has become incorrect due to the change), and in those cases I usually just say "Please do this." I also try to point out things I like when they are present.

Perhaps, silence would be a good choice on this issue?

To my view, coding is as much art as science, and it is limitlessly fascinating to me how different people find different ways of expressing ideas and solutions.

Also, the code review process is fraught with opportunities for insult, misunderstanding and unfortunate power dynamics. It is inherently difficult regardless of the actual content being reviewed.

On the other hand, if there is a significant issue here "I am having trouble following your thinking here. Perhaps dividing this up into smaller functions would help?" Might be a good review comment?

The style wars are very tempting to engage in, but they virtually never drive greater productivity, real quality improvements, or positive team dynamics.

If there are real reasons that this wants to be broken out into a separate function (reusability for instance), then make that clear in your suggestion.

Neither of the first two nor anything else ending in a question mark unless you're really asking a question.

Not #3 because you're still being coy.

I'd suggest #4 or #5 when dealing with peers but they aren't interchangeable, they mean different things.

Save #6 for when you're giving tons of feedback to a junior.

If you're worried about being perceived as harsh or unfriendly, remember you can still be super friendly in the chatter on either side of the review session.

There are three main categories of code reviews:

1.) Things are mainly bad.

2.) Things are mainly good.

3.) Somewhere in between.

Within those, everything I see and flag as worthy of discussion is either:

1.) Something that violates our rules/style.

2.) Something I have a strong opinion on.

3.) Something I don’t understand.

Depending on which main category the review falls into, I’ll deal with these differently. If things are mostly bad and I don’t understand something, I’ll ask a lot of questions about readability and naming conventions. If something is mostly good and I don’t understand something, I’ll ask “Hey, can you explain lines 71-104?”

In general, I try to keep things friendly and positive because I don’t want people to dread the experience. Life is too short to dread a big part of your job.

> "Extract this to a separate function please"

This sounds really bad. Even if you’re their boss. You’re dealing with professionals and part of that is considering their input.

I’d start with the assumption that they know what they’re doing and might have had a good reason and phrase it from there. Even if not true it sets the right tone of respect.

I've learned that commands not phrased as such usually aggravate people the most - especially #1 from your list or questions where the one questioning knows the answer and wants to suggest something.

If I don't know how to phrase my comments I just schedule a call and we go over the code together. Much less time spent than on comment ping-pong.

If the code generally works, but other aspects, like maintainability, would benefit from some rework I use the phrase "You can...", e.g. "The second argument to the `map` callback is the element index. You can use it to create this range instead of using an external variable incremented on each step."

The recipient has the possibility, but is not actually forced to do anything, just like with code suggestions. In the vast majority of cases they follow my advice though.

If there's an error or a kludge I use:

-Code suggestions - no comment, so nothing to get upset over and if the recipient is feeling particularly irritable, they can just ignore it.

-Direct language in imperative form, e.g. "make sure that X, otherwise this won't work as intended", "avoid X in Y", "use Z here (documentation link)".

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.

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.

Actually reaching a level of team culture where you can be as brief, blunt and to the point as possible is a wonderful thing.

There is no offence to be taken, because the deeply held respect for each other as team mates is already there and taken as a given. You trust each other to know that any comments are addressed directly to the code, rather than to the person behind them. All input is valued because everyone knows the goal is just to make the code better and help each other out, rather than criticise an individual. Discussion can be frank, open, unbiased and non-confrontational.

However, because that takes a long time to develop and is somewhat of a rarity, I've had quite good luck with the phrasing:

"Have you considered X here?"

It has a number of advantages:

* It doesn't imply that an author should have done it a different way

* It doesn't imply that an author has missed something you think they should have covered

* It's a good starter for a conversation, rather than a conflict

* If the answer is "no", it provides a good prompt for an author to come up with a suggestion first

* If the answer is "yes", they can explain why they ended up with the solution they did, which is visible to everyone who wants to look at the PR. That can also be a good prompt to add a comment

Another method: prepend "nit: " to comments that improve the code but are not calling for changing the main logic. As a reviewer, you are simply acknowledging that this is a nitpick :)

All of this should be hammered out in a set of guidelines around code review so that ego/feelings don't need to come into play. Have style guides, code review checklists, and best practices documented.

The actual words used should be determined by the goal of code review and the relationship between reviewers.

Anything that can possibly be automated by CI should be. Style guide and test coverage should automatically fail PRs before a human needs to add any "nit"

That means code review should be a teaching or question moment (between a senior and junior, newer and veteran) or a discussion between peers.

From a junior to a senior -> why did you extract this? why didn't you do it this way?

From a senior to a junior -> typically, I would extract this because X. Check our best practices doc for a longer explanation.

Between peers or senior/junior (maybe one is newer) -> it isn't in our coding best practices document, but normally we extract this because X

Between peers -> what do you think about extracting this because X? Should this be in the coding best practices document?

The last is important because ideally you're updating your coding guide so this same review doesn't need to be done 10 times. And, if you don't think it should be documented as a best practice then don't bring it up in a review.

Across the different teams and orgs I have worked at, there's always been 1 constant during code reviews: Each comment provided an explanation of why some change was being debated. Every engineer knew the review is not a judgement of their coding prowess. Rather each discussion was purely on the merits of that line of code. Sure, there can be disagreements, but it was never judgemental. Usually folks are amenable when there is good justification to the feedback in my experience. It goes both ways too: I personally don't care if some of my nits are not fixed.

Btw, since I think it is relevant to this discussion, we're working on making code reviews deeper than text based diffs. Check out (DiffLens)[https://github.com/marketplace/difflens] if you're code base is primarily TS, JS and/or CSS to get language aware diffs on your GitHub pull requests. We've found that it makes understanding code changes much easier

I think the only correct answer is "it depends"

It depends on the relationship with the person who's code you are reviewing, it depends on the relative seniority differences, it depends on the project, it depends on the rest of the team culture, it depends on the rest of the company culture, it depends on how long you've had a working relationship with this person, it depends on who else might see the review.

I've had working relationships with people where it was beneficial/easier to be super curt and to the point, because there was sufficient 2-sided trust that best interests were at heart. I've had working relationships where I had to take having "open-ended genuine curiosity" to the extreme because of fear of feelings being hurt.

That being said, by default I take a question-asking tone to the code review process, with the intention of possibly learning something. It opens up the door to "being wrong" and allowing things to progress without incessant arguing. There are times when this approach isn't appropriate, but I think it's a reasonable default. Again, it depends.

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

Remember to be problem oriented rather than solution oriented. If you just tell people "I think this should be the solution" or "you should change it to this solution", you aren't working with them, you're instructing them. And a key to teamwork, especially reviewing, is a cooperative spirit, not an instructional one.

If instead of saying what you would do, or what you think they should do, you express a concern about the code, the tone is completely different. Now it's just saying something like "do you think repeating this across files will be problematic?", which allows the other person to be the determiner. If they agree they may well do exactly what you would have suggested, and if they don't, it can start a discussion that helps bridge together how various team members code. It's win/win.

It also helps people understand what you aim for when writing code, which helps them keep in mind your needs in the future (and vice-versa for you to keep in mind their needs).

> "do you think repeating this across files will be problematic?"

If you write that you clearly think it's problematic but hide it behind an unnatural question. Then either I do what you hint at or you will push / discuss until I agree. I am not the determiner. It sounds like you are schooling me with an awkward back and forth.

Honestly, to me this is beating around the bush and doesn't actually make me confident we can have a real discussion (in the context of that code review).

"Do you perhaps think that doing this <tylically discouraged coding practice> is problematic?" sounds like either you want to passively insult me, or you don't consider me a peer with whom to have an actual discussion on equal footing, or you don't have the guts to share your opinion without contortions.

The wording above could be improved for sure. But the intention is to shift focus away from telling somehow how to solve a problem to forming consensus on whether there's a problem at all. And that starts with the reviewer expressing a concern rather than immediately providing a solution. "Is a change needed?" should come before "this is a needed change".

A better example might be "I'm seeing this same code in various places--do we benefit from keeping them distinct?" or "you took an inheritance approach here, but I'm concerned about the long-term impacts as the codebase changes. How do you think this code compares with a composition approach?".

If in doing so you would think that I'm being sly, that's unfortunate, but there's not much I can do in how you interpret stuff. I can't control for others assuming bad faith. And if there's bad faith, there are probably communication and/or trust problems amongst the team members that are broader than code reviews, and is something that should be tackled directly.

When you are reviewing, what is your authority level? Do you enforce a checklist? Are you merely asked to give your professional opinion? Are you a senior supervising a junior? These are critical inputs for me to calculate tone.

When I've done reviews, I had some seniority and an organizational privilege to veto some code. I worked from a checklist and a goal (with which the checklist was meant to align, but we knew it was not possible to fully automate those aspects of review). These are my takeaways from that arrangement:

Language like "declined" or "REJECTED" or "can't approve" is discouraging to the individual contributor. I replaced all that with discussion of why I can't allow that, under the obvious subtext of rejection. No need to just rub it in when there is learning to offer.

When indicating required changes, especially where I was more-or-less handing them the replacement code, I always said please. Always.

Most importantly, I gave accurate feedback. I took the time to be sure I was right before I wrote a review. Otherwise what's the point. Even simple patches got tested.

Here's a trick: always approve the code review, unless it's an actual bug, a security risk, or obvious quality issue. Then you can use whatever tone you want without much damage.

"approved, but I think this should be in a separate function"

Couple things I try to keep in mind when giving PR feedback specifically.

0) Give feedback with respect and just like as if I were sitting with someone face to face telling them these things. And keep it about the code, not the author.

1) Remember that I could always be wrong (whether from misreading something, not having the whole pictures, etc. - many reasons my suggestion may not be the right one).

2) Communicate how strongly I feel about a change - I use `nit:` to indicate opinions or annoyances that are basically "I wouldn't do it this way, but up to you whether to change it", slightly stronger language for things I think require changes, up to "I feel strongly about this, let's chat about it" for things I would not approve the PR before they (or my mind) are changed.

3) Phrase as requests and/or suggestions (could, would, prefer) over command (do X)

4) As others have mentioned, give the reasoning. Personally, I prefer request first, reasoning second, but I see many people expressing preference for it the other way. My reasoning is that, while the reviewee may read the reasoning once, the request is what they may come back for when fixing things, so ensure it is the first thing seen so someone doesn't have to parse through the reasoning again to find the specific request.

5) Provide examples if it's clearer to communicate that way as compared to writing it out.

My suggestion would be typical: concise, professional, and when straight-forward, specific. The key is to remember that the nature of a code review is first to improve product, and perhaps secondarily to improve process (ops or procedure) and perhaps thirdly, people improvement in the form of collective learning.

Thus, if we're talking about refactoring a line-of-code then just writing the refactored line-of-code is probably the most clear and concise. If it's more involved than that, then it may be worth taking offline. In the example you've provided here, I'd suggest the best language is likely: Refactoring this block as a separate function provides the following benefits: 1. blah, 2. blah blah, and 3. blah blah blah and no side effects. The only additional clarity that's helpful is to say whether it's optional, required, or some other directive.

1) State the specific reason that motivates you to ask for a change.

‶This code is duplicated several times.″

2) State the change you'd like.

‶I'd suggest extracting it to a separate function.″

3) (Optional) Expand on why you suggest solving the issue with this solution instead of another one.

‶We could create a macro instead but it's not worth it as we can just let the compiler decide whether to inline the function or not.″

Complementary to other answers, I would suggest introducing this technique: https://dev.to/allthecode/moscow-the-best-code-review-techni... However, share this w/ the reviewee first!

I know some devs that are anxious if they don't fix _every_ single comment in their PR, even if I'm just suggesting something as "nice to have". This helped to mark which comments are actual priority.

Also, make sure you're using code formatter in your CI, to avoid reviewing the code style, as this can start some unnecessary wars and frustration.

Also, I suggest sharing this article https://mtlynch.io/code-review-love/ with your colleagues, especially step 8:

> As the author, you ultimately control your reaction to feedback. Treat your reviewer’s notes as an objective discussion about the code, not your personal worth as a human.

To some degree, as a reviewer you can keep your tone friendly, but ultimately _every_ sentence can be took as personal attack, and you can't really do anything about that. Keep that in mind, don't focus _too much_ on the tone, and don't let that prevent you from giving a comprehensive review.

One other thing I personally do, because I'm famous for giving _really_ thorough reviews, is to make sure from time to time that the other person is fine w/ that. Simple DM "I hope you don't feel too overwhelmed by my CR?" helps, especially that often people respond that they actually approve I'm so thorough. If you worry people get offended, it is easy way to make sure if these worries are valid or not.

Code review is a form of communication, and thus depends very much on culture.

This can both be company culture and origin culture of the developer you're communicating with.

For example, sometimes I write comments like "I don't particularly like this because" plus an explanation, plus "but I don't see any significant better way, so I'm fine with it for now". Some of my colleagues are fine with that, maybe they add a code comment like "TODO: find a way to fix problem X", or they just acknowledge and don't change anything. And then there are developers I work with that come from a different country, and they'll spend another half day or even two days coming up with a better solution, or don't dare to click on "resolve thread".

I think their culture is really geared towards avoiding verbal criticism, and if I even bother to write something negative (and maybe I'm also senior to them, in the org chart), it must be really meaningful and must be addressed.

So for some of these "foreign" developers I only leave comments where I expect something to change; for others that whose culture I better understand, I sometimes leave comments that are more conversational.

Is this work you're collaborating on together (as peers) or work you're supervising? Are you in any sort of relationship where you could be considered to be 'mentoring' the developer?

When collaborating, I like to provide my feedback in a way that allows the person to provide their own perspective.

"What do you think about moving this to a separate function so that we can keep each functions in the class small and focused on one specific purpose?"

When supervising you of course want to allow two-way-communication but you can be more direct, but make sure to use the correction as a teaching moment.

"Please move this block to a separate function so that we keep the functions in the class small and focused on their one specific purpose. See abc.js and xyz.js for good examples of classes that follow this pattern."

The emphasis should be on clarity. Overall you want to leave the requester in no doubt about what it will take to get approval because the most frustrating thing for both of you will be ping pong.

Tone is very hard to judge in written form particularly when there might be cultural and language divides, in my experience though it doesn't really matter what style you use provided that you also remember to comment on things that are good and do not need changing. This is routinely forgotten and more than balances any perception of negative tone the requester might inadvertently infer.

Never use should. It's confusing and inherently negative. It's also not very clear. Compare should with could in your examples. Could is very clear, a direct question to the read.

When it comes to feedback, ensure that you give the engineer recieving the feedback the maximum potential to grow to ensure the engineer does a better job next time.

Its not the tone but rather the feedback style according that should be adapted to the knowledge/motivation of the engineer in the situation. For example by using the https://situational.com/blog/the-four-leadership-styles-of-s... herustic:

1: Engineer is junior in the context and insecure/unmotivated: Do X 2: Engineer is junior in the context and motivated/secure: Take the decision for the engineer and explain why 3: Engineer is senior in the context but insecure: Coach by open ended questions: how large of a function do you think is appropriate? 4: Engineer is senior and motivated/secure: From your perspective how should the function be and how should we do this in the future to reach our goals.

I tend to be overly “nice” in code reviews and phrase things more like suggestions and provide some justification for them:

> have you considered x? It is standard practice in y to do z.

> should we do x instead? I think it would be better in the long run because y.

If there is a more serious issue, I prefer DMing the author and hashing things out synchronously.

I think it is important to phrase things gently as much as possible, as tone is hard to read from text, and being overly harsh might discourage other teammates from taking risks and asking for feedback.

“Won’t it be better if we did that instead of this?” “Do you think it would be better if we did that instead of this” “I think it would be even better if we do this instead of that”

I usually use “we” instead of “you” in my code reviews, “we” feels more friendly and comforting and way less judgy.

I agree. I tend to use "we" when for the negative stuff of anything that has to be decided by the team ("we changed the code here but it caused a regression", "should we refactor this?"), however I also like to use "you" for positive stuff and to give credit ("your change in b8e63dca also fixed this bug, livinglist").

The clear declarative: "Extract this to a separate function." Everything thing else can be misinterpreted as a suggestion, dig, ect.

Actually an interview question to work with us. We need code, not egos. Write and be wrong, correct and continue. Do not sweat errors. None of us are perfect, and neither is this review.

In addition to other comments saying to add reasoning, I’m a big fan of the ‘MoSCoW’ method (must/should/could/would) to remove ambiguity around if things are a question vs a command vs a thought. ‘You must do x’ sets a clear condition before you approve a PR. ‘I would have done y’ shares perspective and experience without blocking when you think you might be just bike shedding.

I’ll usually link something like this page to the repo and align on the meanings of each phrase.

https://en.m.wikipedia.org/wiki/MoSCoW_method

The tone of a code review suggestion reflects an underlying assumption about what a code review is (or should be). Often developers disagree on this point. It seems like we fall into one of two camps.

1. Reviewing code is like proofreading a paper. Suggestions are welcome, but there isn't an expectation that all suggestions be followed. The author of the code makes the final decision based on their objectives.

2. A code review is a gate that is closed until all comments have been fully addressed. The author must comply to the liking of all who choose to comment, or they are not authorized to continue.

The tone of review comments will naturally follow from their understanding. Understanding #1 yields comments aimed at persuasion. (Should we, Could we, You might consider) While #2 yields more compulsory language (Change ..., You should). Or passive aggressive language that is meant to be compulsory but softened to sound less aggressive (I would, Could you, shouldn't you).

It is my opinion that understanding #1 is most appropriate. It prevents unnecessary bottlenecks and is more respectful or differing opinions. If developers are peers, than #1 is the understanding the most accurately reflects that.

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!

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.

Code reviews are tough when people have strong opinions. I try to ask questions rather then making statements, it gives a room for discussion and makes the whole process less challenging

> * Should we extract this to a separate function?

Reads very passive aggressive. No.

> * Could you extract this to a separate function?

Yes, I could.

> * I would extract this to a separate function.

Good for you.

> * This could be extracted to a separate function.

This one's OK as a style suggestion that you won't block the merge over.

> * This should be extracted to a separate function.

This is kinda, sorta OK, if you're going to block the merge if this isn't fixed.

> * Extract this to a separate function.

This is better because the tone matches the intent: it's a command. This gets addressed, or no merge. "This should..." is indirect—you're not stating what you actually mean. This phrasing is better.

4 is the best of these for optional suggestions, 6 is best if you consider the fix a requirement for a merge. In general, say what you actually mean—dancing around it leaves room for miscommunication and can give all kinds of bad impressions. Of course, that can also mean weakening language: don't command a change that you don't consider a big enough deal to block the merge.

[EDIT] And, as others have noted in the thread, of course expanding with justifications is always a good idea. "Do this" without a "why" should be avoided. But lead with what you want done, not the why—that follows.

I agree with everything you said. But I’m Dutch.

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.

First decide what do you want:

* Is the change mandatory for your approval? Then be polite and firm with reasoning - "This is poorly readable and doesn't conform to our code style chapter 5. Please extract this into a separate function.". Don't mess around with "Should, Could, I would" starters when it's not a question - be clear what you're asking.

* Are you trying to start a debate? Then ask a question properly - "I think this is unreadable because of A, B, C. Do you think extracting this will help readability?" Again, be clear it's a question.

* Are you saying an opinion and you're not very hung up on it? Mark it as such - I tend to put "Optional" or "Nit:" (team term) in those cases. "Nitpick/Optional: I think this isn't readable, if you have a moment maybe extract the method?"

The worst you can do is veil your intent (order/question/opinion) into faux questions which will confuse people - especially if they're non-native english speakers. Adding background / explanation / context for your comment always helps too.

The different ways you're expressing the question give different meanings to your review. It's not clear to me what you want: are you blocking the review on extracting the code? Does this have to be done now or could this be on a separate patch? After or before the series? Is there an actual good reason to extract it (like you plan on using it tomorrow) or is this just about making the code easier to read?

- Try to make this not personal (opposite of how I started the paragraph above!). Talk about the code, not about what the author did. The code does this, the code does that, instead of you wrote this, you wrote that. Also, it's our code, so "we should do this" instead of "you should do that" in case you can't use "the code should do that".

- Be clear on what's just nice-to-have and what is really blocking the review process.

- Explain to them why you think this should be extracted to a separate function. Maybe they have a good reason against it or a good counter argument.

I have been looking for the tone too. Tone is important for techies, too. I just think it's not the biggest issue.

I think the real problem is the we lack a framework that makes explicit:

* What is the goal of a review, according to the team? Spotting bugs? Prevent security flaws? Guarding the architecture? Making suggestions to grow better as programmers? These goals partially overlap, but are different. Are you aligned, as a team?

* Encode your priority in each comment. We could use some semantic prefix for a comment that categorizes it as, in increasing order: 1.suggestion to share views and alternative approaches, do as you please / 2. I think another approach would be better, suggest you use it / 3. I think it's very important to change something, let's discuss / 4. I'm not willing to approve or compromise, but feel free to get approval from someone else / 5. If anyone allows this to be merge, I will escalate.

Then still we should use good tone, but it's more explicit what to expect, now. Open communication is key to a psychologically safe environment. But I'm Dutch :-)

    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.

There's definitely a lot to good code reviews. One addition perspective I've started taking that I haven't seen so far in the comments is a "Have you considered this alternative approach type question". And to describe the alternative, show it, etc. And this ensures there is an option to basically reject the suggestion, a valid response is I've considered and rejected that idea or approach.

Your examples lack a reasoning or explanation as to why code should be abstracted to a separate function.

To a senior dev they might not need an explanation to justify the action of extracting the coded. It could just be oversight.

But to junior and intermediate you might need to explain the reasoning for requesting code to be abstracted.

I second guess myself and edit a lot, especially relative to how much we actually do code review, so I'm interested to see others' thoughts.

I would say though that a lot depends on team dynamic & hierarchy. For example, I have no direct reports, so I'm never going to use the imperative as in 'extract this to a separate function' - taken literally, I don't have the authority to tell anyone to do that.

Most often I think I favour 'I would', and give a reason. I.e. I found this a bit surprising, because I would have done it differently.

Along the same lines I also ask (honest) 'is there a particular reason' or 'why not' type questions. Something else seems most obvious to me, but maybe they thought of (even tried) and dismissed that.

You've done a good job here, but I might suggest stepping back even a bit further. If you want reviews to work, they must be in the spirit of "we all improve and learn together" rather than "bad dog! bad!" The tone you set in why you are doing them, who is involved, how they run makes all the difference in the world. What you want is to create a culture where it is safe to point out something that could have ramifications, and to promote a culture where there can be dialog about what's best for the health of the team and products you support. More often than not though, I've seen these meetings turn into "beatings shall continue until morale improves" sessions.

Just my $0.02.

>> There are lots of ways to provide feedback. I suggest stating the problem with the code and providing a solution. If that's the only possible solution to get past your review, state and don't ask. You can also give a carrot with "do this and I'll approve the merge."

Yes. This. Clarity is a prerequisite for tact.

Also, be consistent in your tone and style across reviews in a given project or org -- no matter who you're talking to. For instance, on one development project I was informed that all new tickets should use the language "shall" instead of "should" or "will." Consistent language gives everyone a common reference point and helps keep people from feeling "singled out."

For code reviews we try to automate as much of the nit-picky things as possible. So for example in our Python code we run checks using: black, isort, flake8, mypy, and pylint. For reviewing. I try not to use the word "you" as I feel it can make people feel defensive. So for your options above I would like:

Should this be extracted into a separate function?

As a code reviewer I may think it is obvious that it should be in a separate function or something else that seems obvious. But often times the author has already tried that and had issues with doing that. Thus they did it the way it is because of that.

Some of my coworkers have a convention of prefixing messages with [request], [suggestion], [question], etc. in order to make the intent extremely explicit. This is particularly useful if you have people coming from multiple cultures with different levels of forthrightness.

While some people don't like hearing it: It is also partly up to the author of the code to ensure that reviews are handled professionally.

If you're fluent in English, there's a clear difference in the tone of the listed comment forms. If you're working with colleagues that may struggle a little with English, you can easily read to much into the tone of a code review. The reviewer may not mean to come of direct or even aggressive, but because they literally just translated directly from German or Finnish the tone will be way off.

It's also up to the author of the code to assume good faith, and look beyond the language used. Try to be kind when reviewing the code of others, and assume that the reviewers is trying to help.

Personally, I prefer "What do you think about extracting this into a separate function?"

Unlike in any of your examples, the subtext is: "I anticipate you had reasons for doing it the way you did and I am open to listening to them." It engages the author on the same footing, leaves the door open for discussion but still communicates your intent in a straightforward manner.

I mostly only ever use the form: "This should be ... " with a possible explanation why. We're all mature enough to not take it personally, so I'm not going to beat around the bush by writing a lot of proza. The imperative of the last example ("Do this") leaves no room for argument (because I might be wrong).

Either that or I ask for clarification of a piece of code: "Why did you do this instead of this?".

Like already stated by someone else; your approach really should depend on the team you're in. I'd take a less direct approach with junior devs who tend to be a bit more insecure (and stubborn).

None of the suggestions contain any justification for taking the action. In many cases a reason may be obvious, but as it is not the case that every opportunity for creating a separate function should be taken (for one thing, doing so can make reading and understanding the code more difficult), this becomes an issue of judgement.

Lots of good suggestions.

I don't think there are simple answers, and it's something you'll have to figure out separately for every reviewee, and sometimes every review.

You have to consider power dynamics, preexisting relationships, your own time, the importance of the change, and cultural differences.

If the power dynamics are too far apart, the right answer is often "don't". A high-level engineer or architect reviewing a relatively junior coder's patch should not be commenting on anything involving taste or stylistic suggestions. They should be reviewing for functionality, security, compatibility, etc. And only bring up maintainability or performance when it really makes a difference.

Some suboptimal code will get landed as a result, but it's better than creating a culture where the senior's personal preferences are always going to win out. When there's a power imbalance, nuance gets lost—the junior either has to bend to the senior's preferences even when they have a good reason to do things differently, or they have to go to a lot of effort to justify their position. Which is fine in some cases, but it's a waste of time and effort for things that don't matter as much. (This isn't about avoiding hurting people's feelings, by the way, though it does serve that purpose as well.)

I also agree that pointing out positive qualities of a patch can make up for quite a few critical comments. Explicitly constructive criticism (eg sketching out an alternative, or doing some work to provide data or justification for a comment) can also "buy" a smaller amount of more negative-sounding comments.

And of course, if you're swapping patches with someone regularly, there's no need to overthink it: state the problems you see, the things you're not sure about that concern you, the suggestions you have, etc., using as clear and concise language as possible.

In all cases, do not make the reader guess your actual opinion or intention. If you're couching a criticism in gentler language to avoid making them feel bad, that's fine, but don't play games or leave out important pieces. There's always a way to coach or criticize honestly, and you'll do yourself a lot of good by finding it.

Personally I have two tones I take in a code review comment: the "Should ... ?" form for when I want to raise awareness but otherwise leave it up to the other person, and the "I recommend ..." form for when I think there's a real issue. Especially in the latter case, I'll back it up with a real-world scenario of how it could cause a problem; either a bug, difficulty reading the code by future maintainers, or making it more likely to introduce a bug when the code gets refactored later. Finally, back-linking to documented standards if necessary to avoid the appearance of a me-against-you type of thing.

It depends on who the corworker is, how well I know them and how long the review is. My comments range from very polite to "???" "delete this" or "don't copy paste this".

Once I have worked with someone for a while (and perhaps have gone out for beers / coffee a few times), I write shorter comments.

At that point, they've usually figured out that I mean well, but am blunt.

For new hires, I spend a lot more time spent on "why" and tone.

If my team set guidelines for this, that'd be a strong signal of much, much deeper dysfunction and I'd consider switching jobs.

All of these are equivalent if you have showed your team that you trust them and value their work.

If not, then only the first version is acceptable.

“Please extract this to a separate function”

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

I like "Consider extracting this to a separate function."

Trying to never use the word "you", it seems a bit aggressive. Using "we" or "one" instead, or rephrasing to get rid of pronouns completely.

"if we extract this to another function we can then add a test like this" with an example. Gotta say what the value add is. The value add could just be that it's more like the rest of the codebase! "In the past we usually extract functions like this link optional second link"

I think none of those, one that is focused on teaching and explaining your reasoning would be better. You can always do it humbly because the author may have some info you don’t that they came across when implementing, which is why it is the way it is.

I feel like this is a problem ML could solve for assisted feedback focused on positive tone, ml models scans your response and offers suggestions on phrasing.

My favorite guide on this is https://phauer.com/2018/code-review-guidelines/

There are also some great things in the book, Debugging Teams, like the Humility-Respect-Trust (HRT) concept.

I explicitly prefix Error: when I am requiring a change, Warn: if I am suggesting a change, Info: if I have an unrelated comment

Error: `< n` is an off by one error, the last element won't be processed

Warn: Consider dropping the loop altogether. We know there are always eight elements.

Info: Nice name!

A bit related - do you ever go from PR comments to DMs? Sometimes when discussions are needed, it feels much easier to just chat 1 on 1. Especially if it's a small team and everyone's close. Though that prevents others from experiencing the "interaction".

If its not just asking for clarification on a comment then I really try to stick to the PR, so that the context is captured. If a back and forth is needed then I make sure to include a recap somewhere in the PR.

This is a great question because I find that when I use soft language like this, a lot of the suggestions get ignored. Sometimes that is fine but other times, I have to follow up after and explain that the 'suggestion' wasn't actually optional. I find it hard sometimes to protect egos and quality at the same time.

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.

only suggest improvements, and provide reasons if possible, never tell anyone they should do something in a certain way unless it’s an obvious bug. I fucking hate know it alls that confuse their opinions and preferences with some absolute truth.

Typically I say something like:

“What would be the advantage of using X instead?” If I’m not entirely sure or fine either way.

If I think X is really the way to go I’ll say something like:

“Doing X would have these benefits. The benefit of Y is such and such. So X is preferred for …”

I always try to make suggestions as questions because it's less threatening, gives more agency to the author, and oftentimes ends up being a discussion on the merits. If the suggestion really is a blocker, I'll be a little more direct, but always always friendly.

I typically go with something like "Have you considered extracting this to a separate function"?

I'm sure this will piss people off: but almost all of the 'code reviews' I've been a part of were a waste of time. People trying to force stylistic changes to how a program works (OPs post is an example) aren't contributing anything constructive. What is worthwhile is trying to find critical security, logical, or performance bugs and reporting those. If you have taste / style concerns about how someone does something yet their solution is still fine then I'd suggest not wasting peoples time. There is nothing worse than nit picking.

Depends on the situation. If it is a clear cut case or you're a mentor/senior to the reviewee, a more direct tone is fine. If it's a matter of opinion or it's a peer review then a more considerate tone would come across better.

My "formula":

1. Try to find something positive to say about the code. This reinforces what is being done well and supports the notion that everyone is on the same team chasing the same goal.

2. I like to go deep into the "why", being as objective and fact-based as I can. Code style and "best practices" are sometimes confused with personal opinions and preferences. I avoid words like "I prefer". If I can't back up my opinion with strong reason-based arguments for why my opinion is objectively better then I keep it to myself and continue to ask myself if it's a personal preference or if there's a good reason for favouring that.

3. Tone must always be friendly, polite and constructive. Of your options I often go "I would extract this to a separate function because ..." and then I go into the "why"

4. Sometimes I have a lot of supporting information to explain a position. I will often label these "Academic Digressions" and then drop a TL;DR. Sometimes people don't have time to read them right away but I'm often told that people appreciate them and save them for later.

Pointing out the good in a PR is such an important thing to do, particularly with juniors!

"I think we should…"

"Maybe it would be better…"

"IMO we should do …, what do you think?"

You really want to be on this person side and help them achieve. That's your job after all

My personal favourite is: "Have you considered X"

I also like to often prefix my suggestion with: "It might have missed something, but X", especially if it's a larger change.

I hate it when people say "have you considered X?" and mean "do X or I will block your change." It's all about communicating what you mean.

I absolutely get that. I was a bit in a hurry when I wrote the above comment, and therefore didn't get around to better detail how I use those sentences.

It's all about understanding the context and not talking down to the author.

When I see obvious improvements I use the "order" style and just state what needs to change. Because surely the author can see that the change will be for the better.

The other sentences are for bigger more complex changes where I'm curious about a certain decision. Most often I don't agree on the current design, but I would never write such a comment unless I'm ready to be convinced otherwise.

When someone has spend hours on some functionality and I spend just 5-10 minutes skimming through the code, there is a very real chance that the change I'm going to propose was already considered. It would be arrogant and condescending to just conclude that my idea is better, so I open the door for the authors explanation.

I agree with this so much.

IMO if it's not mandatory I don't think it should be said at all in a code review.

I would extract this to a separate function.

Don't forget a big part of this is the dependent on the image of you the recipient has in their head.

Consider extracting this to a separate function.

After reviewing my last few code review comments, this is my most frequent phrasing too. I get that questions are a way to soften the bluntness of a code review comment, but I've been hugged to death by this sort of niceness in the past. It was not fun.

"Consider ______" has the directness I, myself, enjoy, and whose response can take the form of a change, a reply, or a jumping off point for discussion. It indicates some action needs to take place without specifying exactly which one and leaves me open to having my mind changed if the response is "I've considered it and actually ..."

Still, if there is ever a flaw so great that the code simply will not work without a fix, I will phrase the fix in the form of a command.

Harsh but stick a joke in there so it looks like you aren't a dick.

Several other people made good points about the overall review structure that might well be more important. But to answer your actual question about tone:

There are two axes I think about here when deciding on tone. Why am I reviewing this code? And why did this person write the code?

For the me axis, at one extreme I'm paid to review this code, and I designed or co-designed the software that's being modified in the reviewed change. Architectural decisions were mine. At the other extreme, I'm just another volunteer, this system I'm presumably interested and somewhat knowledgeable about (otherwise why review code for it?) but I may not even be an expert, and certainly can't be said to "own" it in any way.

For the other person axis, at one extreme they are assigned to work on my project, this is their job, at the other extreme it's some volunteer who has worked with this ten years longer than me and maybe designed it and it's good of them to ask me to review it, they could just push commit.

#6 "Extract this to a separate function" is a command, so it's not appropriate unless either they're an employee and I'm supposed to be mentoring them, or they sent unsolicited contributions to some project where I'm an expert and frankly I'd rather not have their contribution than take it as it stands. If it's a project that invites contributions (even very informally) then this is always the wrong tone.

#1 "Should we extract this to a separate function?" is a question, and so it's not appropriate if I am expected to provide guidance (e.g. in mentoring or when reviewing code from a brand new team member) but absolutely appropriate if I'm reviewing code by somebody who knows the project much better.

#3 "I would extract this as a separate function" is always appropriate if you are sure you would do that work. However, if you are sure you would do that work, consider just doing it instead. In the scenario where it's a junior employee being mentored maybe there's pedagogic value in them extracting it so write either #5 or #6 depending on whether they seem to need the explicit instruction or whether it's implied in the usual workings of your environment. In the scenario where you're a new volunteer maybe the author has a good reason it should not be extracted and so it's worth asking. But in the middle case why aren't you doing it? If it's just not worth your effort the same is true for the author.

#4 "This could be extracted to a separate function" is redundant and so is #2 "Could you extract this to a separate function" the answers are "duh" and "hopefully you can do so or else you should learn this language". So overall I think I see three tones I would use (#1 #5 and #6), but in quite different circumstances, two tones I think are too vague/ redundant (#2 and #4), and one tone which is probably never useful (#3) because it suggests I should do work instead, in which case why not just do the work and say you did it "also I extracted this as a separate function, [link]..."?

* This shall be extracted to a separate function.

Tone: if you write, cultural differences will always remove any tone; the subtleties will be lost in translation. If you talk to the people, the tone of the voice and the body language is more important than the words.

I’d say the last two are best, but I’m Dutch.

last one... save people time. i don't need pleasantries.

You may not need them but what about others?

A - for juniors

1 - This / should / could etc etc make sense only if the code is duplicated. If it's long, better to make a sub-function and use those in main body of the function for better readability / maintenance.

2 - As for tones, you are the senior. Don't sugarcoat juniors, tell them the truth green in their faces. You'll be appreciated more in the future by them because you're helping them grow, not running for mayor office and you need to be PC. Also, don't be rude / asshole, they still need your help though.

3 - I found out, the best is to talk to them like you talk to your friends.

B - for seniors

No sub-points here, seniors who make blatant mistakes gets treated like juniors (see section A). Those who make honest mistakes, well I found out they only need you for rubberducking because mid-review they'll realize and correct / change course on the spot.

isnt the first one good for everything, why even bother with the other options?

Personally, I think that the focus on on the phrasing itself, which as you mentioned are all more or less synonymous anyway, is letting the underlying challenge get away from you. You can safely use any of them, but that there's other stuff that can be done that's more important.

Any time you can convincingly explain exactly why you're suggesting something it makes the suggestion itself seem much more reasonable, and it's a valuable check on your own reasoning. From there, you can reword the explanation itself in creative and valuable ways to give feedback. If you get really good at this, then a lot of times you don't even need to ask for a specific change, you can just turn the problem you're seeing (I.e. 'explanation' from above) into a question. I'll give an example.

Let's say you want to say "Extract this to a separate function". Ok, now, ask yourself why? So you tell yourself, "Because we may want to use part of this function again, but not the rest of it, and the exact same behavior can be achieved with little to no additional effort by function composition (or whatever you do in your PL)" Ok, that seems reasonable, but maybe now it may be perceived as a little too abstract to someone else, especially if they're just trying to get their work done. But you can take solve that by taking the above 'explanation' and make it concrete. One good way to do this is to think of a counter example, where it clearly doesn't work how we want.

As an example, let's just pretend that they're doing something like parsing a thing from a database into some strongly typed thing, and then doing foo work on the strongly typed thing. You can ask yourself then, well if we were unit testing this, we won't care about the parsing part, we can just start with the strongly typed thing as the precondition of our test, and then isolate what we're testing to just be the foo functionality. Now you have a simple example that you can rephrase into your original feedback.

This thought exercise all leads to the feedback going from "extract this to a separate function" to "Does this mean that unit testing the foo feature requires a parsing step for each test case?"

Maybe that's a silly, contrived example, but I've found that if you even capable of going through this thought process then not only do people receive your feedback much better, but also you just tend to think of much better feedback altogether. After a lot of practice you end up being able to think this way very quickly and then you can focus on adjusting the wording based on your relationship with that person.

At the end of the day, it's not what we say that really matters, it's how someone feels they're being regarded by one their peers. If they sense contempt, then it won't matter how politely or expertly you word things. Having extremely well thought out why's in your back pocket, and being able to provide concrete counter examples just makes it clear you're not wielding your review power just to talk but to illuminate the problem to them. It also solves the problem of when you're the idiot (as happens to all of us from time to time), by forcing you to actually justify your feedback before you share it.

When I've done this, I've noticed that my reviews become faster, and I focus on stuff that people don't think about. It makes the code better and then people will actually seek out your review.

Easy:

Start with "Consider..." in case it‘s not a must.

If it‘s a must, start with explaining the consequences, e.g. "this value must be kept in a request scoped context or there will be race conditions and other concurrent misbehavior. Consider putting it in a @RequestScope bean."

As someone on the junior end, the tone is less relevant here than the content, and there is not a lot of content. Why do you want it extracted?

Will we re-use it? Do you just not like blocks of code bigger than X (been given this reason many times)? Do you just feel the need to comment (a lot of devs do and I appreciate it when they are upfront about that rationale and I have done this myself to show that I am paying attention)? Are you concerned about readability?

I need to know for future code reviews, so I can avoid the issue in the future.

The key is to understand the value and importance of what you are suggesting. “Remove this SQL injection.” Vs “consider moving this functionality to the superclass”. One is a must do and not doing it is unacceptable. The other is something that may improve the code, but isn’t hurting anything if you don’t.

I find that I rarely use direct commands in code reviews, even ones expressed indirectly (e.g. "can yo do X" instead of "do X").

If there are blockers in a PR, I'll add reasoning for why it is a problem (e.g. "this exposes us to vulnerability X", "in this edge case, what would happen is that ...", "I tested it and appears that there is a bug", etc.) If it's a team-agreed style issue (that isn't caught by the linter), I'd rather say "we use convention so and so in this code base" than "please change this to this style".

The reasoning is that I feel that I work with professionals and while they may need feedback, they don't really need commands. I can make suggestions as to how they can fix the issue I raised, but ultimately it's up to them to fix it and if they find an even better way that I didn't think of, why should I limit their problem solving capabilities artificially?

I find that most people I've worked with use a similar style (more centered around the code than around power dynamics), and those people who used a lot of "please do X" style comments were usually also pretty defensive about their code or ideas, would get uneasy when you changed some of their code ("why did you refactor this? please change it back") etc.

Now there may be some people (inexperienced, or just idiots) who really need some more forceful language, but you shouldn't necessarily start out on that default assumption.

That covers the blocker-style comments. Everything else should probably be done in either a "I'm opening up a discussion here and would be interested in your opinion" or a "this is just a nitpick, feel free to ignore" kind of style.

For the specific example, depending on the reasoning for why you might want the change:

- "The linter is complaining that this function is too long and we typically stick to those rules. I think lines 20-30 could be extracted into a separate function, for example."

- "I found it a bit hard to understand this function initially because it does a number of things at once (such as...). Maybe we could split it up?"

- "This section of code is repeated between here and that other function and I think we should keep the implementations in sync, so I would recommend extracting that common logic."

- "We've actually had to do something similar in other situations (see here and here) and your solution seems better than what we came up with before, so what do you think about extracting this functionality into a function and calling it from all those places?"

and so on (and it doesn't need to be as verbose if you know that you have more shared context, for example).

The tone also deeply depends on the culture of the receiver.

Depending on whether their cultural background is North American, Scandinavian, German, French, Italian or Chinese (to pick from the very situations I have faced), and especially if they are more junior, you cannot expect the same outcome from the same feedback, without additional context/care in the communication.

I adopt, and appreciate when others adopt a neutral/brutal tone.

"These lines have been repeated a number of places and probably ought to be pulled into a function called something like xyz in module zyq"

Generally your tone doesn't matter as long as it's not mean. "This is the 10th time I've told you declare constants FOR EVERY MAGIC NUMBER," does not belong in a code review; it belongs in feedback.

However there is something a bit dangling here which is linting and static analysis. If code has been repeated and genuinely ought to be pulled into a function, static analysis ought to be able to find this.

If you have already dealt with the broad class of possible change requests that can be caught by static analysis, the nature of the reviews should get a lot more straight forward. It will stay at a higher level and naturally tend toward verbiage like "I wonder if we ..." or, "I was thinking if ..."

In cases where static analysis would not find it for e.g. in the middle of a function doing something, we have several lines of code collecting data for some analytics event which is never repeated, but still confusing; I would say something like "For readability, could we pull the analytics bit into a separate function?"

Generally; if you can use "we" instead of "you" or "I"; I would prefer "we" in any team setting.

To add to the other points, remember you can also leave positive feedback.

Don't force a compliment for no reason because it will come across as insincere... but if you see something well done/implemented, don't be afraid to call it out.

It helps balance the tone sometimes, and is especially helpful with junior engineers who are well served not just learning what's wrong, but what's right.

I like, "I would extract this to a separate function."

Unless you're asking a question, posing it as a question comes off as passive-aggressive, even if it's not what you intended. A simple declarative statement of, "I would do it this way", is probably best. You don't know why he did it the other way, and he might have had a very good reason (for example, to be sure the subroutine would be inlined).

The best tone to use is one that is neutral and efficient; prefer active voice but avoid imperative mood unless you're 100% sure you're right, in which case it's usually fine.

It probably goes without saying, but you should assume the change was made in good faith. It's possible that the other programmer hasn't seen before what you consider to be good practices; it's also possible that she's great at her job but just made a mistake. It's also possible that you're wrong. So, for this reason, it's always best to be conservative with tone and stay as factual as you can.

There's not a lot you can contribute unless you worked on the ticket. Notice how the only example you came up with was refactoring.

This was more important back in the day but now linters and formatters are mainstream.

The best time to code-review is at the beginning.

Use number #6. If it's not necessary, then why say it at all in a code review? Everyones got other things they'd like to do.

All the "should" and "considers" are tickets that can go on the backlog after a merge has delivered functionality.

Then YAGNI will probably turn out to apply.

How about

* Why isn’t this extracted to a separate function?

Static Analysis should be able to provide a "complexity" metric. If the method reaches a certain complexity, certain tools will consider it a bug. Refactoring is sometimes the only way to address these issues.

It's about power dynamics.

You ask someone above you. You tell someone below you. You discuss with someone on the same level as you.

If you're smart, you'll quickly spot the assholes who think they're above others - you probably shouldn't comment on their code at all, other than 'great, looks good'.

If you think someone is below you, you're an asshole.

If you want to discuss, you need to include the word 'because'. 'Hey, can we do X, because I/we will need it for Y?' is the way I usually go about it. By the way, discussions are best had before anyone writes code, not after. So for me, I don't do code review - if someone's bad enough that they need their code reviewed on a regular basis, I will be leaving soon anyway.

I normally say why - even if they're 'experienced' sometimes people can sometimes come from, say in this case, cultures where copypasted code is normal. So say why:

* "We're repeating this in a few places. Could you extract this to a separate function? It'll keep the codebase smaller and more readable, and if we update it, there's only one place we need to change."

If it's negative feedback or something that feels controversial I dont put it on a PR at all.

I try to do it in person or (remotely) send a private slack message.

People can be sensitive to negative feedback and one way of ameliorating that is not to give it in a public forum where everybody can see it.

If this is on an open source community, it's bad advice IMHO. On open source communities, please make all feedback public. It is really important to make the history of the project public. There are many ways to write the feedback in a not-offensive way, many of the comments in this thread have good suggestions.

I was assuming a normal working environment.

OSS is a different kettle of fish. You dont have to worry about bruised egos nearly as much & the value of public feedback is higher.

Code reviews are primarily a way to detect bugs and to verify compliance with a set of rules.

Most comments are therefore expected to be 'negative' in some way.

I wouldn’t be happy working in the team where people constantly consider #6 as rudeness.

This looks so unproductive and unprofessional to care too much about the tone vs content in programming business.

That's fair if you have the same stance for all social interactions. If you don't then it's odd since nothing about programming makes people less human and less impacted by the usual social considerations. Although even if that is your stance for everything it's likely not the stance of everyone on the team.

I've seen people hurt by 1-4 versions though - it's not clear for a lot of people (especially non-native speakers) that those questions coming from Americans actually represent a veiled command shrouded in faux politeness.

I've seen junior devs from other countries get into trouble because they ignored these kind of "questions" since they actually thought they were optional questions and not expectations by other developers.

People can go ahead mod me down for this, but I honestly don't care at this point. Here's the best ways to avoid this:

1.) Quit doing PRs for feedback. Start doing pair or mob programming. Bonus points if you ditch PRs completely and do https://trunkbaseddevelopment.com/ instead while you're at it. Asynchronous code review via PRs is waste (in the Lean sense). 2.) If you still want PRs despite pairing & mobbing (or you're about to tell me you've never done it, you're team won't and/or whatever insert lame excuse here as to slam/dismiss it without trying), spend a faster 5 min via screen share and audio AND CAMERA! Do the PR like an old skool code review session live/remote + camera, but have the author ANNOTATE for their own notes what you've reviewed when they go back to revise. 80% of human communication is non-verbal - interact w/ voice & camera. Help them learn & build confidence, then you won't worry about your writing tone. 3.)