Hacker News new | past | comments | ask | show | jobs | submit login
Ask HN: What tone to use in code review suggestions?
302 points by zorr on June 27, 2022 | hide | past | favorite | 298 comments
When writing suggestions in code reviews I have used all of these forms:

* Should we extract this to a separate function?

* Could you extract this to a separate function?

* I would extract this to a separate function.

* This could be extracted to a separate function.

* This should be extracted to a separate function.

* Extract this to a separate function.

As you can see, these have very different tones and I would like to be more consistent and as constructive as possible. Is there some general best practice for this? Are you or your team using a set of rules or guidelines?




Review: a formal assessment or examination of something with the possibility or intention of instituting change if necessary.

None of your examples provide feedback as to why you want a change. That may be why you have been led to ask this question.

Consider:

* There is a potential overflow in this code. The library function xyz already does this and can log when the app is in debugging mode.

* This portion duplicates the same set of processes as over there. Refactor these into a single function, and we'll be good to go.

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

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

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

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

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


I've long wanted to write a blog post on applying what I learned from effective communications books to code reviews.

Your comment mirrored something I wrote in another thread about the problems with the Socratic method in general[1]:

"If you have a concern, then express the concern openly before asking your question. This will make it clear to the recipient what your intent is, and they will not have to guess."

The worst comment I see in code reviews (and sadly, all too often) is:

"Why did you do it this way?"

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

Even this is problematic:

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

Possible (unhelpful) answers:

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

"Because it solved the problem."

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

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

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


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

Argh, yes, that's one line that might make me just walk away from a PR as a new/junior/casual contributor.

You, the reviewer, are an expert in the system. Likely you are the or one of the most expert people in the entire world on this exact thing. You know X exists and why to use it. As you should, because you put it there. You also should know that people who aren't experts (like me) don't know about it, simply because they didn't use it, in this PR, when they should have. Why don't they know it? Probably because you haven't used it consistently in your own code, or it's not documented. This newbie has cobbled this PR together from what sense I can make of this project. Probably 90% is guessed from code I found in there already.

What wouldn't wind me up?

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

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

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

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


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

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

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

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

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

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


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

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

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

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

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

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

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


> "Why did you do it this way?"

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

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

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


Seeing an open source project's reviewer begin with "Why did you do it this way instead of X?" is the the most shocking shutdown I have seen this year.

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


> Seeing an open source project's reviewer begin with "Why did you do it this way instead of X?" is the the most shocking shutdown I have seen this year.

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.


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

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


Thanks!


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

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


+1 the reasoning is the entire point and all of these examples are good.

Just telling people to make changes without telling why doesn't teach them anything - especially when sometimes what's being requested isn't actually better (or is at least subjective).


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

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


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

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


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

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


Sometimes you catch people in low energy or bad moods. It doesn't excuse the behaviour but try not to take it personal.

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.


I would talk it out with the person. Such comments aren't helpful at all, unless the mistake made on that line is obvious.


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

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


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


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

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

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

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


This!

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

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

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


I ignore style check nitpick comments.

Fix your linter and autoformatter.

You don't pay me to lint code.


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


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


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


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


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


The original post seemed to be concerned about tone, not content.


That’s the point: the replies are saying that the content is more of an issue than the tone, and so OP should focus on the hints for the former.


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


Because all of the versions have the same problem regardless of tone. If the assumptions are in error and the OP had been leaving off the justifications suggested, they can reply and say so! (But, as it turns out, didn’t.)


My rules for reviews (which influence the language used) are:

* Criticise the code, not the author ("There's a bug here" vs. "You inserted a bug here");

* Reach common agreement that people shouldn't cling to code written ("sunk cost fallacy"). Code is liability, and it's okay to throw things away and restart from scratch, the value is in the learning;

* Don't use questions that invite discussion if you're not inviting a discussion, instead expose the reason you believe something should be different ("Should this be like X?" vs. "I think this should be like X for reasons A, B, C."). This avoids wasting time and reviews that turn into long discussions.

