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?

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

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.

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.

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.

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.

Buddy, you let that issue languish for a year. How about let my junior dev friend have a go without being a nitwit reviewer?

Thing is, in most cases it's not meant to be nasty at all. Their intent usually is to point out they think there's a better way, and are either trying to say you should do it that way, or are trying to invite you to give your reasons. The problem is they're phrasing it in a manner where there are multiple interpretations to their intent.

Given that commenters responded negatively to it - beyond what I had originally meant when I highlighted it - makes it a good example of how not to communicate.

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.

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

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

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.

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

My trick is to write the wtf in pending review then go and look later in the day if it's still valid. Often I just needed to eat something and I'll be over it.

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

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.

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.

Fix your linter and autoformatter.

You don't pay me to lint code.

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

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!

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?

> My natural tendency is to do it one way, but our style guidelines prefer it the other way

Which is fine if it's a reason it's an occasionally slip up. But apparently, in spite of knowing this tendency, they frequently submit code to review where they've messed up to such a degree that reading every instance is considered annoying. Think of the poor reviewer who had to put all the comments in!

I previously worked on code bases where it had to be one way. It became pretty engrained in me. Now it needs to be the other way. I try my best to use the "new" style now, but because I'm human and busy, I sometimes forget. I spend most of my time focusing on the problem I'm working on and not how I'm placing characters on the line.

I'm not the only one, so I don't think I'm being a particularly bad programmer. We have one reviewer who's really concerned about star placement. It's sometimes the only thing he points out, and he points it out on a lot of different people's work, so I'm certainly not worse than anyone else on the team. As I say, I do try, but sometimes I forget.

The other thing is sometimes I didn't write the code in question. I hit the issue where I'm refactoring and I move a chunk of previously-written code that didn't follow the guidelines, but I didn't modify the code I moved, so now it looks like I "forgot" but in reality, I just moved some code around. How much time do we want to spend reformatting code instead of working on the actual problem we're trying to fix or implement? I would love if we could automate this, but see my other comment about the issues with that.

I think these are all understandable things that busy people have to deal with. It's not that I don't think I shouldn't have to follow the rules. It's that some rules are much more important than others, and differences in code that have the same readability but don't in any way change the functionality seem less important to me than things that can actually cause bugs.

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…

For a python project for example, you could set pre-commit to check formatting, linting and typing, which means that when your code reaches the remote repository, before any CI is even started or any PR is openned (before you even push really), you know your code follows the rules that have been set for the project. Thus everyone knows you don't need to spend time in the code review looking for these mistakes and instead focus on the actual features.

pre-commit does not even need to be pushed to remote to work, so if your company has no plan to enforce it but it still helps you, you can still use it, unlike a CI.

As for this: > 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!

It's true that one mistake is easy to fix later, but it's just as easy to fix now. If you postpone it once you will postpone it again when it's not highlighted in a PR diff, it's just a recipe for never fixing it. Every language has automatic linters/formaters that check (and even fix) your code according to rules you define, turning the work of finding these mistakes into a simple `lint ./src/` command. It's takes a small amount of time to setup but saves you a lot of time down the line.

The problem with my current project is that it has millions of lines of code and that code all has different styles. So even if you only check the lines that actually changed in a pre-commit hook, you can end up with different styles in the same file, which can be as bad as using the wrong style. At least if it's consistently wrong, working within a single file is doable.

> It's true that one mistake is easy to fix later, but it's just as easy to fix now.

Eh... maybe. I see your point, but I guess what I'm saying is "choose your battles." Every minor thing one points out like this takes time away from me fixing bigger issues, so be judicious in how much of a stickler you want to be for the rules. Yep, I can fix it now pretty easily, and then I won't be working on the next bug until it's fixed. And the way we have GitHub set up, if I make any change, I have to get at least 1-3 reviewers to re-review the code and re-approve it. That's the real time waster because everyone else if fixing the nit-picky things for their latest pull request. If they take too long, then I have merge conflicts and I have to address those before I can commit, and, oops! that causes the review clock to start over. We get to do it all again! Talk about Kafka-esque.

If we were starting a new project, I would probably demand that we start with some sort of pre-commit formatter that forces the appropriate style onto our code, but we're unlikely to be doing that for the foreseeable future, unfortunately.

If my eyes stumble on formatting violations every other line, you're probably also not going to get good feedback from me.

