Ask HN: What tone to use in code review suggestions?
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?
308 comments
[ 2.6 ms ] story [ 402 ms ] threadNot #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.
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.
Person is already invested in his code by PR review time and doesn't really want to do PR ping pong for their task. What if you come back with more comments after they address your comments. Will they ever get to finish their task or forever be hostage to you subjective comments.
Most of it is usually subjective too. you had to explicitly mention 'security issues' which imply that you have a feeling that most of your pr comments were your subjective interpretation .
People are more open to suggestions during pair programming because people don't deliver feedback in cutting way ( with or without coffee) and its easy for get a common understanding and move forward.
That was something we discussed at various points. We never tried it when I worked there.
> you had to explicitly mention 'security issues' which imply that you have a feeling that most of your pr comments were your subjective interpretation .
Aren't almost all code review comments? I can't think of a single one except ones that point out literal bugs. Most are "do it this way because I think it will be more maintainable" or "do it this way because that is how we do it here." In this case, I mentioned "security issues" because they were literal bugs that introduced new attack surfaces that didn't previously exist. When I read comments on my code, I make sure to read it without any inflection. Some people have really bizarre code review styles, and some are totally straightforward. Some people are lenient on whether or not there actually needs to be a change, while some people will not accept code until changes are made, no matter how little the change request is.
Knowing your reviewer is about as important as how you review code, which is why I suggested being consistent. If you're always straightforward, and then suddenly not, the reviewee is going to wonder if something is going on. Vice-versa, if you're suddenly straightforward, people are going to be offended or feel like you are taking something out on them. In my case, reviewing code within 3 minutes of waking up was a terrible idea and I have never done it since because I'm "grumpy-mc-grumpy-pants" for at least 10-15 minutes after waking up and having some coffee.
I feel like a lot of the conversation in this thread is about how to review code, but the relationship between the reviewee and the reviewer is more important than any of that and how you review code will affect that relationship whether you want it to or not.
yes I agree with this 100%. I've gotten nasty reviews from people i like and it didn't bother me at all.
I have a co-worker who's socially retarded as well and I can say that the vast majority of his comments that tend to rub people the wrong way are just badly phrased versions of legitimate opinions (that he sometimes should just keep to himself because no one asked). He's a better reviewer than you seem to be, though; probably because he can take the time to type out better versions of things he'd normally just blurt out.
Edited to add:
It's a massive chore to have a teammate that causes stuff like this and I don't envy your teammates or lead (even worse if you're the lead, obviously).
> I'd hope that I've grown quite a bit
I hope so too, because "I hadn't had my coffee yet har har" is a pretty thin excuse.
I'm pretty blunt/rude in the mornings, within half an hour of waking up, even these days. Back then, I did not know that about myself -- or rather, just how rude I came across.
Most people I interact with never see me like that, but it generally takes me 10-15 minutes to get some coffee going. Hence why I said "before coffee" since I assume nearly everyone is a little weird within the first 30 minutes of waking up, which also tends to be before a morning coffee.
People who wake up "ready to go" seem to be pretty rare. Granted, my number of people I've had the opportunity to be around when they wake up is only a few hundred people (basic training, etc).
Additionally, if you made someone cry I think most of us draw comparisons to similar situations we've seen where, generally, the person who thinks they're just being blunt is actually very offensive and usually the most sensitive to critique.
However, I think it’s worth pointing out, that at no point in the mentioned code review was I mean, or disrespectful. I was just direct vs. my usual indirect self. I never said anything like “this is stupid” or anything personal. I didn’t have any malicious intent nor did it come off that way, unless you had gotten a review by me before and knew how I usually code review:
“I think this would be better expressed as X for performance, wdyt?”
became:
“Express this as X for better performance”
I didn’t use my lack of being awake as an excuse at work. I apologized and just said I didn’t realize how wording things differently would affect my coworker and it truly did upset me as well. It probably wasn’t until I worked with someone who always reviewed like that did I realize how annoying that kind of wording actually was, but that wasn’t until years later. Anyway, it was a weird day, because I wasn’t intentional in my wording and I paid for it. It’s especially weird when you know exactly what happened and you want to explain it so that maybe the other person feels better about the situation but doing so would just come across as being an ass or just trying to make an excuse. It’s a terrible feeling when it’s a real issue in your own life, but other people don’t have a basis to relate. Further, it a terrible feeling to see someone suffering from it and all you can do is feel shame, and so sorry for the person you’ve said something bad too without even being aware of saying it, or unable to stop yourself from saying it.
So yeah, I don’t think it’s ok, but I’ve accepted it at this point in my life. Whatever bs comes out of my mouth, I accept responsibility for that bs.
A) Your co-worker is more sensitive than anyone I've ever met B) You are expressing yourself much harsher than you think you are
Again, I don't know you but pattern-matching based on my experiences it's almost always B.
I know this is a complete tangent, but do you find this to be true specifically of habitual coffee drinkers? I have a pretty horrid history with a sleep disorder, and while I'm a little slow after waking up, I don't think my mood is especially worse. I wonder if it's simply because I don't have a stimulant addiction like most people do.
I used this term all the time growing up and I'm trying to stop because it's definitely offensive to some people. Have a good day.
It gets tricky. I haven't had to deal with this friction for many years, but it's not because I entirely avoid the type of directness described in OP's last two examples. It's because I'm careful about who I'm direct with, and because I've ended up in environments that are increasingly devoid of the emotionally-fragile.
I start out with a baseline of assuming that people are as fragile and childish as what your comment alludes to. In many companies, I'm sure this is actually the case. But as I get to know a coworker, it's very often the case that they have the talent, maturity, and emotional stability to handle direct communication (in both directions) without spiraling into an episode.
In my early career, I joined a company with an awful hiring pipeline as the first employee. That was my first introduction to the idea that a lot of people react violently to being treated with respect if they think they don't deserve it. I quickly started treating them like children as you suggest, and the problem immediately vanished.
But as I've progressed in my career, the amount of time I spend around mediocrities has plummeted, and my prior that the person I'm communicating with is a mental adult has risen. I've found that high-productivity professional contexts weed out the emotionally-unstable in the same way they weed out (eg) the disorganized; the value of good-faith efficient communication without mental breakdowns is simply too high.
"You wrote the code, and you seem to be unable to admit that your code was buggy. It's not a compiler bug. It's your bug. Stand up like a man, instead of trying to flail around and blame anything else but yourself."
I've seen modern art - it's not working.
I think software developers would do well to take such a course.
Was this developer generally unprofessionally sensitive? What happened to your manager?
I had a similar overly-sensitive colleague. (She was support, and would get flustered at my short responses like "please file a ticket" or "logs.")
She left the day after I met with her and explained common sense. (Customers expect you to know how to use the product; don't bypass the ticketing system by emailing me directly; follow the documented process to escalate to engineering; don't just copy and paste customer emails without doing your own investigation...)
I was having a lot of trouble with getting my manager to intervene with her, so in this case her departure was a godsend. We ended up firing my manager a little later, and things ran so smoothly afterwards that I realized he was the real source of the problems.
I usually use “we” instead of “you” in my code reviews, “we” feels more friendly and comforting and way less judgy.
Unlike in any of your examples, the subtext is: "I anticipate you had reasons for doing it the way you did and I am open to listening to them." It engages the author on the same footing, leaves the door open for discussion but still communicates your intent in a straightforward manner.
I'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.
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/
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.
Conventional commits: https://www.conventionalcommits.org/en/v1.0.0/
This looks so unproductive and unprofessional to care too much about the tone vs content in programming business.
I've seen junior devs from other countries get into trouble because they ignored these kind of "questions" since they actually thought they were optional questions and not expectations by other developers.
I put a clear line between social and work interactions.
And from my experience just being direct (not intentionally rude of course!) in work environment pays off dramatically versus wasting time trying not to hurt someone’s feelings.
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."
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.
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.
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."
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.
- you have an opinion on how things ought to be done and want a dialog
- you see some code that's wrong or violates an agreed-upon rule and so it should be changed with no discussion
I'd switch the tone based on what you're addressing. Giving a rationale when you share an opinion or point out a mistake also softens the tone (for the better IMO).
>* Should we extract this to a separate function?
>* Could you extract this to a separate function?
These are essentially the same: opening a dialog over an opinion. I'd suggest this when there's not an obvious flaw or rule violation or you have a gut-feeling about how code should be and want the author's input.
>* I would extract this to a separate function.
This is almost a command but isn't clearly one. It should be followed by some rationale, at least.
>* This could be extracted to a separate function.
This one's the least useful. You could do a lot of things with code. It doesn't resemble the command or inviting tones of the other examples here.
>* This should be extracted to a separate function.
>* Extract this to a separate function.
These are commands and are practically the same tone and best when catching mistakes. Unless obvious, a rationale should be given like a demonstrable flaw in logic or inconsistent abstraction, etc. There should be a few sentences explaining this. If there isn't a clear violation, I'd prefer the dialog invitation tones.
At least I try to remember back to when I was a junior, and how "personal" the feedback felt when I was just starting. So when reviewing for others in the same situation I make extra sure it doesn't come off the wrong way by adjusting my tone.
But for someone I've worked with for years we're often a bit shorter and to the point, and everyone's happy.
If someone disagrees I expect them to speak up, if they’re not so inclined I’m interested to know why, so we can address/resolve that and move on.
I was junior for most of that time and I think "use a totally different approach" or "change these 10 things" can sting a lot less that way. Your mistakes aren't all in black and white for the world to see, people can tell when you get something and don't end up patronising you, but you can also blur the lines a bit between "hey, what do you think of this, not sure" and "I think this code is 100% ready for merge and officially request permission to do so" - like, I know there are going to be comments, pretending otherwise will just make it worse.
Most of the time you'd get a review immediately too, less complicated structure then so one approval was usually enough, and it made sense for that to be high priority in my mind - this task is one step away from being done, I don't mind being interrupted.
The only thing is when I was the reviewer, it maybe went a bit too far one-sided learning experience, sometimes comments would get brushed under the carpet without much explanation/there was an assumption I'd approve.
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.
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.
> 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.