* 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?
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?
* 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.
- 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.
- No ego (Don't defend a point to win an argument or double-down on a mistake.)
- Assume positive intent (If a message feels like a slight, assume positive intent while asking for clarification.)
- Get to know each other (Building a rapport enables trust.)
- Say thanks (Taking every opportunity to share praise creates a climate where feedback is viewed as a gift rather than an attack.)
- Kindness (It costs nothing to be kind, even if you do not believe someone deserves it.)
- It's impossible to know everything (You can't know how your words are interpreted without asking.)
- Short toes (GitLab is a place where others can feel comfortable with others contributing to their domains of expertise.)
> Communicate assuming others have low context. We provide as much context as possible to avoid confusion.
It also helps to provide context, especially if you have a particular fix in mind. "See this PR/MR for someone who encountered a similar situation recently."
Hope this helps.
1. https://about.gitlab.com/company/culture/all-remote/effectiv...
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'd use the more question style comments for the minor changes, and save the stronger comment style for those changes that are really important.
So I'd word a comment about changing a variable name to something slightly better as a question. e.g. Would accounts be a better name for this array?
While a comment addressing a security concern would be a direct statement. e.g. Use a parameterised query here, as it'll be harder for someone to exploit.
One thing to note is that when making the strong statements I like to back them up with a reason, that way it helps me to think through the comment itself, helps a more junior developer understand why they should change it and helps more senior developers to not take the comment personally.
The other side to this though is that as senior developers we need to set an example for our more junior colleagues on how to take review comments. I always try to take them in a good mood, make the changes when I agree with them and have a constructive conversation about those that I don't agree with.
Prefix every suggestion with M, S or C and then just write the suggested change as a plain statement. The prefix handles the severity and importance without you having to worry about tone.
Coulds can be ignored by the coder author with no explanation as to why they are ignoring but if they have time or want to implement the change then they can. Shoulds can only be ignored with a reason and the reviewer has to agree or the change should be made. Musts have to be implemented and should be used rarely. The only person who can over rule a must is a department head, lead or CTO type person.
This massively reduces friction in the CR process.
Article here: https://allthecode.co/blog/post/how-to-get-better-at-code-re...
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.
I view (peer) code reviews mostly as an opportunity to discuss and familiarize your team with your code, and I think they should conform to the "social rules" that prevail in the rest of the world. If a peer in the real world was sharing something with me, I would very rarely command or strongly suggest they change this or that thing about it. Instead I would strive to create a collegial atmosphere by being receptive and supportive 90% of the time so that 10% of the time a suggestion I offered would be received gratefully and gracefully.
None of this can happen in a github pull request.
There was an alternative that I used to see before the PR monoculture took over that I liked. It could look like a weekly "code review" in-person meeting where a random developer would put together a presentation about a new module they'd written and project it on a wall. Mostly the other devs would listen and learn, and any suggestions they made would be strictly optional. The dev might leave feeling a bit embarrassed about something - but never feel "under the thumb" of a peer who was forcing an issue.
Feedback Ladders: How We Encode Code Reviews at Net-lify
https://www.netlify.com/blog/2020/03/05/feedback-ladders-how...
How to Make Good Code Reviews Better
https://stackoverflow.blog/2019/09/30/how-to-make-good-code-...
Best Practices for Code Review
https://smartbear.com/learn/code-review/best-practices-for-p...
Code Review Guidelines for Humans
https://phauer.com/2018/code-review-guidelines/
Unlearning toxic behaviors in a code review culture
https://medium.com/@sandya.sankarram/unlearning-toxic-behavi...
How to Do Code Reviews Like a Human
https://mtlynch.io/human-code-reviews-2/
Stop Nitpicking in Code Reviews
https://blog.danlew.net/2021/02/23/stop-nitpicking-in-code-r...
Personally, I have roughly three distinct levels of things that I comment on in code reviews:
1. Does not need response, but is my personal opinion about how the code probably would look nicer. In this case, I would do #2. If you just say "no," that's fine. I can revisit the issue later. Some people think these comments don't actually belong in a code review, and I think it depends a lot on your relationship with the code and the person asking for review.
2. Needs response, but is potentially open for discussion instead of a change to the code. I would do #4 here.
3. You must make this change or I will not accept. I would personally do #6. There is no need to use "non-confrontational" language with many people when you mean to be confrontational. There is a hard line about what you will and won't accept. If it is a major comment, you need to explain your rationale here out of courtesy to the person sending you code.
Most things that fall into the third category are around code style conventions and minor bugs. More substantive changes, like changes to an architecture, often fall into the second category.
Edit: Also, when I nitpick your code for one reason or another (which generally only happens when I am enforcing a company style guide), I will often say "Nit:" and then use the sixth style. If you are new to the team, I will quote the guide to you. Most people don't feel so bad about the nitpicks when you admit that you are nitpicking, and that it is for a reason.
"I find this method to be overwhelmingly long and it's hard to keep track through all of it. However, it looks like this, this and this are rather isolated steps, they could be extracted to make the overall structure easier to see".
"To me, this, this and this look like duplicated code. Do you expect them to evolve differently, or could they be extracted into a common function / module?"
And if the code goes against architectural or style guidelines, it also softens the feedback to start with that reason and go to the suggesting from there. "This class was designed to be independent of that module to be testable. Please extract your code into the foobar-interface and inject it"
To my view, coding is as much art as science, and it is limitlessly fascinating to me how different people find different ways of expressing ideas and solutions.
Also, the code review process is fraught with opportunities for insult, misunderstanding and unfortunate power dynamics. It is inherently difficult regardless of the actual content being reviewed.
On the other hand, if there is a significant issue here "I am having trouble following your thinking here. Perhaps dividing this up into smaller functions would help?" Might be a good review comment?
The style wars are very tempting to engage in, but they virtually never drive greater productivity, real quality improvements, or positive team dynamics.
If there are real reasons that this wants to be broken out into a separate function (reusability for instance), then make that clear in your suggestion.
He has the following suggestions:
- "Ensure that reviews are two-way. Never have people who only review and people who only get reviewed."
- "Always focus on the code and not the person who wrote the code."
- "Make the reviews small, frequent, and informal. Marathon group sessions in rooms make people defensive."
- "Frame things as questions and suggestions rather than orders and accusations. Ask that others do the same."
- "Automate as many checks as possible so that reviews don't focus on simple details."
- You can frame the review as optional "asking for advice" instead of a gatekeeper approach of "getting the code approved"
- Says that the potential harm of the bad approach is worse than taking up the risks, that is taking the risks that come with the policy of not requiring a code review for each and every commit.Not #3 because you're still being coy.
I'd suggest #4 or #5 when dealing with peers but they aren't interchangeable, they mean different things.
Save #6 for when you're giving tons of feedback to a junior.
If you're worried about being perceived as harsh or unfriendly, remember you can still be super friendly in the chatter on either side of the review session.
If I don't know how to phrase my comments I just schedule a call and we go over the code together. Much less time spent than on comment ping-pong.
If the code generally works, but other aspects, like maintainability, would benefit from some rework I use the phrase "You can...", e.g. "The second argument to the `map` callback is the element index. You can use it to create this range instead of using an external variable incremented on each step."
The recipient has the possibility, but is not actually forced to do anything, just like with code suggestions. In the vast majority of cases they follow my advice though.
If there's an error or a kludge I use:
-Code suggestions - no comment, so nothing to get upset over and if the recipient is feeling particularly irritable, they can just ignore it.
-Direct language in imperative form, e.g. "make sure that X, otherwise this won't work as intended", "avoid X in Y", "use Z here (documentation link)".
If instead of saying what you would do, or what you think they should do, you express a concern about the code, the tone is completely different. Now it's just saying something like "do you think repeating this across files will be problematic?", which allows the other person to be the determiner. If they agree they may well do exactly what you would have suggested, and if they don't, it can start a discussion that helps bridge together how various team members code. It's win/win.
It also helps people understand what you aim for when writing code, which helps them keep in mind your needs in the future (and vice-versa for you to keep in mind their needs).
Everyone in the company knows it's sort of a gimmick, and we even make fun of it a bit, but even so it still works!
"Did you consider extracting this code into a separate well-named function with clear inputs and outputs, to make the original function smaller and easier to reason about and maintain?"
- This code looks pretty similar to this other code; is there a way it can be deduplicated?
- I don't understand this code; I think it's because there are too many conceptual steps. Is there some way to structure it for better readability?
- Is there a way to unit test this specific behavior?
Sometimes this style doesn't work (e.g. correcting a comment that has become incorrect due to the change), and in those cases I usually just say "Please do this." I also try to point out things I like when they are present.
1.) Things are mainly bad.
2.) Things are mainly good.
3.) Somewhere in between.
Within those, everything I see and flag as worthy of discussion is either:
1.) Something that violates our rules/style.
2.) Something I have a strong opinion on.
3.) Something I don’t understand.
Depending on which main category the review falls into, I’ll deal with these differently. If things are mostly bad and I don’t understand something, I’ll ask a lot of questions about readability and naming conventions. If something is mostly good and I don’t understand something, I’ll ask “Hey, can you explain lines 71-104?”
In general, I try to keep things friendly and positive because I don’t want people to dread the experience. Life is too short to dread a big part of your job.
This sounds really bad. Even if you’re their boss. You’re dealing with professionals and part of that is considering their input.
I’d start with the assumption that they know what they’re doing and might have had a good reason and phrase it from there. Even if not true it sets the right tone of respect.
The actual words used should be determined by the goal of code review and the relationship between reviewers.
Anything that can possibly be automated by CI should be. Style guide and test coverage should automatically fail PRs before a human needs to add any "nit"
That means code review should be a teaching or question moment (between a senior and junior, newer and veteran) or a discussion between peers.
From a junior to a senior -> why did you extract this? why didn't you do it this way?
From a senior to a junior -> typically, I would extract this because X. Check our best practices doc for a longer explanation.
Between peers or senior/junior (maybe one is newer) -> it isn't in our coding best practices document, but normally we extract this because X
Between peers -> what do you think about extracting this because X? Should this be in the coding best practices document?
The last is important because ideally you're updating your coding guide so this same review doesn't need to be done 10 times. And, if you don't think it should be documented as a best practice then don't bring it up in a review.
There is no offence to be taken, because the deeply held respect for each other as team mates is already there and taken as a given. You trust each other to know that any comments are addressed directly to the code, rather than to the person behind them. All input is valued because everyone knows the goal is just to make the code better and help each other out, rather than criticise an individual. Discussion can be frank, open, unbiased and non-confrontational.
However, because that takes a long time to develop and is somewhat of a rarity, I've had quite good luck with the phrasing:
"Have you considered X here?"
It has a number of advantages:
* It doesn't imply that an author should have done it a different way
* It doesn't imply that an author has missed something you think they should have covered
* It's a good starter for a conversation, rather than a conflict
* If the answer is "no", it provides a good prompt for an author to come up with a suggestion first
* If the answer is "yes", they can explain why they ended up with the solution they did, which is visible to everyone who wants to look at the PR. That can also be a good prompt to add a comment
Personally I prefer the first one on this list: it gives the author the opportunity to refuse the suggestion. Perhaps its a frivolous request and the PR is already been open for far too long. Maybe the author would prefer to leave the duplication there until there's more justification for extracting it to a function. Who can say?
Not all feedback is necessarily good or useful.
Personally I don't bother with suggestions like this where it's more of a style preference unless there is a style guide to adhere to where "extracting this to a function" would clearly connect to a guideline.
I tend to focus on verifying the that the author did their homework: what evidence/proof did they submit that ensures me the change is correct wrt. specs/requirements/tasks? If that evidence/proof is sufficient that's all I care about: it makes it easier to manage a higher volume of review requests.
I have a pet peeve for suggestions that are "trivial" and based on, "well I would have written it this way," as I don't find them terribly useful most of the time. They rarely affect the specification of the program and their claims to readability/maintainability are often dubious and superficial. Bike shedding is a huge waste of time in the review process. At least when worded with, "Should we..." there's a chance for the author to explain whether they had already considered that or accept the suggestion as helpful and make the change.
When I've done reviews, I had some seniority and an organizational privilege to veto some code. I worked from a checklist and a goal (with which the checklist was meant to align, but we knew it was not possible to fully automate those aspects of review). These are my takeaways from that arrangement:
Language like "declined" or "REJECTED" or "can't approve" is discouraging to the individual contributor. I replaced all that with discussion of why I can't allow that, under the obvious subtext of rejection. No need to just rub it in when there is learning to offer.
When indicating required changes, especially where I was more-or-less handing them the replacement code, I always said please. Always.
Most importantly, I gave accurate feedback. I took the time to be sure I was right before I wrote a review. Otherwise what's the point. Even simple patches got tested.
Btw, since I think it is relevant to this discussion, we're working on making code reviews deeper than text based diffs. Check out (DiffLens)[https://github.com/marketplace/difflens] if you're code base is primarily TS, JS and/or CSS to get language aware diffs on your GitHub pull requests. We've found that it makes understanding code changes much easier
Its not the tone but rather the feedback style according that should be adapted to the knowledge/motivation of the engineer in the situation. For example by using the https://situational.com/blog/the-four-leadership-styles-of-s... herustic:
1: Engineer is junior in the context and insecure/unmotivated: Do X 2: Engineer is junior in the context and motivated/secure: Take the decision for the engineer and explain why 3: Engineer is senior in the context but insecure: Coach by open ended questions: how large of a function do you think is appropriate? 4: Engineer is senior and motivated/secure: From your perspective how should the function be and how should we do this in the future to reach our goals.
I know some devs that are anxious if they don't fix _every_ single comment in their PR, even if I'm just suggesting something as "nice to have". This helped to mark which comments are actual priority.
Also, make sure you're using code formatter in your CI, to avoid reviewing the code style, as this can start some unnecessary wars and frustration.
Also, I suggest sharing this article https://mtlynch.io/code-review-love/ with your colleagues, especially step 8:
> As the author, you ultimately control your reaction to feedback. Treat your reviewer’s notes as an objective discussion about the code, not your personal worth as a human.
To some degree, as a reviewer you can keep your tone friendly, but ultimately _every_ sentence can be took as personal attack, and you can't really do anything about that. Keep that in mind, don't focus _too much_ on the tone, and don't let that prevent you from giving a comprehensive review.
One other thing I personally do, because I'm famous for giving _really_ thorough reviews, is to make sure from time to time that the other person is fine w/ that. Simple DM "I hope you don't feel too overwhelmed by my CR?" helps, especially that often people respond that they actually approve I'm so thorough. If you worry people get offended, it is easy way to make sure if these worries are valid or not.
I guess I will say this: Code review isn't about who can write the best code. Nobody can write the best code. Rather, code reviews help push the team to ensure that only the best code gets pushed.
I guess the only other advice I will give is to never criticize code without suggesting an alternative approach. Present that approach in the pull request and not privately. Accept criticism on the behalf of the code writer as well as others. Understand that no single developer will have the perfect solution to a problem, even when comparing juniors to seniors. I've been in this business for more than 20 years, and I still get surprised by fresh blood. If you aren't surprised, you aren't growing as an individual developer.
The WHY is to typically refer to some best practice, coding standards, something as neutral as possible.
I use the combination "believe" and "should" as it's friendly but still has some hint of authority. Another tip is to not just post negative review comments, I also comment on things that went well.
When I see somebody repeatedly make the same mistake, or very severe mistakes, I schedule a session to discuss the topic, as there's likely a knowledge gap or misunderstanding. Invest in people like that and make it clear that this improves life for both themselves and the reviewer. Win-win.
People can be very diverse, of course. Still, if you need to sugar coat feedback too much and are afraid of a possible backlash or drama, that's unhealthy. The exchange of good faith professional feedback is part of business.
‶This code is duplicated several times.″
—
2) State the change you'd like.
‶I'd suggest extracting it to a separate function.″
—
3) (Optional) Expand on why you suggest solving the issue with this solution instead of another one.
‶We could create a macro instead but it's not worth it as we can just let the compiler decide whether to inline the function or not.″
When collaborating, I like to provide my feedback in a way that allows the person to provide their own perspective.
"What do you think about moving this to a separate function so that we can keep each functions in the class small and focused on one specific purpose?"
When supervising you of course want to allow two-way-communication but you can be more direct, but make sure to use the correction as a teaching moment.
"Please move this block to a separate function so that we keep the functions in the class small and focused on their one specific purpose. See abc.js and xyz.js for good examples of classes that follow this pattern."
I usually use “we” instead of “you” in my code reviews, “we” feels more friendly and comforting and way less judgy.
I’ll usually link something like this page to the repo and align on the meanings of each phrase.
Tone is very hard to judge in written form particularly when there might be cultural and language divides, in my experience though it doesn't really matter what style you use provided that you also remember to comment on things that are good and do not need changing. This is routinely forgotten and more than balances any perception of negative tone the requester might inadvertently infer.
"These lines have been repeated a number of places and probably ought to be pulled into a function called something like xyz in module zyq"
Generally your tone doesn't matter as long as it's not mean. "This is the 10th time I've told you declare constants FOR EVERY MAGIC NUMBER," does not belong in a code review; it belongs in feedback.
However there is something a bit dangling here which is linting and static analysis. If code has been repeated and genuinely ought to be pulled into a function, static analysis ought to be able to find this.
If you have already dealt with the broad class of possible change requests that can be caught by static analysis, the nature of the reviews should get a lot more straight forward. It will stay at a higher level and naturally tend toward verbiage like "I wonder if we ..." or, "I was thinking if ..."
In cases where static analysis would not find it for e.g. in the middle of a function doing something, we have several lines of code collecting data for some analytics event which is never repeated, but still confusing; I would say something like "For readability, could we pull the analytics bit into a separate function?"
Generally; if you can use "we" instead of "you" or "I"; I would prefer "we" in any team setting.
0) Give feedback with respect and just like as if I were sitting with someone face to face telling them these things. And keep it about the code, not the author.
1) Remember that I could always be wrong (whether from misreading something, not having the whole pictures, etc. - many reasons my suggestion may not be the right one).
2) Communicate how strongly I feel about a change - I use `nit:` to indicate opinions or annoyances that are basically "I wouldn't do it this way, but up to you whether to change it", slightly stronger language for things I think require changes, up to "I feel strongly about this, let's chat about it" for things I would not approve the PR before they (or my mind) are changed.
3) Phrase as requests and/or suggestions (could, would, prefer) over command (do X)
4) As others have mentioned, give the reasoning. Personally, I prefer request first, reasoning second, but I see many people expressing preference for it the other way. My reasoning is that, while the reviewee may read the reasoning once, the request is what they may come back for when fixing things, so ensure it is the first thing seen so someone doesn't have to parse through the reasoning again to find the specific request.
5) Provide examples if it's clearer to communicate that way as compared to writing it out.
I tend to provide more open ended questions like "what do you think about ..." where "..." could be a few options to take. In my mind I've most likely picked one based on personal preference but I like proposing a few choices to spark some discussion amongst team mates and give everyone a chance to contribute. Often times throwing out a few options will result in someone coming up with another option that only became apparent after they've seen a few alternatives (which is a good thing).
It also lets more folks make decisions and that's a very important thing. If you keep making every decision then folks won't feel like they can contribute anything because "why bother". I remember an old TNG episode (S3E21) around this with "Broccoli" when he was trying to integrate on the engineering team but Jordi kept alienating him. Picard dropped some really good life advice in that episode!
This can both be company culture and origin culture of the developer you're communicating with.
For example, sometimes I write comments like "I don't particularly like this because" plus an explanation, plus "but I don't see any significant better way, so I'm fine with it for now". Some of my colleagues are fine with that, maybe they add a code comment like "TODO: find a way to fix problem X", or they just acknowledge and don't change anything. And then there are developers I work with that come from a different country, and they'll spend another half day or even two days coming up with a better solution, or don't dare to click on "resolve thread".
I think their culture is really geared towards avoiding verbal criticism, and if I even bother to write something negative (and maybe I'm also senior to them, in the org chart), it must be really meaningful and must be addressed.
So for some of these "foreign" developers I only leave comments where I expect something to change; for others that whose culture I better understand, I sometimes leave comments that are more conversational.
I could even be wrong. Maybe it's not an issue at all.
So I'll say "Could we move this out to a separate method? recountFooToilets seems pretty sepf contained and reusable" or just "I'd extract that method".
I don't want to say anything that implies I know better than you.
I'm pretty big on best practices, and a lot of people aren't, so when I find something like that I try to frame it as my problem, not their problem.
Especially with wheel reinvention, in-house implemented algorithms, use of low level primitives when the language provides high level things, lack of unit tests, or any other lone genius/cowboy coder/three star stuff.
I try to use the kind of tone Rust's compiler uses. "I don't have any reason to believe there's anything wrong here, but I don't know how to evaluate it, I don't have any experience with this style, it's above my skill level, and probably above some others too. Doing X is the tried and true standard, anyone can understand it, it will be easier to maintain".
Reads very passive aggressive. No.
> * Could you extract this to a separate function?
Yes, I could.
> * I would extract this to a separate function.
Good for you.
> * This could be extracted to a separate function.
This one's OK as a style suggestion that you won't block the merge over.
> * This should be extracted to a separate function.
This is kinda, sorta OK, if you're going to block the merge if this isn't fixed.
> * Extract this to a separate function.
This is better because the tone matches the intent: it's a command. This gets addressed, or no merge. "This should..." is indirect—you're not stating what you actually mean. This phrasing is better.
4 is the best of these for optional suggestions, 6 is best if you consider the fix a requirement for a merge. In general, say what you actually mean—dancing around it leaves room for miscommunication and can give all kinds of bad impressions. Of course, that can also mean weakening language: don't command a change that you don't consider a big enough deal to block the merge.
[EDIT] And, as others have noted in the thread, of course expanding with justifications is always a good idea. "Do this" without a "why" should be avoided. But lead with what you want done, not the why—that follows.
* Please extract this to a separate function.
And I think my favorite is
* Should we extract this to a separate function?
Asking the person for their opinion seems more productive than commanding them to do something. If you word it like a command, then you're basically stating that you know better than the author and the author has underperformed. That's my opinion at least.
1. Reviewing code is like proofreading a paper. Suggestions are welcome, but there isn't an expectation that all suggestions be followed. The author of the code makes the final decision based on their objectives.
2. A code review is a gate that is closed until all comments have been fully addressed. The author must comply to the liking of all who choose to comment, or they are not authorized to continue.
The tone of review comments will naturally follow from their understanding. Understanding #1 yields comments aimed at persuasion. (Should we, Could we, You might consider) While #2 yields more compulsory language (Change ..., You should). Or passive aggressive language that is meant to be compulsory but softened to sound less aggressive (I would, Could you, shouldn't you).
It is my opinion that understanding #1 is most appropriate. It prevents unnecessary bottlenecks and is more respectful or differing opinions. If developers are peers, than #1 is the understanding the most accurately reflects that.
It depends on the relationship with the person who's code you are reviewing, it depends on the relative seniority differences, it depends on the project, it depends on the rest of the team culture, it depends on the rest of the company culture, it depends on how long you've had a working relationship with this person, it depends on who else might see the review.
I've had working relationships with people where it was beneficial/easier to be super curt and to the point, because there was sufficient 2-sided trust that best interests were at heart. I've had working relationships where I had to take having "open-ended genuine curiosity" to the extreme because of fear of feelings being hurt.
That being said, by default I take a question-asking tone to the code review process, with the intention of possibly learning something. It opens up the door to "being wrong" and allowing things to progress without incessant arguing. There are times when this approach isn't appropriate, but I think it's a reasonable default. Again, it depends.
* Is the change mandatory for your approval? Then be polite and firm with reasoning - "This is poorly readable and doesn't conform to our code style chapter 5. Please extract this into a separate function.". Don't mess around with "Should, Could, I would" starters when it's not a question - be clear what you're asking.
* Are you trying to start a debate? Then ask a question properly - "I think this is unreadable because of A, B, C. Do you think extracting this will help readability?" Again, be clear it's a question.
* Are you saying an opinion and you're not very hung up on it? Mark it as such - I tend to put "Optional" or "Nit:" (team term) in those cases. "Nitpick/Optional: I think this isn't readable, if you have a moment maybe extract the method?"
The worst you can do is veil your intent (order/question/opinion) into faux questions which will confuse people - especially if they're non-native english speakers. Adding background / explanation / context for your comment always helps too.
I think the real problem is the we lack a framework that makes explicit:
* What is the goal of a review, according to the team? Spotting bugs? Prevent security flaws? Guarding the architecture? Making suggestions to grow better as programmers? These goals partially overlap, but are different. Are you aligned, as a team?
* Encode your priority in each comment. We could use some semantic prefix for a comment that categorizes it as, in increasing order: 1.suggestion to share views and alternative approaches, do as you please / 2. I think another approach would be better, suggest you use it / 3. I think it's very important to change something, let's discuss / 4. I'm not willing to approve or compromise, but feel free to get approval from someone else / 5. If anyone allows this to be merge, I will escalate.
Then still we should use good tone, but it's more explicit what to expect, now. Open communication is key to a psychologically safe environment. But I'm Dutch :-)
Unlike in any of your examples, the subtext is: "I anticipate you had reasons for doing it the way you did and I am open to listening to them." It engages the author on the same footing, leaves the door open for discussion but still communicates your intent in a straightforward manner.
- Try to make this not personal (opposite of how I started the paragraph above!). Talk about the code, not about what the author did. The code does this, the code does that, instead of you wrote this, you wrote that. Also, it's our code, so "we should do this" instead of "you should do that" in case you can't use "the code should do that".
- Be clear on what's just nice-to-have and what is really blocking the review process.
- Explain to them why you think this should be extracted to a separate function. Maybe they have a good reason against it or a good counter argument.
> Consider extracting this to a separate function to make this more re-usable.
This gives the developer the option to disregard the comment. Essentially I am trusting that they will consider it. It shows that I trust their judgment instead of ordering them.
For stuff that is more important but not critical I prefix with "I'd recommend".
I'd recommend doing [current approach] [migration statement] [your suggestion]. It will [benefit].
> I'd recommend testing UI components in a similar way that a user would interact with it. It will make your tests more resilient and prevent other developers from breaking your code.
> I'd recommend moving this logic into it's own function, it will allow this code to be used by future developers and allow you to test it easier.
This both declares the recommendation and explains why it was recommended.
For stuff that is critical, I might just write it more directly:
> We need to extract this into it's own function so it doesn't lead to/cause X.
> Should we extract this to a separate function?
When you are not familiar with the codebase, potential reuse, and want the submitter to clarify rationale for the approach.
> This should be extracted to a separate function.
When you are the authority but you're open to a discussion. You should include the "why" aspect in this situation.
> Extract this to a separate function.
When you are the authority and there will be no discussion. If you want you can include the "why" but it's a waste of time. You are the law.
The rest are sugar coated versions of the same thing.
"I think...", "I would...", "Maybe we...", are unnecessary additions. Clearly it's what you would do as you're the one giving the review.
If people are too thin-skinned to take direct feedback then they should work on improving that before submitting more code. Life is too short to waste time picking words that try not offend when you can spend that time actually producing something of value.
e.g. "Extract this to a separate function please" if you aren't asking. "Should this be a separate function" otherwise.
This also assumes that theres a certain protocol to PRs in terms of veto power. I've always followed the rule that the writer must make all requested updates by reviewer unless explicitly challenged. In other words, you can't silently not change something they mentioned, but you can say things like "thats out of scope for this update" or "I think this design is better because XYZ" and the writer is able to mark as resolved at that point.
Sometimes this ends up with a conversation that needs to happen where the reviewer disagrees with the writer, but honestly it rarely does at least on the teams I've been on. It would really have to be a contentious point to escalate to that level rather than just being a difference of opinion.
If they already self reviewed the code and it has problems, you can write a more detailed message explaining why there's a better way to do things. This way it doesn't come off negatively because the comment is a constructive suggestion. You may also have to compromise some lower priority feedback for development velocity as long as the submitter understands and can improve in the future.
Thus, if we're talking about refactoring a line-of-code then just writing the refactored line-of-code is probably the most clear and concise. If it's more involved than that, then it may be worth taking offline. In the example you've provided here, I'd suggest the best language is likely: Refactoring this block as a separate function provides the following benefits: 1. blah, 2. blah blah, and 3. blah blah blah and no side effects. The only additional clarity that's helpful is to say whether it's optional, required, or some other directive.
Actually an interview question to work with us. We need code, not egos. Write and be wrong, correct and continue. Do not sweat errors. None of us are perfect, and neither is this review.
1 - This / should / could etc etc make sense only if the code is duplicated. If it's long, better to make a sub-function and use those in main body of the function for better readability / maintenance.
2 - As for tones, you are the senior. Don't sugarcoat juniors, tell them the truth green in their faces. You'll be appreciated more in the future by them because you're helping them grow, not running for mayor office and you need to be PC. Also, don't be rude / asshole, they still need your help though.
3 - I found out, the best is to talk to them like you talk to your friends.
B - for seniors
No sub-points here, seniors who make blatant mistakes gets treated like juniors (see section A). Those who make honest mistakes, well I found out they only need you for rubberducking because mid-review they'll realize and correct / change course on the spot.
To a senior dev they might not need an explanation to justify the action of extracting the coded. It could just be oversight.
But to junior and intermediate you might need to explain the reasoning for requesting code to be abstracted.
There are also some great things in the book, Debugging Teams, like the Humility-Respect-Trust (HRT) concept.
You can soften that language by using phrasing like: "This function has grown long."
Explaining how to resolve a problem if the solution is obvious can be perceived as rude. Avoid. Don't tell other programmers how they have to solve a problem either - it's their job to figure that out - make suggestions instead.
If you want to make a suggestion, explain how it addresses the problem and if possible put an emphasis on improving things for the sake of others: "Extracting this to a separate function would make it easier to grasp."
Brief, helpful, and you're not appearing like you want to hold anyone's hand or micromanage them.
I would say though that a lot depends on team dynamic & hierarchy. For example, I have no direct reports, so I'm never going to use the imperative as in 'extract this to a separate function' - taken literally, I don't have the authority to tell anyone to do that.
Most often I think I favour 'I would', and give a reason. I.e. I found this a bit surprising, because I would have done it differently.
Along the same lines I also ask (honest) 'is there a particular reason' or 'why not' type questions. Something else seems most obvious to me, but maybe they thought of (even tried) and dismissed that.
You ask someone above you. You tell someone below you. You discuss with someone on the same level as you.
If you're smart, you'll quickly spot the assholes who think they're above others - you probably shouldn't comment on their code at all, other than 'great, looks good'.
If you think someone is below you, you're an asshole.
If you want to discuss, you need to include the word 'because'. 'Hey, can we do X, because I/we will need it for Y?' is the way I usually go about it. By the way, discussions are best had before anyone writes code, not after. So for me, I don't do code review - if someone's bad enough that they need their code reviewed on a regular basis, I will be leaving soon anyway.
All of your examples are wrong, because you don't state WHY you think it would be better. Never give instructions without at least a short explanation.
None of your examples include WHY you think it would be better to do something. I never give instructions myself without explaining at least briefly why I think my suggestion is better, because they more likely understand the problem or accept my suggestion.
Yes. This. Clarity is a prerequisite for tact.
Also, be consistent in your tone and style across reviews in a given project or org -- no matter who you're talking to. For instance, on one development project I was informed that all new tickets should use the language "shall" instead of "should" or "will." Consistent language gives everyone a common reference point and helps keep people from feeling "singled out."
Just my $0.02.
If you're fluent in English, there's a clear difference in the tone of the listed comment forms. If you're working with colleagues that may struggle a little with English, you can easily read to much into the tone of a code review. The reviewer may not mean to come of direct or even aggressive, but because they literally just translated directly from German or Finnish the tone will be way off.
It's also up to the author of the code to assume good faith, and look beyond the language used. Try to be kind when reviewing the code of others, and assume that the reviewers is trying to help.
Start with "Consider..." in case it‘s not a must.
If it‘s a must, start with explaining the consequences, e.g. "this value must be kept in a request scoped context or there will be race conditions and other concurrent misbehavior. Consider putting it in a @RequestScope bean."
We include this expectation in our working agreement, stick to it rigorously for reviews where reviewer and reviewee haven't established a strong rapport yet, and use it as appropriate beyond that. So far we've had no confusion about tone in reviews.
Either that or I ask for clarification of a piece of code: "Why did you do this instead of this?".
Like already stated by someone else; your approach really should depend on the team you're in. I'd take a less direct approach with junior devs who tend to be a bit more insecure (and stubborn).
Should this be extracted into a separate function?
As a code reviewer I may think it is obvious that it should be in a separate function or something else that seems obvious. But often times the author has already tried that and had issues with doing that. Thus they did it the way it is because of that.
“Instead of doing A here, let us instead use a B approach, otherwise there is a risk of C happening.”
And for shit: “This isn’t great because X. A refactor needs to take Y into consideration, which will result in Z-quality code.”
It’s all case by case, though, in reality. If I know someone we’ll, I might ask why they went with a certain style or a particular algorithm and just talk them through to grokking the error. If they’re somebody who is newer to the field, I usually point them towards a linter, especially if what I’m reviewing would’ve been flagged by said linter.
Stereotypical corporate Americans of course would use the first two, but in Australia using language that tiptoes around peoples' feelings too much (e.g. making sure to point out positives even when someone clearly fucked up) can make it look like you don't trust them or think they're incompetent.
The only other time we'd hear that level of "compassion" is when we have way too much to drink and are acting like a knob.
I've heard Europeans make similar comments about American corporate norms confusing things.
Once I have worked with someone for a while (and perhaps have gone out for beers / coffee a few times), I write shorter comments.
At that point, they've usually figured out that I mean well, but am blunt.
For new hires, I spend a lot more time spent on "why" and tone.
If my team set guidelines for this, that'd be a strong signal of much, much deeper dysfunction and I'd consider switching jobs.
I don't think there are simple answers, and it's something you'll have to figure out separately for every reviewee, and sometimes every review.
You have to consider power dynamics, preexisting relationships, your own time, the importance of the change, and cultural differences.
If the power dynamics are too far apart, the right answer is often "don't". A high-level engineer or architect reviewing a relatively junior coder's patch should not be commenting on anything involving taste or stylistic suggestions. They should be reviewing for functionality, security, compatibility, etc. And only bring up maintainability or performance when it really makes a difference.
Some suboptimal code will get landed as a result, but it's better than creating a culture where the senior's personal preferences are always going to win out. When there's a power imbalance, nuance gets lost—the junior either has to bend to the senior's preferences even when they have a good reason to do things differently, or they have to go to a lot of effort to justify their position. Which is fine in some cases, but it's a waste of time and effort for things that don't matter as much. (This isn't about avoiding hurting people's feelings, by the way, though it does serve that purpose as well.)
I also agree that pointing out positive qualities of a patch can make up for quite a few critical comments. Explicitly constructive criticism (eg sketching out an alternative, or doing some work to provide data or justification for a comment) can also "buy" a smaller amount of more negative-sounding comments.
And of course, if you're swapping patches with someone regularly, there's no need to overthink it: state the problems you see, the things you're not sure about that concern you, the suggestions you have, etc., using as clear and concise language as possible.
In all cases, do not make the reader guess your actual opinion or intention. If you're couching a criticism in gentler language to avoid making them feel bad, that's fine, but don't play games or leave out important pieces. There's always a way to coach or criticize honestly, and you'll do yourself a lot of good by finding it.
Will we re-use it? Do you just not like blocks of code bigger than X (been given this reason many times)? Do you just feel the need to comment (a lot of devs do and I appreciate it when they are upfront about that rationale and I have done this myself to show that I am paying attention)? Are you concerned about readability?
I need to know for future code reviews, so I can avoid the issue in the future.
> have you considered x? It is standard practice in y to do z.
> should we do x instead? I think it would be better in the long run because y.
If there is a more serious issue, I prefer DMing the author and hashing things out synchronously.
I think it is important to phrase things gently as much as possible, as tone is hard to read from text, and being overly harsh might discourage other teammates from taking risks and asking for feedback.
I’d prefer something like this: “I think we should extract this logic/code/etc into a separate function for better testing, less code duplication, and better maintainability.”
You only have to say that a few times before you can use the short hand: “Extract this into a function.” because you stated reasons before.
I try to give critiques in this way, as it gives the person that wrote the code ample opportunity to defend their approach, but also gives explanation of why your suggestion might be better.
But I see code review as a conversation. Quite often it calls for a bit more discussion, and, depending on how the team deals with this, I sometimes reach out to the developer whose code I reviewed over a separate channel to discuss the issue a bit more.
“What might happen if the input is a null value?”
Sometimes they realize things on their own after one or two questions. Sometimes many. But it’s always so neutral and keeps them in the driver’s seat.
* "We're repeating this in a few places. Could you extract this to a separate function? It'll keep the codebase smaller and more readable, and if we update it, there's only one place we need to change."
Don't force a compliment for no reason because it will come across as insincere... but if you see something well done/implemented, don't be afraid to call it out.
It helps balance the tone sometimes, and is especially helpful with junior engineers who are well served not just learning what's wrong, but what's right.
Depending on whether their cultural background is North American, Scandinavian, German, French, Italian or Chinese (to pick from the very situations I have faced), and especially if they are more junior, you cannot expect the same outcome from the same feedback, without additional context/care in the communication.
If not, then only the first version is acceptable.
All the "should" and "considers" are tickets that can go on the backlog after a merge has delivered functionality.
Then YAGNI will probably turn out to apply.
Error: `< n` is an off by one error, the last element won't be processed
Warn: Consider dropping the loop altogether. We know there are always eight elements.
Info: Nice name!
"approved, but I think this should be in a separate function"
This was more important back in the day but now linters and formatters are mainstream.
The best time to code-review is at the beginning.
“What would be the advantage of using X instead?” If I’m not entirely sure or fine either way.
If I think X is really the way to go I’ll say something like:
“Doing X would have these benefits. The benefit of Y is such and such. So X is preferred for …”
1. Try to find something positive to say about the code. This reinforces what is being done well and supports the notion that everyone is on the same team chasing the same goal.
2. I like to go deep into the "why", being as objective and fact-based as I can. Code style and "best practices" are sometimes confused with personal opinions and preferences. I avoid words like "I prefer". If I can't back up my opinion with strong reason-based arguments for why my opinion is objectively better then I keep it to myself and continue to ask myself if it's a personal preference or if there's a good reason for favouring that.
3. Tone must always be friendly, polite and constructive. Of your options I often go "I would extract this to a separate function because ..." and then I go into the "why"
4. Sometimes I have a lot of supporting information to explain a position. I will often label these "Academic Digressions" and then drop a TL;DR. Sometimes people don't have time to read them right away but I'm often told that people appreciate them and save them for later.
It's remarkable how this change of perspective improves one's clarity on which code polishes aren't actually worth the opportunity cost.
I also like to often prefix my suggestion with: "It might have missed something, but X", especially if it's a larger change.
"Maybe it would be better…"
"IMO we should do …, what do you think?"
You really want to be on this person side and help them achieve. That's your job after all
I try to do it in person or (remotely) send a private slack message.
People can be sensitive to negative feedback and one way of ameliorating that is not to give it in a public forum where everybody can see it.
* Why isn’t this extracted to a separate function?
Any time you can convincingly explain exactly why you're suggesting something it makes the suggestion itself seem much more reasonable, and it's a valuable check on your own reasoning. From there, you can reword the explanation itself in creative and valuable ways to give feedback. If you get really good at this, then a lot of times you don't even need to ask for a specific change, you can just turn the problem you're seeing (I.e. 'explanation' from above) into a question. I'll give an example.
Let's say you want to say "Extract this to a separate function". Ok, now, ask yourself why? So you tell yourself, "Because we may want to use part of this function again, but not the rest of it, and the exact same behavior can be achieved with little to no additional effort by function composition (or whatever you do in your PL)" Ok, that seems reasonable, but maybe now it may be perceived as a little too abstract to someone else, especially if they're just trying to get their work done. But you can take solve that by taking the above 'explanation' and make it concrete. One good way to do this is to think of a counter example, where it clearly doesn't work how we want.
As an example, let's just pretend that they're doing something like parsing a thing from a database into some strongly typed thing, and then doing foo work on the strongly typed thing. You can ask yourself then, well if we were unit testing this, we won't care about the parsing part, we can just start with the strongly typed thing as the precondition of our test, and then isolate what we're testing to just be the foo functionality. Now you have a simple example that you can rephrase into your original feedback.
This thought exercise all leads to the feedback going from "extract this to a separate function" to "Does this mean that unit testing the foo feature requires a parsing step for each test case?"
Maybe that's a silly, contrived example, but I've found that if you even capable of going through this thought process then not only do people receive your feedback much better, but also you just tend to think of much better feedback altogether. After a lot of practice you end up being able to think this way very quickly and then you can focus on adjusting the wording based on your relationship with that person.
At the end of the day, it's not what we say that really matters, it's how someone feels they're being regarded by one their peers. If they sense contempt, then it won't matter how politely or expertly you word things. Having extremely well thought out why's in your back pocket, and being able to provide concrete counter examples just makes it clear you're not wielding your review power just to talk but to illuminate the problem to them. It also solves the problem of when you're the idiot (as happens to all of us from time to time), by forcing you to actually justify your feedback before you share it.
When I've done this, I've noticed that my reviews become faster, and I focus on stuff that people don't think about. It makes the code better and then people will actually seek out your review.
We don't have any junior devs at my current place so I'm reviewing code from devs with similar experience as myself, therefore I ask more questions. "Could this be improved by doing x?", "Was there a reason you did x over y?", etc. Then when I'm done with the review I normally send a message to the reviewee just letting them know I'm done and happy to discuss if they have any questions or thoughts. Most of the time I find there is some reason why decisions were made and we can discuss that if we need to. Alternatively they'll agree with my suggestion and just go ahead with making the changes.
The reason I like questions is because I'm not perfect and the person who wrote the code spent a lot more time thinking about it than I have. As a reviewer of another senior dev's code I see myself as a fresh set of eyes, not as someone with more ability or authority.
Some obvious things like typos or formatting issues I'll just note without a question. "Typo here", etc. There's really no discussion to be had in those cases. What I wouldn't do is say something like, "Typo here. Fix this please". The "fix this" is unnecessary and comes across as a bit rude imo. I know when my code is being reviewed I just want to be shown what I've done wrong / what I can improve. I don't want orders. If I've made a mistake I'll fix it and I trust others will do the same. I think that's generally the kind of mutual respect experienced devs expect from each other.
With junior devs there's room to be more authoritative, although tone is important. When I'm working with a junior dev and I think they've gone about something the wrong way I prefer to just have a chat with them and explain how I think they could have done something better. I think it's good to make it clear that you're not trying to be difficult, but you know there's a better way to do something and you want to help guide them. I know when I've been a junior dev on projects I've received some overly critical and demoralising code reviews. Realising the thing you've spent the last week thinking about and working on is wrong isn't a fun experience as it is, but a review with an overly critical and unconstructive tone can make it even worse.
That's just my thoughts anyway. What's right in your case likely to depend on the context of whos code you're reviewing and what you're reviewing. The only constants I'd suggest are in tone. If you're not being perceived as being respectfully constructive in your code reviews you're probably doing something wrong.
There are two axes I think about here when deciding on tone. Why am I reviewing this code? And why did this person write the code?
For the me axis, at one extreme I'm paid to review this code, and I designed or co-designed the software that's being modified in the reviewed change. Architectural decisions were mine. At the other extreme, I'm just another volunteer, this system I'm presumably interested and somewhat knowledgeable about (otherwise why review code for it?) but I may not even be an expert, and certainly can't be said to "own" it in any way.
For the other person axis, at one extreme they are assigned to work on my project, this is their job, at the other extreme it's some volunteer who has worked with this ten years longer than me and maybe designed it and it's good of them to ask me to review it, they could just push commit.
#6 "Extract this to a separate function" is a command, so it's not appropriate unless either they're an employee and I'm supposed to be mentoring them, or they sent unsolicited contributions to some project where I'm an expert and frankly I'd rather not have their contribution than take it as it stands. If it's a project that invites contributions (even very informally) then this is always the wrong tone.
#1 "Should we extract this to a separate function?" is a question, and so it's not appropriate if I am expected to provide guidance (e.g. in mentoring or when reviewing code from a brand new team member) but absolutely appropriate if I'm reviewing code by somebody who knows the project much better.
#3 "I would extract this as a separate function" is always appropriate if you are sure you would do that work. However, if you are sure you would do that work, consider just doing it instead. In the scenario where it's a junior employee being mentored maybe there's pedagogic value in them extracting it so write either #5 or #6 depending on whether they seem to need the explicit instruction or whether it's implied in the usual workings of your environment. In the scenario where you're a new volunteer maybe the author has a good reason it should not be extracted and so it's worth asking. But in the middle case why aren't you doing it? If it's just not worth your effort the same is true for the author.
#4 "This could be extracted to a separate function" is redundant and so is #2 "Could you extract this to a separate function" the answers are "duh" and "hopefully you can do so or else you should learn this language". So overall I think I see three tones I would use (#1 #5 and #6), but in quite different circumstances, two tones I think are too vague/ redundant (#2 and #4), and one tone which is probably never useful (#3) because it suggests I should do work instead, in which case why not just do the work and say you did it "also I extracted this as a separate function, [link]..."?
If there are blockers in a PR, I'll add reasoning for why it is a problem (e.g. "this exposes us to vulnerability X", "in this edge case, what would happen is that ...", "I tested it and appears that there is a bug", etc.) If it's a team-agreed style issue (that isn't caught by the linter), I'd rather say "we use convention so and so in this code base" than "please change this to this style".
The reasoning is that I feel that I work with professionals and while they may need feedback, they don't really need commands. I can make suggestions as to how they can fix the issue I raised, but ultimately it's up to them to fix it and if they find an even better way that I didn't think of, why should I limit their problem solving capabilities artificially?
I find that most people I've worked with use a similar style (more centered around the code than around power dynamics), and those people who used a lot of "please do X" style comments were usually also pretty defensive about their code or ideas, would get uneasy when you changed some of their code ("why did you refactor this? please change it back") etc.
Now there may be some people (inexperienced, or just idiots) who really need some more forceful language, but you shouldn't necessarily start out on that default assumption.
That covers the blocker-style comments. Everything else should probably be done in either a "I'm opening up a discussion here and would be interested in your opinion" or a "this is just a nitpick, feel free to ignore" kind of style.
---
For the specific example, depending on the reasoning for why you might want the change:
- "The linter is complaining that this function is too long and we typically stick to those rules. I think lines 20-30 could be extracted into a separate function, for example."
- "I found it a bit hard to understand this function initially because it does a number of things at once (such as...). Maybe we could split it up?"
- "This section of code is repeated between here and that other function and I think we should keep the implementations in sync, so I would recommend extracting that common logic."
- "We've actually had to do something similar in other situations (see here and here) and your solution seems better than what we came up with before, so what do you think about extracting this functionality into a function and calling it from all those places?"
and so on (and it doesn't need to be as verbose if you know that you have more shared context, for example).
Could we please
Unless you're asking a question, posing it as a question comes off as passive-aggressive, even if it's not what you intended. A simple declarative statement of, "I would do it this way", is probably best. You don't know why he did it the other way, and he might have had a very good reason (for example, to be sure the subroutine would be inlined).
The best tone to use is one that is neutral and efficient; prefer active voice but avoid imperative mood unless you're 100% sure you're right, in which case it's usually fine.
It probably goes without saying, but you should assume the change was made in good faith. It's possible that the other programmer hasn't seen before what you consider to be good practices; it's also possible that she's great at her job but just made a mistake. It's also possible that you're wrong. So, for this reason, it's always best to be conservative with tone and stay as factual as you can.
This looks so unproductive and unprofessional to care too much about the tone vs content in programming business.
1.) Quit doing PRs for feedback. Start doing pair or mob programming. Bonus points if you ditch PRs completely and do https://trunkbaseddevelopment.com/ instead while you're at it. Asynchronous code review via PRs is waste (in the Lean sense). 2.) If you still want PRs despite pairing & mobbing (or you're about to tell me you've never done it, you're team won't and/or whatever insert lame excuse here as to slam/dismiss it without trying), spend a faster 5 min via screen share and audio AND CAMERA! Do the PR like an old skool code review session live/remote + camera, but have the author ANNOTATE for their own notes what you've reviewed when they go back to revise. 80% of human communication is non-verbal - interact w/ voice & camera. Help them learn & build confidence, then you won't worry about your writing tone. 3.)