Code formatting isn't very important, yet it is.

I hated it. It absolutely should be automated, but I could not find any tools for formatting Cypher query strings inside Javascript code. I'd probably have to write it myself.

https://github.com/NateEag/diff-check

Maybe someone's built the complete version out there somewhere?

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.

Consider avoiding the entire formatting arguments as part of the code review process. There are excellent linters for every language that can run as part of the check-in process. GitHub + Actions can automate every aspect of this using all turkey stuff.

Automating where possible avoids all the arguments, actually enforces standards, and saves time for everyone. Using "Standard" linting rules (aka: The defaults) also help promote good practices for everyone.

That is not going to help me if people send code to review that has formatting problems. We catch a lot of stuff using automated tools, but there's some stuff that you cannot catch. Some people declare every variable as global rather than private, and then I have to follow up with "This variable/constant should be private, any reason you marked it as external?" etc.

You can't just automate everything, and we also have our own specific code formatting guidelines that a lot of tools don't automatically detect by default. I have raised some of these points to our toolchain and engineer productivity teams but it's not my job to dig deep into our tooling and automation so until there's better support in our internal tools (we don't use github + actions) to detect that, I'll keep pointing out any formatting mishap that gets through our linters.

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

Large companies pick experienced internal transfers for shiny new projects and hire externally for unglamorous legacy systems. Since all new grads are external hires, they end up starting out on legacy systems.

I recommend that junior devs start out at Amazon. They will learn a lot of important things in a year or two: the pain of technical debt, the value of automated tests, the pain of production issues when you're on call, that working overtime is not rewarded and does not solve job insecurity, and that switching companies is a great way to get more experience and earn more money.

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

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.

If someone disagrees I expect them to speak up, if they’re not so inclined I’m interested to know why, so we can address/resolve that and move on.

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.

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

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

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

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

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

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

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

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

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

Ego isn't bad. To use it in a context directly indicating it's bad is wrong. If the world agrees to do away with the word ego then it must be replaced by something that also means "a person's sense of self-esteem or self-importance.".

A coder without any ego is a coder who doesn't care, at all, about anything.

It would be clearer to just say, "don't be an asshole" but that comes off as crass in professional communications.

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

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.

It gets tricky. I haven't had to deal with this friction for many years, but it's not because I entirely avoid the type of directness described in OP's last two examples. It's because I'm careful about who I'm direct with, and because I've ended up in environments that are increasingly devoid of the emotionally-fragile.

I start out with a baseline of assuming that people are as fragile and childish as what your comment alludes to. In many companies, I'm sure this is actually the case. But as I get to know a coworker, it's very often the case that they have the talent, maturity, and emotional stability to handle direct communication (in both directions) without spiraling into an episode.

In my early career, I joined a company with an awful hiring pipeline as the first employee. That was my first introduction to the idea that a lot of people react violently to being treated with respect if they think they don't deserve it. I quickly started treating them like children as you suggest, and the problem immediately vanished.

But as I've progressed in my career, the amount of time I spend around mediocrities has plummeted, and my prior that the person I'm communicating with is a mental adult has risen. I've found that high-productivity professional contexts weed out the emotionally-unstable in the same way they weed out (eg) the disorganized; the value of good-faith efficient communication without mental breakdowns is simply too high.

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

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

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.

However, I think it’s worth pointing out, that at no point in the mentioned code review was I mean, or disrespectful. I was just direct vs. my usual indirect self. I never said anything like “this is stupid” or anything personal. I didn’t have any malicious intent nor did it come off that way, unless you had gotten a review by me before and knew how I usually code review:

“I think this would be better expressed as X for performance, wdyt?”

became:

“Express this as X for better performance”

I didn’t use my lack of being awake as an excuse at work. I apologized and just said I didn’t realize how wording things differently would affect my coworker and it truly did upset me as well. It probably wasn’t until I worked with someone who always reviewed like that did I realize how annoying that kind of wording actually was, but that wasn’t until years later. Anyway, it was a weird day, because I wasn’t intentional in my wording and I paid for it. It’s especially weird when you know exactly what happened and you want to explain it so that maybe the other person feels better about the situation but doing so would just come across as being an ass or just trying to make an excuse. It’s a terrible feeling when it’s a real issue in your own life, but other people don’t have a basis to relate. Further, it a terrible feeling to see someone suffering from it and all you can do is feel shame, and so sorry for the person you’ve said something bad too without even being aware of saying it, or unable to stop yourself from saying it.

So yeah, I don’t think it’s ok, but I’ve accepted it at this point in my life. Whatever bs comes out of my mouth, I accept responsibility for that bs.

A) Your co-worker is more sensitive than anyone I've ever met B) You are expressing yourself much harsher than you think you are