* Don't give orders ("Do this", "Do that") to your colleagues, since they are not your employees, and it doesn't help promote learning & growth by not exposing the reasoning behind the demand;

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

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


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

I'd love some suggestions for how to get people to avoid this type of bike-shedding. While I'd love for us to use some tools to automate this, we have a huge codebase with existing code that doesn't follow the rules, and it's not in my ability to implement said tools. (Big company, lots of overhead, will affect a lot of people, don't have a lot of time, not my department, etc.)

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


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

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

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

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

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


Their concern isn't that other reviewers point it out. It's that they think they shouldn't have to follow the guidelines:

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


Well that's a very uncharitable way to read what I wrote.

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.


In addendum, if a couple of style guidelines don't apply to a couple of lines of code for a couple of people, would it have any meaning at all?


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


Yeah, I've been looking for something to do that, but haven't found something that fits into my workflow yet.


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

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


I don't have the permissions to add it to the CI/CD pipeline. I've filed bugs on our CI/CD to add it, but it's apparently not a high priority (you know, until someone sits down to review code, then suddenly it's the most important thing in the world).


If I were you, I’d be looking to move on from your company then. Developers should own the testing part of the pipeline at the very least.


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


Yes, please. It's awful when a one-line PR changes the whole file to fix formatting


Something as simple as a pre-commit hook that checks files that were changed in your commit and rejects the commit if the rules aren't followed is a very cheap and effective way to enforce a coding style.

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.


I love the idea of a pre-commit hook. I did actually implement this for a past project, and the rest of the team hated it. Whenever they thought it was being "unfair" to them, I'd get email about it because I wrote it.

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.


>(Particularly if they don't provide any useful feedback.)

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've been on a project where we dynamically generated tons of different Cypher queries. To keep those queries readable, consistent formatting was very important, and I had to keep bringing that up in code review.

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.


I haven't yet taken this concept to completion, but when I had a similar issue years ago I hacked out a cheesy program to filter out static analysis messages on lines not modified by the diff:

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

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


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


> * Don't give orders ("Do this", "Do that") to your colleagues, since they are not your employees, and it doesn't help promote learning & growth by not exposing the reasoning behind the demand;

I think this depends a lot. I do "readability" reviews at my company to make sure people apply good coding standards and adhere to the coding guidelines that we have. A lot of these are very "mechanical" in nature and when I am reviewing changes that have obvious coding standard mistakes I will write comments like a "linter" would. One of the most common examples:

> Use descriptive ("Uses") rather than imperative ("Use") style for function docstrings. See: <url-explaining-docstring-guidelines>

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


> I do "readability" reviews at my company to make sure > people apply good coding standards and adhere to the > coding guidelines that we have.

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

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


> Consider avoiding the entire formatting arguments as part of the code review process.

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.


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

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


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


Even when you express it clearly, many juniors won't understand and will feel frustrated. Some things can't be taught and must be experienced. Every junior developer must work on a crufty legacy codebase to develop understanding and habits for writing maintainable code.

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.


That's good advice!


There's basically two categories here:

- you have an opinion on how things ought to be done and want a dialog

- you see some code that's wrong or violates an agreed-upon rule and so it should be changed with no discussion

I'd switch the tone based on what you're addressing. Giving a rationale when you share an opinion or point out a mistake also softens the tone (for the better IMO).

>* Should we extract this to a separate function?

>* Could you extract this to a separate function?

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

>* I would extract this to a separate function.

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

>* This could be extracted to a separate function.

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

>* This should be extracted to a separate function.

>* Extract this to a separate function.

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


I also think it depends a bit on the other person and the rapport one has with them.

At least I try to remember back to when I was a junior, and how "personal" the feedback felt when I was just starting. So when reviewing for others in the same situation I make extra sure it doesn't come off the wrong way by adjusting my tone.

But for someone I've worked with for years we're often a bit shorter and to the point, and everyone's happy.


Yeah I remember being a junior and feeling like blunt comments were kind of rude. So I make an effort to be nicer to new developers - sometimes simply adding “what do you think?”/“do you agree?” is enough to make those command style sentences sound much less blunt, and also encourages them to ask questions instead of blindly following my suggestion if they don’t fully understand it.


To me those questions could come off as condescending. I say could because I wouldn’t think that’s your intent, but the tone is there.


That depends entirely on whether you add the question as a formality or if you are genuinely interested in why the other person might disagree.


To me ’…don’t you agree?’ is the sort of rhetoric that parents use to make their kids internalise their beliefs.

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


The comment says "do you agree?", not "don't you agree?". I agree that the latter sounds condescending, but the former sounds reasonable to me.


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


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

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

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

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


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


There are different categories of things you might see:

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

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

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

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


> I'd switch the tone based on [whether you want a discussion or are insisting on a change]

I think that's a great example of where "tone" is absolutely not sufficient, especially in the modern world where many readers aren't going to share your first language and won't absorb the nuance.

If there's a situation where your disagreement is absolute, you need to say that factually, ideally with a recipe for how to resolve that included:

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

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

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

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


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


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

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

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


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


This is really useful. Thanks.


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


I do code reviews as the meat of my job and have personally benefited from GitLab's handbook article on this topic [1]

    - No ego (Don't defend a point to win an argument or double-down on a mistake.)

    - Assume positive intent (If a message feels like a slight, assume positive intent while asking for clarification.)

    - Get to know each other (Building a rapport enables trust.)

    - Say thanks (Taking every opportunity to share praise creates a climate where feedback is viewed as a gift rather than an attack.)

    - Kindness (It costs nothing to be kind, even if you do not believe someone deserves it.)

    - It's impossible to know everything (You can't know how your words are interpreted without asking.)

    - Short toes (GitLab is a place where others can feel comfortable with others contributing to their domains of expertise.)

Specific to your situation, I'd focus on what others have said as well. If they *need* to perform something as part of their job role, remove the suggestion and place an imperative. Ex: "This needs to be a separate function" vs "You should. . .".

> Communicate assuming others have low context. We provide as much context as possible to avoid confusion.

It also helps to provide context, especially if you have a particular fix in mind. "See this PR/MR for someone who encountered a similar situation recently."

Hope this helps.

1. https://about.gitlab.com/company/culture/all-remote/effectiv...


I agree with "Don't defend a point to win an argument or double-down on a mistake.". I disagree with the juxtaposition that ego is bad, i.e. "no ego". Ego is defined by Oxford as "a person's sense of self-esteem or self-importance.", I agree.

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 feels unnecessarily pedantic to debate the usage of a word in a common English idiom, especially with an outdated definition from Oxford.

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


On the contrary, HN is the place to debate the usage of words. Precise word choice is how to effectively get your point across. Words should have more definitions than the obvious two ("something universally good" and "something universally stupid, annoying, embarrassing, and smelling like fart") and it's not a shame to talk about subtleties of their definitions.


I feel like this is often overlooked.

One morning, I did a code review before having any coffee. I basically used the latter two throughout the whole thing. I came into the office to find my coworker literally crying. Later in the day, someone else said it was the best code review they’d ever read… and asked me to come to their team…

There was so much drama from that code review. That code got merged as-is to appease the author without a single one of my statements addressed (including security considerations), and no one else would review it. My manager was in a tight spot, felt sorry for him.

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

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


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

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

Edited to add:

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


> socially retarded

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


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

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.


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


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

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

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


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

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

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

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


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

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


I don’t think it’s “ok” per se, but it’s something I’ve had to live with, as well as my family. I can honestly say that it’s not something I do intentionally, it’s as though my “social filter” is booting up and I just say the absolute first thing that pops into my head, with the wrong inflection to top it off. I’ve said some pretty fucked up things in the first 30 seconds of waking up. I own them, apologize for them, try to explain the situation so we both can avoid it in the future, etc. I don’t think it’s ok, and I just try to avoid speaking at all. Like I said somewhere in this thread, I didn’t know how abnormal when this story happened. If it makes any difference, I don’t sleep like normal people. Apparently, when I sleep, I become totally unconscious and nothing will wake me. Alarm clocks don’t work, people don’t work, explosions don’t work, gunfire doesn’t work, etc. It was an issue in the military at first and I had to learn how to work with it. So, when I wake up, I’m coming from basically being “brain-dead” to full alert. It literally feels exactly the same as being knocked out and coming back in a fight. That might have something to do with it.

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.


“Express this as X for better performance” doesn't make people cry. So either

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.


> People who wake up "ready to go" seem to be pretty rare.

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.


Using "socially retarded" is a huge red flag to me, about you.


Is this person working on his own. Why can't people pair and give each other at the moment feedback vs doing code review.

Person is already invested in his code by PR review time and doesn't really want to do PR ping pong for their task. What if you come back with more comments after they address your comments. Will they ever get to finish their task or forever be hostage to you subjective comments.

Most of it is usually subjective too. you had to explicitly mention 'security issues' which imply that you have a feeling that most of your pr comments were your subjective interpretation .

People are more open to suggestions during pair programming because people don't deliver feedback in cutting way ( with or without coffee) and its easy for get a common understanding and move forward.


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

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

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

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

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

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


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


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


You sound like a Dutch person. Working with different cultures I've had to adapt to varying levels of directness but like to think it's helped me become more up-front. I've learned to appreciate people that say what they're thinking, so you can avoid the 2nd and 3rd order analysis about intent and thoughts about how other people will receive things.


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


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


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


That's not level==Linus; for that you'll have to tell something like:

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


Needs more profanity to be accurate


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


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


I'm one of the likes who appreciates directness over subtle critique. I hate the latter but I also found out that it seems to work better with most of the people.


They have courses on constructive critique in art school!

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


Try going to an art school. Critique is how we get better.

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


I'm curious, what happened in the long run?

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


I left the company within the next few months, the manager is still doing well. I haven’t kept in touch with the other dev.


Good move.

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.


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

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

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

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

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

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


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

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

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

This massively reduces friction in the CR process.

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


Yeah, that is great.

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

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

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

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


We do something very similar:

* NICE: Optional change. PR is approved.

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

* MUST: Must be fixed. PR is rejected.

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


I saw another person talk about Red/Amber/Green — where Red is "fix this, it's wrong"; Amber is "here's a consideration, do with this what you will" and Green is "I don't know but I want to ask questions to learn".

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


The word "must" should only be used in the Military. Anywhere else, it is profusely out of place. In its stead, I recommend the term "requirement" and then cite an actual reference by the PO or someone officially representing the business requirements.


Can you explain why? I’ve definitely seen must used in non military contexts. It also seems really strict to limit musts to things specifically laid out in requirements, unless I’m just used to dealing with really loose requirements. My gut is that you’d run into two categories of shoulds, one which is a much bigger deal than the other.


And protocol specifications


[Suggestion] Use conventional comments[1] to flag how important the comment is.

Most of my comments end up being "suggestions", but then when I put a [blocking] on it, it clearly communicates that I think this should be fixed before merging in.

1: https://conventionalcomments.org/


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


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


Offtopic but related:

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


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


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


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

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

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

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

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

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


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

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

None of this can happen in a github pull request.

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


I keep a running list of articles I found useful over the years. There are many great articles about Code Reviews out there. Consider this a more general answer to your question:

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


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

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

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

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

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

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

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


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

- reviewer ego

- author ego

- reviewer ambiguity

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

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

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


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

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


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


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


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


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

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

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

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


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

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

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

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

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

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


Smatbear has a great article on code reviews https://smartbear.com/blog/avoiding-the-politics-of-code-rev...

He has the following suggestions:

    - "Ensure that reviews are two-way. Never have people who only review and people who only get reviewed."
    - "Always focus on the code and not the person who wrote the code."
    - "Make the reviews small, frequent, and informal. Marathon group sessions in rooms make people defensive."
    - "Frame things as questions and suggestions rather than orders and accusations. Ask that others do the same."
    - "Automate as many checks as possible so that reviews don't focus on simple details."
    - You can frame the review as optional "asking for advice" instead of a gatekeeper approach of "getting the code approved"
    - Says that the potential harm of the bad approach is worse than taking up the risks, that is taking the risks that come with the policy of not requiring a code review for each and every commit.


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

Not #3 because you're still being coy.

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

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

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


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

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

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

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

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

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

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


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

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

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


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

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

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

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


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

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

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


Based on a great article we read almost 15 years ago ("Effective Code Reviews Without the Pain": https://www.developer.com/guides/effective-code-reviews-with...), we start most code review comments with "Did you consider...?" This makes it non-confrontational and non-judgmental, as the author can easily reply that, no, they hadn't considered that and thanks for the suggestion.

Everyone in the company knows it's sort of a gimmick, and we even make fun of it a bit, but even so it still works!

"Did you consider extracting this code into a separate well-named function with clear inputs and outputs, to make the original function smaller and easier to reason about and maintain?"


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

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

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

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

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


There are three main categories of code reviews:

1.) Things are mainly bad.

2.) Things are mainly good.