Again, I don't know you but pattern-matching based on my experiences it's almost always B.

I know this is a complete tangent, but do you find this to be true specifically of habitual coffee drinkers? I have a pretty horrid history with a sleep disorder, and while I'm a little slow after waking up, I don't think my mood is especially worse. I wonder if it's simply because I don't have a stimulant addiction like most people do.

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.

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.

yes I agree with this 100%. I've gotten nasty reviews from people i like and it didn't bother me at all.

"You wrote the code, and you seem to be unable to admit that your code was buggy. It's not a compiler bug. It's your bug. Stand up like a man, instead of trying to flail around and blame anything else but yourself."

I think software developers would do well to take such a course.

I've seen modern art - it's not working.

Was this developer generally unprofessionally sensitive? What happened to your manager?

I had a similar overly-sensitive colleague. (She was support, and would get flustered at my short responses like "please file a ticket" or "logs.")

She left the day after I met with her and explained common sense. (Customers expect you to know how to use the product; don't bypass the ticketing system by emailing me directly; follow the documented process to escalate to engineering; don't just copy and paste customer emails without doing your own investigation...)

I was having a lot of trouble with getting my manager to intervene with her, so in this case her departure was a godsend. We ended up firing my manager a little later, and things ran so smoothly afterwards that I realized he was the real source of the problems.

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.

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

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.

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

So "Green: Why did you do it this way?" immediately comes across differently to "Red: Why did you do it this way?"

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/

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

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

Feedback Ladders: How We Encode Code Reviews at Net-lify

https://www.netlify.com/blog/2020/03/05/feedback-ladders-how...

How to Make Good Code Reviews Better

https://stackoverflow.blog/2019/09/30/how-to-make-good-code-...

Best Practices for Code Review

https://smartbear.com/learn/code-review/best-practices-for-p...

Code Review Guidelines for Humans

https://phauer.com/2018/code-review-guidelines/

Unlearning toxic behaviors in a code review culture

https://medium.com/@sandya.sankarram/unlearning-toxic-behavi...

How to Do Code Reviews Like a Human

https://mtlynch.io/human-code-reviews-2/

Stop Nitpicking in Code Reviews

https://blog.danlew.net/2021/02/23/stop-nitpicking-in-code-r...

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.

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

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.

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

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.

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.

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.

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

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

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.

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.

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

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

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.

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.

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.

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

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.

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.

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

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

I guess I will say this: Code review isn't about who can write the best code. Nobody can write the best code. Rather, code reviews help push the team to ensure that only the best code gets pushed.

I guess the only other advice I will give is to never criticize code without suggesting an alternative approach. Present that approach in the pull request and not privately. Accept criticism on the behalf of the code writer as well as others. Understand that no single developer will have the perfect solution to a problem, even when comparing juniors to seniors. I've been in this business for more than 20 years, and I still get surprised by fresh blood. If you aren't surprised, you aren't growing as an individual developer.

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.

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

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

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

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

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.

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

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.

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!

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.

I could even be wrong. Maybe it's not an issue at all.

So I'll say "Could we move this out to a separate method? recountFooToilets seems pretty sepf contained and reusable" or just "I'd extract that method".

I don't want to say anything that implies I know better than you.

I'm pretty big on best practices, and a lot of people aren't, so when I find something like that I try to frame it as my problem, not their problem.

Especially with wheel reinvention, in-house implemented algorithms, use of low level primitives when the language provides high level things, lack of unit tests, or any other lone genius/cowboy coder/three star stuff.

I try to use the kind of tone Rust's compiler uses. "I don't have any reason to believe there's anything wrong here, but I don't know how to evaluate it, I don't have any experience with this style, it's above my skill level, and probably above some others too. Doing X is the tried and true standard, anyone can understand it, it will be easier to maintain".

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.

* Please extract this to a separate function.

And I think my favorite is

* Should we extract this to a separate function?