3.) Somewhere in between.

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

1.) Something that violates our rules/style.

2.) Something I have a strong opinion on.

3.) Something I don’t understand.

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

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


> "Extract this to a separate function please"

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

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


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

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

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

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

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

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

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

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

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


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

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

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

"Have you considered X here?"

It has a number of advantages:

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

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

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

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

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


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


I think it depends on your teams' expectations for code review. It seems like perhaps your team doesn't define what is acceptable/constructive/valued, etc from reviewers.

Personally I prefer the first one on this list: it gives the author the opportunity to refuse the suggestion. Perhaps its a frivolous request and the PR is already been open for far too long. Maybe the author would prefer to leave the duplication there until there's more justification for extracting it to a function. Who can say?

Not all feedback is necessarily good or useful.

Personally I don't bother with suggestions like this where it's more of a style preference unless there is a style guide to adhere to where "extracting this to a function" would clearly connect to a guideline.

I tend to focus on verifying the that the author did their homework: what evidence/proof did they submit that ensures me the change is correct wrt. specs/requirements/tasks? If that evidence/proof is sufficient that's all I care about: it makes it easier to manage a higher volume of review requests.

I have a pet peeve for suggestions that are "trivial" and based on, "well I would have written it this way," as I don't find them terribly useful most of the time. They rarely affect the specification of the program and their claims to readability/maintainability are often dubious and superficial. Bike shedding is a huge waste of time in the review process. At least when worded with, "Should we..." there's a chance for the author to explain whether they had already considered that or accept the suggestion as helpful and make the change.


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

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

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

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

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


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

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


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

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

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


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

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

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

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

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

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

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


I try to be polite, yet precise. Remember that the author likely put a ton of work into their code. I usually let them know that if they want to collaborate further, we can huddle on slack. I push others to do the same for me and have done so when needed.

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.


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

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

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

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

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


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

‶This code is duplicated several times.″

2) State the change you'd like.

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

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

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


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

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

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

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

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


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

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


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


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

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

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


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

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


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


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

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

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

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

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

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

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


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

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

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

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

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

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

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


I think setting team goals are important before anything. For example if the goal is to create the most maintainable and best version of the code in the long term I think this opens up doors for feedback not being taken personally as long as the person giving the feedback isn't dropping lines like "why didn't you just do xyz instead?".

I tend to provide more open ended questions like "what do you think about ..." where "..." could be a few options to take. In my mind I've most likely picked one based on personal preference but I like proposing a few choices to spark some discussion amongst team mates and give everyone a chance to contribute. Often times throwing out a few options will result in someone coming up with another option that only became apparent after they've seen a few alternatives (which is a good thing).

It also lets more folks make decisions and that's a very important thing. If you keep making every decision then folks won't feel like they can contribute anything because "why bother". I remember an old TNG episode (S3E21) around this with "Broccoli" when he was trying to integrate on the engineering team but Jordi kept alienating him. Picard dropped some really good life advice in that episode!


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

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

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

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

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