Asking the person for their opinion seems more productive than commanding them to do something. If you word it like a command, then you're basically stating that you know better than the author and the author has underperformed. That's my opinion at least.

I literally had someone write me an impassioned email claiming that questions about why he made a design choice "served only to humiliate him, not an interrogative purpose", and that "when he did something wrong, I should just tell him what to do instead of asking him why he made that choice". This was beyond insane to me: I would hate it if someone spent 15m reviewing a design I had spent days on, and left no space for the possibility that they missed something.

Sometimes there really is a well-established relationship in which both people are aware that one person has much stronger knowledge of how to write healthy code than the other. While my instinct was to still approach the other person as a peer, experience has taught me that it often causes much _more_ friction to maintain this pretense.

That's an interesting point about high skill/experience/knowledge inequality leading to unequal code output. It seems that the less skilled person wants you to sign off on the code in which case they turn their brain off a little and defer to you. I think approaching the matter as a peer suggests you expect this person to learn from you and so you are giving them room to grow, explore and ask questions but this means they need to commit more time to solving problems because clearly if you just did the work for them then you'd arrive at a solution much quicker. Sometimes it feels like we are under so much pressure to produce that there isn't time to really master something just let your coworker flex their strength and the problem goes away.

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

No, this was a white guy born in America. I think it was just a self-esteem thing: he didn't feel worthy of being treated as a peer, and wasn't emotionally healthy enough to handle the confrontation of one's mistakes that good engineering requires.

> That's an interesting point about high skill/experience/knowledge inequality leading to unequal code output. It seems that the less skilled person wants you to sign off on the code in which case they turn their brain off a little and defer to you.

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

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

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

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

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

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

I think the no-ego environment does a lot of work towards being more effective and increasing self-esteem. It's just so conducive to learning! But learning can be difficult especially if someone who's smart has never really been challenged as I can imagine being the case if you're doing advanced research where everyone has so much deep expertise.

I can only speak to my limited dataset, but it's a lot more correlated with self-esteem than actual skills IME.

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.

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.

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

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

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.

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

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

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

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.

If they already self reviewed the code and it has problems, you can write a more detailed message explaining why there's a better way to do things. This way it doesn't come off negatively because the comment is a constructive suggestion. You may also have to compromise some lower priority feedback for development velocity as long as the submitter understands and can improve in the future.

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.

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.

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.

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.

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

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.

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

    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.

    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.

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

Just my $0.02.

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.

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

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.

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

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.

“Instead of doing A here, let us instead use a B approach, otherwise there is a risk of C happening.”

And for shit: “This isn’t great because X. A refactor needs to take Y into consideration, which will result in Z-quality code.”

It’s all case by case, though, in reality. If I know someone we’ll, I might ask why they went with a certain style or a particular algorithm and just talk them through to grokking the error. If they’re somebody who is newer to the field, I usually point them towards a linter, especially if what I’m reviewing would’ve been flagged by said linter.

Stereotypical corporate Americans of course would use the first two, but in Australia using language that tiptoes around peoples' feelings too much (e.g. making sure to point out positives even when someone clearly fucked up) can make it look like you don't trust them or think they're incompetent.

The only other time we'd hear that level of "compassion" is when we have way too much to drink and are acting like a knob.

I've heard Europeans make similar comments about American corporate norms confusing things.

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.

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.

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.

Assuming the code is processed by SonarQube for regular branch builds, which it should be.

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

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

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

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.

But I see code review as a conversation. Quite often it calls for a bit more discussion, and, depending on how the team deals with this, I sometimes reach out to the developer whose code I reviewed over a separate channel to discuss the issue a bit more.

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

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

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.

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.

If not, then only the first version is acceptable.

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.

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!

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

On my team, the PR author is ultimately responsible for their work. All review comments are considered suggestions. The Approve button is used only to indicate completion of the review. If a reviewer has a problem with the merged work, they can open a new case describing the problem.

We train all developers to have sufficient competency to work on any part of the code. If some code is too complex for a developer to understand, it is simplified. Documentation is prioritized and included as part of the work effort.

We have worked together this way for about 7 years, with high developer satisfaction and low turnover. The quality of the product has steadily improved during that time.

I'd feel very low to none incentive to spend time first understanding the problem and then reviewing the solution to the problem if I knew that my opinion can be disregarded just like that. Seems like a waste of time from reviewer POV.

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.

“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 …”

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.