I usually prioritize diplomacy over everything else. You are probably better than me at this. The fact that I found an issue you didn't is probably just coincidence.

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


> * Should we extract this to a separate function?

Reads very passive aggressive. No.

> * Could you extract this to a separate function?

Yes, I could.

> * I would extract this to a separate function.

Good for you.

> * This could be extracted to a separate function.

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

> * This should be extracted to a separate function.

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

> * Extract this to a separate function.

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

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

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


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


We have no guidelines where I work. People have different styles. I think the only one I particularly dislike is

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


Fwiw, I used to use respect-by-default until I encountered (multiple times) people who were too emotionally unhealthy and insecure about their abilities to handle it.

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.


Wow that's crazy! Was it cultural issues for example foreign born coworker who doesn't understand how respect in the USA works in text form?

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?


> Was it cultural issues for example foreign born coworker who doesn't understand how respect in the USA works in text form?

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

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

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

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

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

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

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

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


Thanks for the considerate response!

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.


Yea, it's weird. I don't think I've actually really encountered someone smart and experienced who has thus problem. They tend to be secure enough in their own intelligence that honest consideration of their mistakes and gaps doesn't send them spiraling into insecurity and ego-protective backlashes.

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


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

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

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

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

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


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


I think the only correct answer is "it depends"

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

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

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


First decide what do you want:

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

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

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

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


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

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

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

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

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


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

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


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

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

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

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


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

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

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

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

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

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

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

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

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

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


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


Depending on the situation, one of the following three:

> Should we extract this to a separate function?

When you are not familiar with the codebase, potential reuse, and want the submitter to clarify rationale for the approach.

> This should be extracted to a separate function.

When you are the authority but you're open to a discussion. You should include the "why" aspect in this situation.

> Extract this to a separate function.

When you are the authority and there will be no discussion. If you want you can include the "why" but it's a waste of time. You are the law.

The rest are sugar coated versions of the same thing.

"I think...", "I would...", "Maybe we...", are unnecessary additions. Clearly it's what you would do as you're the one giving the review.

If people are too thin-skinned to take direct feedback then they should work on improving that before submitting more code. Life is too short to waste time picking words that try not offend when you can spend that time actually producing something of value.


Default to short, polite statements, but make it clearly suggestive if its optional.

e.g. "Extract this to a separate function please" if you aren't asking. "Should this be a separate function" otherwise.

This also assumes that theres a certain protocol to PRs in terms of veto power. I've always followed the rule that the writer must make all requested updates by reviewer unless explicitly challenged. In other words, you can't silently not change something they mentioned, but you can say things like "thats out of scope for this update" or "I think this design is better because XYZ" and the writer is able to mark as resolved at that point.

Sometimes this ends up with a conversation that needs to happen where the reviewer disagrees with the writer, but honestly it rarely does at least on the teams I've been on. It would really have to be a contentious point to escalate to that level rather than just being a difference of opinion.


I'm not sure if the question asked is even the right question (what tone to use isn't the problem). The code review should educate the developer submitting it if they don't know the points you are making. If they do know the points, I would tell them to review their own PR before another person since it helps everybody. We shouldn't be asking for approval for code that's not even double checked.. that throws the responsibility for catching bugs on the reviewer and not even the writer.

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.


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


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

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


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

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


A - for juniors

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

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

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

B - for seniors

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


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

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

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


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

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


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

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

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

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

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


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

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

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

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


It's about power dynamics.

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

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

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

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


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


    All of your examples are wrong, because you don't state WHY you think it would be better. Never give instructions without at least a short explanation.
Better tone of the above sentence, without "blame game":

    None of your examples include WHY you think it would be better to do something. I never give instructions myself without explaining at least briefly why I think my suggestion is better, because they more likely understand the problem or accept my suggestion.
Even if you think something is better only because your taste, it's still better to explain yourself, so you can agree to disagree. I usually let go small things if something is readable and functionally not different.


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

Yes. This. Clarity is a prerequisite for tact.

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


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

Just my $0.02.


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

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

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


Easy:

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

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


Obviously tone is hard in text, and it's worse in multilingual teams. What's been very effective for my current team is adding explicit context as a comment prefix with standardised terms: - Nit for "it doesn't actually matter" - Suggestion for "this approach might be better, what do you think?" - Question for "help me understand this" - Requested Change for "this is actually a blocker"

We include this expectation in our working agreement, stick to it rigorously for reviews where reviewer and reviewee haven't established a strong rapport yet, and use it as appropriate beyond that. So far we've had no confusion about tone in reviews.


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


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

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

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


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

Should this be extracted into a separate function?

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


“Please extract this to a separate function”


I’ve used a template for about 23y now:

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


I don't think there's a clear answer to this question.

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.


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

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

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

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

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


Lots of good suggestions.

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

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

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

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

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

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

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


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

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

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


I like SonarQube for removing ambiguity. If SonarQube is set to fail the build at lower than a certain percentage of coverage or too low of a grade in Maintenance then it gets fixed before the PR even gets filed.

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


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

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

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

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

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


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


second!


Always friendly. Life is too short.


This one is nice.


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

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

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


"How would you feel about extracting this into a separate function? It might be better because the logic seems quite self-contained, and we can then apply the extra business logic immediately after this with a few if-elses."

I try to give critiques in this way, as it gives the person that wrote the code ample opportunity to defend their approach, but also gives explanation of why your suggestion might be better.


I'm fine with all of these. I think the way you phrase it could communicate how confident you are that this should be extracted to a separate function.

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.


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

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

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


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

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


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

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

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


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

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


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

If not, then only the first version is acceptable.


If you really insist on style review, how about defining a metric which allows the autonomous analysis of the code and autonomous recommendation? Then you can put the rationale in a wiki and maintain stats on how often everyone gets caught. If the thing is so stylistic you can't spec it as code, why waste people's time reviewing?


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


None of the above - I'd explain why I think the way the code was written was problematic (e.g. function logic can't easily be readily comprehended in one go, consider how you could break this down). But to a large degree my "tone" and level of detail would depend on the seniority of the author.


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


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


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


Big thing for code reviews with junior developers: Show them where they've done something right. They'll keep doing it that way. Ideally this is done in person, and when you give them a "good job on this", it really takes the sting out of anything negative.


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

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

Then YAGNI will probably turn out to apply.


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


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


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


I think it depends on how confident you are regarding the change and how important the code is. If something needs to be done, don't use language that suggests it isn't important. Similarly, don't made a big deal out of things that don't matter.


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

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

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

Info: Nice name!


I just give it straight. If they cant take it, then tough luck. Just yesterday I reviewed one where someone had used tabs instead of space. How can anyone in their sane mind use tabs instead of spaces. Chased that PR author out of there and closed the PR.


How can anyone in their right mind use spaces instead of tabs?


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


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


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

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


This is similar to how my team operates.

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.


What's the point of reviews if author can unconditionally merge the PR without addressing the comments?

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.


sounds kinda nice...where do you work...?


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

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

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


Typically I say something like:

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

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

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


My "formula":

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

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

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

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


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


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


Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: