Ask HN: What tone to use in code review suggestions?
308 comments
I explicitly prefix Error: when I am requiring a change, Warn: if I am suggesting a change, Info: if I have an unrelated comment
Error: `< n` is an off by one error, the last element won't be processed
Warn: Consider dropping the loop altogether. We know there are always eight elements.
Info: Nice name!
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!
Neither of the first two nor anything else ending in a question mark unless you're really asking a question.
Not #3 because you're still being coy.
I'd suggest #4 or #5 when dealing with peers but they aren't interchangeable, they mean different things.
Save #6 for when you're giving tons of feedback to a junior.
If you're worried about being perceived as harsh or unfriendly, remember you can still be super friendly in the chatter on either side of the review session.
Not #3 because you're still being coy.
I'd suggest #4 or #5 when dealing with peers but they aren't interchangeable, they mean different things.
Save #6 for when you're giving tons of feedback to a junior.
If you're worried about being perceived as harsh or unfriendly, remember you can still be super friendly in the chatter on either side of the review session.
I feel like this is often overlooked.
One morning, I did a code review before having any coffee. I basically used the latter two throughout the whole thing. I came into the office to find my coworker literally crying. Later in the day, someone else said it was the best code review they’d ever read… and asked me to come to their team…
There was so much drama from that code review. That code got merged as-is to appease the author without a single one of my statements addressed (including security considerations), and no one else would review it. My manager was in a tight spot, felt sorry for him.
I was literally just my pre-coffee blunt self. I usually use the first few versions in your list, which is probably why it hit my coworker so hard. We chatted and made up, but our relationship was weird after that.
So, I’d say, just be consistent. Changing tones unexpectedly might affect your coworkers and politics in interesting ways that you might not expect.
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.
Yeah, I think code reviews strike deep for whatever reason. I don't think being blunt works here even if you can be blunt elsewhere.
You sound like a Dutch person. Working with different cultures I've had to adapt to varying levels of directness but like to think it's helped me become more up-front. I've learned to appreciate people that say what they're thinking, so you can avoid the 2nd and 3rd order analysis about intent and thoughts about how other people will receive things.
I found the Dutch directness a breath of fresh air when working in NL.
Is it ironic I moved to the Netherlands and feel right at home here? (this story is from when I lived in the US)
Is this person working on his own. Why can't people pair and give each other at the moment feedback vs doing code review.
Person is already invested in his code by PR review time and doesn't really want to do PR ping pong for their task. What if you come back with more comments after they address your comments. Will they ever get to finish their task or forever be hostage to you subjective comments.
Most of it is usually subjective too. you had to explicitly mention 'security issues' which imply that you have a feeling that most of your pr comments were your subjective interpretation .
People are more open to suggestions during pair programming because people don't deliver feedback in cutting way ( with or without coffee) and its easy for get a common understanding and move forward.
Person is already invested in his code by PR review time and doesn't really want to do PR ping pong for their task. What if you come back with more comments after they address your comments. Will they ever get to finish their task or forever be hostage to you subjective comments.
Most of it is usually subjective too. you had to explicitly mention 'security issues' which imply that you have a feeling that most of your pr comments were your subjective interpretation .
People are more open to suggestions during pair programming because people don't deliver feedback in cutting way ( with or without coffee) and its easy for get a common understanding and move forward.
> Why can't people pair and give each other at the moment feedback vs doing code review.
That was something we discussed at various points. We never tried it when I worked there.
> you had to explicitly mention 'security issues' which imply that you have a feeling that most of your pr comments were your subjective interpretation .
Aren't almost all code review comments? I can't think of a single one except ones that point out literal bugs. Most are "do it this way because I think it will be more maintainable" or "do it this way because that is how we do it here." In this case, I mentioned "security issues" because they were literal bugs that introduced new attack surfaces that didn't previously exist. When I read comments on my code, I make sure to read it without any inflection. Some people have really bizarre code review styles, and some are totally straightforward. Some people are lenient on whether or not there actually needs to be a change, while some people will not accept code until changes are made, no matter how little the change request is.
Knowing your reviewer is about as important as how you review code, which is why I suggested being consistent. If you're always straightforward, and then suddenly not, the reviewee is going to wonder if something is going on. Vice-versa, if you're suddenly straightforward, people are going to be offended or feel like you are taking something out on them. In my case, reviewing code within 3 minutes of waking up was a terrible idea and I have never done it since because I'm "grumpy-mc-grumpy-pants" for at least 10-15 minutes after waking up and having some coffee.
I feel like a lot of the conversation in this thread is about how to review code, but the relationship between the reviewee and the reviewer is more important than any of that and how you review code will affect that relationship whether you want it to or not.
That was something we discussed at various points. We never tried it when I worked there.
> you had to explicitly mention 'security issues' which imply that you have a feeling that most of your pr comments were your subjective interpretation .
Aren't almost all code review comments? I can't think of a single one except ones that point out literal bugs. Most are "do it this way because I think it will be more maintainable" or "do it this way because that is how we do it here." In this case, I mentioned "security issues" because they were literal bugs that introduced new attack surfaces that didn't previously exist. When I read comments on my code, I make sure to read it without any inflection. Some people have really bizarre code review styles, and some are totally straightforward. Some people are lenient on whether or not there actually needs to be a change, while some people will not accept code until changes are made, no matter how little the change request is.
Knowing your reviewer is about as important as how you review code, which is why I suggested being consistent. If you're always straightforward, and then suddenly not, the reviewee is going to wonder if something is going on. Vice-versa, if you're suddenly straightforward, people are going to be offended or feel like you are taking something out on them. In my case, reviewing code within 3 minutes of waking up was a terrible idea and I have never done it since because I'm "grumpy-mc-grumpy-pants" for at least 10-15 minutes after waking up and having some coffee.
I feel like a lot of the conversation in this thread is about how to review code, but the relationship between the reviewee and the reviewer is more important than any of that and how you review code will affect that relationship whether you want it to or not.
> but the relationship between the reviewee and the reviewer is more important than any of that and how you review code will affect that relationship whether you want it to or not.
yes I agree with this 100%. I've gotten nasty reviews from people i like and it didn't bother me at all.
yes I agree with this 100%. I've gotten nasty reviews from people i like and it didn't bother me at all.
> I was literally just my pre-coffee blunt self.
I have a co-worker who's socially retarded as well and I can say that the vast majority of his comments that tend to rub people the wrong way are just badly phrased versions of legitimate opinions (that he sometimes should just keep to himself because no one asked). He's a better reviewer than you seem to be, though; probably because he can take the time to type out better versions of things he'd normally just blurt out.
Edited to add:
It's a massive chore to have a teammate that causes stuff like this and I don't envy your teammates or lead (even worse if you're the lead, obviously).
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).
Considering this story is over a decade old, and I was in my late teens ... I'd hope that I've grown quite a bit even though you're using the present tense like you know me.
The way you told the story it comes off as much more recent.
> I'd hope that I've grown quite a bit
I hope so too, because "I hadn't had my coffee yet har har" is a pretty thin excuse.
> I'd hope that I've grown quite a bit
I hope so too, because "I hadn't had my coffee yet har har" is a pretty thin excuse.
> because "I hadn't had my coffee yet har har" is a pretty thin excuse.
I'm pretty blunt/rude in the mornings, within half an hour of waking up, even these days. Back then, I did not know that about myself -- or rather, just how rude I came across.
Most people I interact with never see me like that, but it generally takes me 10-15 minutes to get some coffee going. Hence why I said "before coffee" since I assume nearly everyone is a little weird within the first 30 minutes of waking up, which also tends to be before a morning coffee.
People who wake up "ready to go" seem to be pretty rare. Granted, my number of people I've had the opportunity to be around when they wake up is only a few hundred people (basic training, etc).
I'm pretty blunt/rude in the mornings, within half an hour of waking up, even these days. Back then, I did not know that about myself -- or rather, just how rude I came across.
Most people I interact with never see me like that, but it generally takes me 10-15 minutes to get some coffee going. Hence why I said "before coffee" since I assume nearly everyone is a little weird within the first 30 minutes of waking up, which also tends to be before a morning coffee.
People who wake up "ready to go" seem to be pretty rare. Granted, my number of people I've had the opportunity to be around when they wake up is only a few hundred people (basic training, etc).
I think you're receiving pushback in the comments because it comes off as though you think it's OK to be rude in the morning.
Additionally, if you made someone cry I think most of us draw comparisons to similar situations we've seen where, generally, the person who thinks they're just being blunt is actually very offensive and usually the most sensitive to critique.
Additionally, if you made someone cry I think most of us draw comparisons to similar situations we've seen where, generally, the person who thinks they're just being blunt is actually very offensive and usually the most sensitive to critique.
I don’t think it’s “ok” per se, but it’s something I’ve had to live with, as well as my family. I can honestly say that it’s not something I do intentionally, it’s as though my “social filter” is booting up and I just say the absolute first thing that pops into my head, with the wrong inflection to top it off. I’ve said some pretty fucked up things in the first 30 seconds of waking up. I own them, apologize for them, try to explain the situation so we both can avoid it in the future, etc. I don’t think it’s ok, and I just try to avoid speaking at all. Like I said somewhere in this thread, I didn’t know how abnormal when this story happened. If it makes any difference, I don’t sleep like normal people. Apparently, when I sleep, I become totally unconscious and nothing will wake me. Alarm clocks don’t work, people don’t work, explosions don’t work, gunfire doesn’t work, etc. It was an issue in the military at first and I had to learn how to work with it. So, when I wake up, I’m coming from basically being “brain-dead” to full alert. It literally feels exactly the same as being knocked out and coming back in a fight. That might have something to do with it.
However, I think it’s worth pointing out, that at no point in the mentioned code review was I mean, or disrespectful. I was just direct vs. my usual indirect self. I never said anything like “this is stupid” or anything personal. I didn’t have any malicious intent nor did it come off that way, unless you had gotten a review by me before and knew how I usually code review:
“I think this would be better expressed as X for performance, wdyt?”
became:
“Express this as X for better performance”
I didn’t use my lack of being awake as an excuse at work. I apologized and just said I didn’t realize how wording things differently would affect my coworker and it truly did upset me as well. It probably wasn’t until I worked with someone who always reviewed like that did I realize how annoying that kind of wording actually was, but that wasn’t until years later. Anyway, it was a weird day, because I wasn’t intentional in my wording and I paid for it. It’s especially weird when you know exactly what happened and you want to explain it so that maybe the other person feels better about the situation but doing so would just come across as being an ass or just trying to make an excuse. It’s a terrible feeling when it’s a real issue in your own life, but other people don’t have a basis to relate. Further, it a terrible feeling to see someone suffering from it and all you can do is feel shame, and so sorry for the person you’ve said something bad too without even being aware of saying it, or unable to stop yourself from saying it.
So yeah, I don’t think it’s ok, but I’ve accepted it at this point in my life. Whatever bs comes out of my mouth, I accept responsibility for that bs.
However, I think it’s worth pointing out, that at no point in the mentioned code review was I mean, or disrespectful. I was just direct vs. my usual indirect self. I never said anything like “this is stupid” or anything personal. I didn’t have any malicious intent nor did it come off that way, unless you had gotten a review by me before and knew how I usually code review:
“I think this would be better expressed as X for performance, wdyt?”
became:
“Express this as X for better performance”
I didn’t use my lack of being awake as an excuse at work. I apologized and just said I didn’t realize how wording things differently would affect my coworker and it truly did upset me as well. It probably wasn’t until I worked with someone who always reviewed like that did I realize how annoying that kind of wording actually was, but that wasn’t until years later. Anyway, it was a weird day, because I wasn’t intentional in my wording and I paid for it. It’s especially weird when you know exactly what happened and you want to explain it so that maybe the other person feels better about the situation but doing so would just come across as being an ass or just trying to make an excuse. It’s a terrible feeling when it’s a real issue in your own life, but other people don’t have a basis to relate. Further, it a terrible feeling to see someone suffering from it and all you can do is feel shame, and so sorry for the person you’ve said something bad too without even being aware of saying it, or unable to stop yourself from saying it.
So yeah, I don’t think it’s ok, but I’ve accepted it at this point in my life. Whatever bs comes out of my mouth, I accept responsibility for that bs.
“Express this as X for better performance” doesn't make people cry. So either
A) Your co-worker is more sensitive than anyone I've ever met B) You are expressing yourself much harsher than you think you are
Again, I don't know you but pattern-matching based on my experiences it's almost always B.
A) Your co-worker is more sensitive than anyone I've ever met B) You are expressing yourself much harsher than you think you are
Again, I don't know you but pattern-matching based on my experiences it's almost always B.
> People who wake up "ready to go" seem to be pretty rare.
I know this is a complete tangent, but do you find this to be true specifically of habitual coffee drinkers? I have a pretty horrid history with a sleep disorder, and while I'm a little slow after waking up, I don't think my mood is especially worse. I wonder if it's simply because I don't have a stimulant addiction like most people do.
I know this is a complete tangent, but do you find this to be true specifically of habitual coffee drinkers? I have a pretty horrid history with a sleep disorder, and while I'm a little slow after waking up, I don't think my mood is especially worse. I wonder if it's simply because I don't have a stimulant addiction like most people do.
Using "socially retarded" is a huge red flag to me, about you.
> It's a massive chore to have a teammate that causes stuff like this and I don't envy your teammates or lead (even worse if you're the lead, obviously).
It gets tricky. I haven't had to deal with this friction for many years, but it's not because I entirely avoid the type of directness described in OP's last two examples. It's because I'm careful about who I'm direct with, and because I've ended up in environments that are increasingly devoid of the emotionally-fragile.
I start out with a baseline of assuming that people are as fragile and childish as what your comment alludes to. In many companies, I'm sure this is actually the case. But as I get to know a coworker, it's very often the case that they have the talent, maturity, and emotional stability to handle direct communication (in both directions) without spiraling into an episode.
In my early career, I joined a company with an awful hiring pipeline as the first employee. That was my first introduction to the idea that a lot of people react violently to being treated with respect if they think they don't deserve it. I quickly started treating them like children as you suggest, and the problem immediately vanished.
But as I've progressed in my career, the amount of time I spend around mediocrities has plummeted, and my prior that the person I'm communicating with is a mental adult has risen. I've found that high-productivity professional contexts weed out the emotionally-unstable in the same way they weed out (eg) the disorganized; the value of good-faith efficient communication without mental breakdowns is simply too high.
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.
Wow not excusing hurting people's feelings, but if you are literally crying you have become too identified with your code and possibly your job. Or else, you just cry too easily and that's a problem when working with others also.
Crying? Did you leave some Linus Torvalds level feedback, like tell them to find a new career or something?
That's not level==Linus; for that you'll have to tell something like:
"You wrote the code, and you seem to be unable to admit that your code was buggy. It's not a compiler bug. It's your bug. Stand up like a man, instead of trying to flail around and blame anything else but yourself."
"You wrote the code, and you seem to be unable to admit that your code was buggy. It's not a compiler bug. It's your bug. Stand up like a man, instead of trying to flail around and blame anything else but yourself."
Needs more profanity to be accurate
People are sensitive. Directness is underappreciated. Inability to take critique is killing code quality. Try going to an art school. Critique is how we get better. Check your ego at the door and you'll go far as a programmer
I'm one of the likes who appreciates directness over subtle critique. I hate the latter but I also found out that it seems to work better with most of the people.
Try going to an art school. Critique is how we get better.
I've seen modern art - it's not working.
I've seen modern art - it's not working.
They have courses on constructive critique in art school!
I think software developers would do well to take such a course.
I think software developers would do well to take such a course.
I'm curious, what happened in the long run?
Was this developer generally unprofessionally sensitive? What happened to your manager?
Was this developer generally unprofessionally sensitive? What happened to your manager?
I left the company within the next few months, the manager is still doing well. I haven’t kept in touch with the other dev.
Good move.
I had a similar overly-sensitive colleague. (She was support, and would get flustered at my short responses like "please file a ticket" or "logs.")
She left the day after I met with her and explained common sense. (Customers expect you to know how to use the product; don't bypass the ticketing system by emailing me directly; follow the documented process to escalate to engineering; don't just copy and paste customer emails without doing your own investigation...)
I was having a lot of trouble with getting my manager to intervene with her, so in this case her departure was a godsend. We ended up firing my manager a little later, and things ran so smoothly afterwards that I realized he was the real source of the problems.
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.
“Won’t it be better if we did that instead of this?”
“Do you think it would be better if we did that instead of this”
“I think it would be even better if we do this instead of that”
I usually use “we” instead of “you” in my code reviews, “we” feels more friendly and comforting and way less judgy.
I usually use “we” instead of “you” in my code reviews, “we” feels more friendly and comforting and way less judgy.
I agree. I tend to use "we" when for the negative stuff of anything that has to be decided by the team ("we changed the code here but it caused a regression", "should we refactor this?"), however I also like to use "you" for positive stuff and to give credit ("your change in b8e63dca also fixed this bug, livinglist").
Personally, I prefer "What do you think about extracting this into a separate function?"
Unlike in any of your examples, the subtext is: "I anticipate you had reasons for doing it the way you did and I am open to listening to them." It engages the author on the same footing, leaves the door open for discussion but still communicates your intent in a straightforward manner.
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.
Personally it all depends on the rest of the comments in the review, if you use one style all the time then you lose the ability to indicate which changes are the ones you feel are more important than others.
I'd use the more question style comments for the minor changes, and save the stronger comment style for those changes that are really important.
So I'd word a comment about changing a variable name to something slightly better as a question. e.g. Would accounts be a better name for this array?
While a comment addressing a security concern would be a direct statement. e.g. Use a parameterised query here, as it'll be harder for someone to exploit.
One thing to note is that when making the strong statements I like to back them up with a reason, that way it helps me to think through the comment itself, helps a more junior developer understand why they should change it and helps more senior developers to not take the comment personally.
The other side to this though is that as senior developers we need to set an example for our more junior colleagues on how to take review comments. I always try to take them in a good mood, make the changes when I agree with them and have a constructive conversation about those that I don't agree with.
I'd use the more question style comments for the minor changes, and save the stronger comment style for those changes that are really important.
So I'd word a comment about changing a variable name to something slightly better as a question. e.g. Would accounts be a better name for this array?
While a comment addressing a security concern would be a direct statement. e.g. Use a parameterised query here, as it'll be harder for someone to exploit.
One thing to note is that when making the strong statements I like to back them up with a reason, that way it helps me to think through the comment itself, helps a more junior developer understand why they should change it and helps more senior developers to not take the comment personally.
The other side to this though is that as senior developers we need to set an example for our more junior colleagues on how to take review comments. I always try to take them in a good mood, make the changes when I agree with them and have a constructive conversation about those that I don't agree with.
[Suggestion] Use conventional comments[1] to flag how important the comment is.
Most of my comments end up being "suggestions", but then when I put a [blocking] on it, it clearly communicates that I think this should be fixed before merging in.
1: https://conventionalcomments.org/
Most of my comments end up being "suggestions", but then when I put a [blocking] on it, it clearly communicates that I think this should be fixed before merging in.
1: https://conventionalcomments.org/
Using conventional comments also forces me to rethink if my comment is really blocking or is just an optional suggestion and I can adjust the tone accordingly.
[deleted]
I use conventional comments everyday. It just helps me communicate clearly and concisely.
praise: I came here to suggest conventional comments as well!
thought: reading this style of sentence would get annoying quickly.
I don't get the point of these TBH. The usefulness of conventional commits is that they can be easily aggregated to get an idea of the types of changes that were made.
Code review comments are purely meant for human communication, so requiring a specific structure is both annoying to write and to read.
I tend to use one, at most two prefixes. "Nit", and rarely "blocker". Though I'd rather specify if something is a blocker once I make my case about what needs to be changed.
Every other comment is assumed to be a suggestion, and depending or not I block the PR by "requesting changes", it's up to the author to either accept my suggestion or provide reasons not to.
Any other strict guideline about how to write prose is silly to me. Especially the decorations section. C'mon now.
I 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.
+1 for use of Conventional Comments. It's a simple lift for a noticeable improvement in clarity, and guidance on what to do next.
Offtopic but related:
Conventional commits: https://www.conventionalcommits.org/en/v1.0.0/
Conventional commits: https://www.conventionalcommits.org/en/v1.0.0/
I wouldn’t be happy working in the team where people constantly consider #6 as rudeness.
This looks so unproductive and unprofessional to care too much about the tone vs content in programming business.
This looks so unproductive and unprofessional to care too much about the tone vs content in programming business.
That's fair if you have the same stance for all social interactions. If you don't then it's odd since nothing about programming makes people less human and less impacted by the usual social considerations. Although even if that is your stance for everything it's likely not the stance of everyone on the team.
I've seen people hurt by 1-4 versions though - it's not clear for a lot of people (especially non-native speakers) that those questions coming from Americans actually represent a veiled command shrouded in faux politeness.
I've seen junior devs from other countries get into trouble because they ignored these kind of "questions" since they actually thought they were optional questions and not expectations by other developers.
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.
It’s not that.
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.
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.
I typically go with something like "Have you considered extracting this to a separate function"?
Code reviews are tough when people have strong opinions. I try to ask questions rather then making statements, it gives a room for discussion and makes the whole process less challenging
There's definitely a lot to good code reviews. One addition perspective I've started taking that I haven't seen so far in the comments is a "Have you considered this alternative approach type question". And to describe the alternative, show it, etc. And this ensures there is an option to basically reject the suggestion, a valid response is I've considered and rejected that idea or approach.
Is this work you're collaborating on together (as peers) or work you're supervising? Are you in any sort of relationship where you could be considered to be 'mentoring' the developer?
When collaborating, I like to provide my feedback in a way that allows the person to provide their own perspective.
"What do you think about moving this to a separate function so that we can keep each functions in the class small and focused on one specific purpose?"
When supervising you of course want to allow two-way-communication but you can be more direct, but make sure to use the correction as a teaching moment.
"Please move this block to a separate function so that we keep the functions in the class small and focused on their one specific purpose. See abc.js and xyz.js for good examples of classes that follow this pattern."
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."
None of the suggestions contain any justification for taking the action. In many cases a reason may be obvious, but as it is not the case that every opportunity for creating a separate function should be taken (for one thing, doing so can make reading and understanding the code more difficult), this becomes an issue of judgement.
When you are reviewing, what is your authority level? Do you enforce a checklist? Are you merely asked to give your professional opinion? Are you a senior supervising a junior? These are critical inputs for me to calculate tone.
When I've done reviews, I had some seniority and an organizational privilege to veto some code. I worked from a checklist and a goal (with which the checklist was meant to align, but we knew it was not possible to fully automate those aspects of review). These are my takeaways from that arrangement:
Language like "declined" or "REJECTED" or "can't approve" is discouraging to the individual contributor. I replaced all that with discussion of why I can't allow that, under the obvious subtext of rejection. No need to just rub it in when there is learning to offer.
When indicating required changes, especially where I was more-or-less handing them the replacement code, I always said please. Always.
Most importantly, I gave accurate feedback. I took the time to be sure I was right before I wrote a review. Otherwise what's the point. Even simple patches got tested.
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.
[deleted]
This is a great question because I find that when I use soft language like this, a lot of the suggestions get ignored. Sometimes that is fine but other times, I have to follow up after and explain that the 'suggestion' wasn't actually optional. I find it hard sometimes to protect egos and quality at the same time.
My "formula":
1. Try to find something positive to say about the code. This reinforces what is being done well and supports the notion that everyone is on the same team chasing the same goal.
2. I like to go deep into the "why", being as objective and fact-based as I can. Code style and "best practices" are sometimes confused with personal opinions and preferences. I avoid words like "I prefer". If I can't back up my opinion with strong reason-based arguments for why my opinion is objectively better then I keep it to myself and continue to ask myself if it's a personal preference or if there's a good reason for favouring that.
3. Tone must always be friendly, polite and constructive. Of your options I often go "I would extract this to a separate function because ..." and then I go into the "why"
4. Sometimes I have a lot of supporting information to explain a position. I will often label these "Academic Digressions" and then drop a TL;DR. Sometimes people don't have time to read them right away but I'm often told that people appreciate them and save them for later.
1. Try to find something positive to say about the code. This reinforces what is being done well and supports the notion that everyone is on the same team chasing the same goal.
2. I like to go deep into the "why", being as objective and fact-based as I can. Code style and "best practices" are sometimes confused with personal opinions and preferences. I avoid words like "I prefer". If I can't back up my opinion with strong reason-based arguments for why my opinion is objectively better then I keep it to myself and continue to ask myself if it's a personal preference or if there's a good reason for favouring that.
3. Tone must always be friendly, polite and constructive. Of your options I often go "I would extract this to a separate function because ..." and then I go into the "why"
4. Sometimes I have a lot of supporting information to explain a position. I will often label these "Academic Digressions" and then drop a TL;DR. Sometimes people don't have time to read them right away but I'm often told that people appreciate them and save them for later.
Pointing out the good in a PR is such an important thing to do, particularly with juniors!
Easy:
Start with "Consider..." in case it‘s not a must.
If it‘s a must, start with explaining the consequences, e.g. "this value must be kept in a request scoped context or there will be race conditions and other concurrent misbehavior. Consider putting it in a @RequestScope bean."
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 wouldn't personally use any of these, but I'm a bit of a code review heretic.
I view (peer) code reviews mostly as an opportunity to discuss and familiarize your team with your code, and I think they should conform to the "social rules" that prevail in the rest of the world. If a peer in the real world was sharing something with me, I would very rarely command or strongly suggest they change this or that thing about it. Instead I would strive to create a collegial atmosphere by being receptive and supportive 90% of the time so that 10% of the time a suggestion I offered would be received gratefully and gracefully.
None of this can happen in a github pull request.
There was an alternative that I used to see before the PR monoculture took over that I liked. It could look like a weekly "code review" in-person meeting where a random developer would put together a presentation about a new module they'd written and project it on a wall. Mostly the other devs would listen and learn, and any suggestions they made would be strictly optional. The dev might leave feeling a bit embarrassed about something - but never feel "under the thumb" of a peer who was forcing an issue.
I 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.
“Please extract this to a separate function”
There's basically two categories here:
- you have an opinion on how things ought to be done and want a dialog
- you see some code that's wrong or violates an agreed-upon rule and so it should be changed with no discussion
I'd switch the tone based on what you're addressing. Giving a rationale when you share an opinion or point out a mistake also softens the tone (for the better IMO).
>* Should we extract this to a separate function?
>* Could you extract this to a separate function?
These are essentially the same: opening a dialog over an opinion. I'd suggest this when there's not an obvious flaw or rule violation or you have a gut-feeling about how code should be and want the author's input.
>* I would extract this to a separate function.
This is almost a command but isn't clearly one. It should be followed by some rationale, at least.
>* This could be extracted to a separate function.
This one's the least useful. You could do a lot of things with code. It doesn't resemble the command or inviting tones of the other examples here.
>* This should be extracted to a separate function.
>* Extract this to a separate function.
These are commands and are practically the same tone and best when catching mistakes. Unless obvious, a rationale should be given like a demonstrable flaw in logic or inconsistent abstraction, etc. There should be a few sentences explaining this. If there isn't a clear violation, I'd prefer the dialog invitation tones.
- 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.
This is really useful. Thanks.
Another thing that people don't often consider is that while some really appreciate being given a pointer to the rule as a justification (these are in a wiki / code style doc usually), many people will chafe at the "pedantry". In general, those second group need a bit thicker skin, but it is something to be considered.
What about clarifications? In a sense when you're not sure if this is good code, and would need to get more details. I sometimes just ask directly ("Why is this there?"/"What does this do?"), but other times I just don't bother and let others review.
I also think it depends a bit on the other person and the rapport one has with them.
At least I try to remember back to when I was a junior, and how "personal" the feedback felt when I was just starting. So when reviewing for others in the same situation I make extra sure it doesn't come off the wrong way by adjusting my tone.
But for someone I've worked with for years we're often a bit shorter and to the point, and everyone's happy.
At least I try to remember back to when I was a junior, and how "personal" the feedback felt when I was just starting. So when reviewing for others in the same situation I make extra sure it doesn't come off the wrong way by adjusting my tone.
But for someone I've worked with for years we're often a bit shorter and to the point, and everyone's happy.
Yeah I remember being a junior and feeling like blunt comments were kind of rude. So I make an effort to be nicer to new developers - sometimes simply adding “what do you think?”/“do you agree?” is enough to make those command style sentences sound much less blunt, and also encourages them to ask questions instead of blindly following my suggestion if they don’t fully understand it.
To me those questions could come off as condescending. I say could because I wouldn’t think that’s your intent, but the tone is there.
That depends entirely on whether you add the question as a formality or if you are genuinely interested in why the other person might disagree.
To me ’…don’t you agree?’ is the sort of rhetoric that parents use to make their kids internalise their beliefs.
If someone disagrees I expect them to speak up, if they’re not so inclined I’m interested to know why, so we can address/resolve that and move on.
If someone disagrees I expect them to speak up, if they’re not so inclined I’m interested to know why, so we can address/resolve that and move on.
The comment says "do you agree?", not "don't you agree?". I agree that the latter sounds condescending, but the former sounds reasonable to me.
The best way I've found to introduce junior developers to code reviews is to pair on it. Just go side-by-side down the diff and hop into an editor when necessary. It also helps teach a developer how to review code and what to look for--which has knock-on benefits of its own.
My company used to do this as default (pre-wfh and other changes), now no-one really thinks of it as a possibility which is a shame.
I was junior for most of that time and I think "use a totally different approach" or "change these 10 things" can sting a lot less that way. Your mistakes aren't all in black and white for the world to see, people can tell when you get something and don't end up patronising you, but you can also blur the lines a bit between "hey, what do you think of this, not sure" and "I think this code is 100% ready for merge and officially request permission to do so" - like, I know there are going to be comments, pretending otherwise will just make it worse.
Most of the time you'd get a review immediately too, less complicated structure then so one approval was usually enough, and it made sense for that to be high priority in my mind - this task is one step away from being done, I don't mind being interrupted.
The only thing is when I was the reviewer, it maybe went a bit too far one-sided learning experience, sometimes comments would get brushed under the carpet without much explanation/there was an assumption I'd approve.
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.
<sarcasm>
I like your suggestions... I would add that they are more effective if you keep a background noise with whips cracking sounds and random screams
</sarcasm>
> I'd switch the tone based on [whether you want a discussion or are insisting on a change]
I think that's a great example of where "tone" is absolutely not sufficient, especially in the modern world where many readers aren't going to share your first language and won't absorb the nuance.
If there's a situation where your disagreement is absolute, you need to say that factually, ideally with a recipe for how to resolve that included:
"I can't approve this scheme, please do X instead."
"This API isn't OK to use here, you have to..."
"Under no circumstances will I ever approve of a feature that does this."
You can phrase those as nicely as you want, but it's imperative that your disagreement be spelled out explicitly.
I think that's a great example of where "tone" is absolutely not sufficient, especially in the modern world where many readers aren't going to share your first language and won't absorb the nuance.
If there's a situation where your disagreement is absolute, you need to say that factually, ideally with a recipe for how to resolve that included:
"I can't approve this scheme, please do X instead."
"This API isn't OK to use here, you have to..."
"Under no circumstances will I ever approve of a feature that does this."
You can phrase those as nicely as you want, but it's imperative that your disagreement be spelled out explicitly.
It's sometimes refreshing to have people just write what they want me to do, clearly and without embellishment. Even if it's sometimes nitpicking, I know that if I just fix it I can then merge my patch. I don't have to wait for a second look from them because their comments were so direct "write this instead".
There are different categories of things you might see:
1. This is wrong. I can tell from reading your code that it doesn't do what the description of this PR or the name of a test or some other indicator shows its supposed to do. This should be communicated in a tone of "you must not merge this".
2. This violates our agreed-upon style or best practices. The strictness of enforcement is part of that agreement and company culture. At my current company, this would be communicated along the lines of "this is not how we prefer to do this, so unless you have a good reason why our standard method is wrong, change it"
3. This is confusing to me, or I have a suggestion for improvement. This should be communicated as a suggestion or general feedback. If you're getting miffed about people not taking the suggestions, maybe it's actually in a category above, or maybe you need to adjust your own assumptions about how much stylistic consistency to insist on, since it sounds like you don't have a consensus.
4. You are new, either to software engineering or at least to this company, and your style is inconsistent with how things are done in this code. This is the same as #3 but should be communicated more strongly and depending on your company normals may be effectively a requirement.
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 could be extracted to a separate function.
> This one's the least useful. You could do a lot of things with code. It doesn't resemble the command or inviting tones of the other examples here.
It's clearly a politely phrased command, and to think otherwise is intentionally missing the point.
> 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.
“Obey!”
Depends on the situation. If it is a clear cut case or you're a mentor/senior to the reviewee, a more direct tone is fine. If it's a matter of opinion or it's a peer review then a more considerate tone would come across better.
1) State the specific reason that motivates you to ask for a change.
‶This code is duplicated several times.″
—
2) State the change you'd like.
‶I'd suggest extracting it to a separate function.″
—
3) (Optional) Expand on why you suggest solving the issue with this solution instead of another one.
‶We could create a macro instead but it's not worth it as we can just let the compiler decide whether to inline the function or not.″
‶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.″
[deleted]
[deleted]
I have been looking for the tone too. Tone is important for techies, too. I just think it's not the biggest issue.
I think the real problem is the we lack a framework that makes explicit:
* What is the goal of a review, according to the team? Spotting bugs? Prevent security flaws? Guarding the architecture? Making suggestions to grow better as programmers? These goals partially overlap, but are different. Are you aligned, as a team?
* Encode your priority in each comment. We could use some semantic prefix for a comment that categorizes it as, in increasing order: 1.suggestion to share views and alternative approaches, do as you please / 2. I think another approach would be better, suggest you use it / 3. I think it's very important to change something, let's discuss / 4. I'm not willing to approve or compromise, but feel free to get approval from someone else / 5. If anyone allows this to be merge, I will escalate.
Then still we should use good tone, but it's more explicit what to expect, now. Open communication is key to a psychologically safe environment. But I'm Dutch :-)
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 :-)
A good way to package possibly negative feedback is to formulate it as an I-message, or an observation and go from there:
"I find this method to be overwhelmingly long and it's hard to keep track through all of it. However, it looks like this, this and this are rather isolated steps, they could be extracted to make the overall structure easier to see".
"To me, this, this and this look like duplicated code. Do you expect them to evolve differently, or could they be extracted into a common function / module?"
And if the code goes against architectural or style guidelines, it also softens the feedback to start with that reason and go to the suggesting from there. "This class was designed to be independent of that module to be testable. Please extract your code into the foobar-interface and inject it"
"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"
I normally say why - even if they're 'experienced' sometimes people can sometimes come from, say in this case, cultures where copypasted code is normal. So say why:
* "We're repeating this in a few places. Could you extract this to a separate function? It'll keep the codebase smaller and more readable, and if we update it, there's only one place we need to change."
* "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."
What do you mean to say when you want to review code?
Personally, I have roughly three distinct levels of things that I comment on in code reviews:
1. Does not need response, but is my personal opinion about how the code probably would look nicer. In this case, I would do #2. If you just say "no," that's fine. I can revisit the issue later. Some people think these comments don't actually belong in a code review, and I think it depends a lot on your relationship with the code and the person asking for review.
2. Needs response, but is potentially open for discussion instead of a change to the code. I would do #4 here.
3. You must make this change or I will not accept. I would personally do #6. There is no need to use "non-confrontational" language with many people when you mean to be confrontational. There is a hard line about what you will and won't accept. If it is a major comment, you need to explain your rationale here out of courtesy to the person sending you code.
Most things that fall into the third category are around code style conventions and minor bugs. More substantive changes, like changes to an architecture, often fall into the second category.
Edit: Also, when I nitpick your code for one reason or another (which generally only happens when I am enforcing a company style guide), I will often say "Nit:" and then use the sixth style. If you are new to the team, I will quote the guide to you. Most people don't feel so bad about the nitpicks when you admit that you are nitpicking, and that it is for a reason.
Personally, I have roughly three distinct levels of things that I comment on in code reviews:
1. Does not need response, but is my personal opinion about how the code probably would look nicer. In this case, I would do #2. If you just say "no," that's fine. I can revisit the issue later. Some people think these comments don't actually belong in a code review, and I think it depends a lot on your relationship with the code and the person asking for review.
2. Needs response, but is potentially open for discussion instead of a change to the code. I would do #4 here.
3. You must make this change or I will not accept. I would personally do #6. There is no need to use "non-confrontational" language with many people when you mean to be confrontational. There is a hard line about what you will and won't accept. If it is a major comment, you need to explain your rationale here out of courtesy to the person sending you code.
Most things that fall into the third category are around code style conventions and minor bugs. More substantive changes, like changes to an architecture, often fall into the second category.
Edit: Also, when I nitpick your code for one reason or another (which generally only happens when I am enforcing a company style guide), I will often say "Nit:" and then use the sixth style. If you are new to the team, I will quote the guide to you. Most people don't feel so bad about the nitpicks when you admit that you are nitpicking, and that it is for a reason.
Yeah I agree w/ you. I think there are three major problems w/ code review:
- reviewer ego
- author ego
- reviewer ambiguity
"Solving" ego is involved, but easy: you have to encourage trust amongst your team, and that can't happen in code review--it's too late.
Reviewer ambiguity (being non-confrontational and wishy-washy when you really mean "this is an XSS vuln, you gotta change it") is an attempt to solve ego, but always fails.
I'm not saying be a jerk, you have to act with compassion and empathy always, which is a marathon and not a sprint. But you're not gonna fix ego/trust/respect issues at code review time, no matter how deferential you are.
- reviewer ego
- author ego
- reviewer ambiguity
"Solving" ego is involved, but easy: you have to encourage trust amongst your team, and that can't happen in code review--it's too late.
Reviewer ambiguity (being non-confrontational and wishy-washy when you really mean "this is an XSS vuln, you gotta change it") is an attempt to solve ego, but always fails.
I'm not saying be a jerk, you have to act with compassion and empathy always, which is a marathon and not a sprint. But you're not gonna fix ego/trust/respect issues at code review time, no matter how deferential you are.
I prefix my nitpicks with the same, but with the 3rd style.
> Edit: Also, when I nitpick your code for one reason or another (which generally only happens when I am enforcing a company style guide), I will often say "Nit:" and then use the sixth style. If you are new to the team, I will quote the guide to you. Most people don't feel so bad about the nitpicks when you admit that you are nitpicking, and that it is for a reason.
This. We do it on our team as well. Especially when it is smaller things like spelling, indentation, etc, I might have quite a few nits in one PR, so it makes it a little bit easier on the receiving end when you see a dozen comments but they're all marked as nit.
This. We do it on our team as well. Especially when it is smaller things like spelling, indentation, etc, I might have quite a few nits in one PR, so it makes it a little bit easier on the receiving end when you see a dozen comments but they're all marked as nit.
If there's a significant number of nitpicks, I'll generate an issue to implement/fix/improve linting tools.
+1 to the comment about using "nit:". I also don't think it's fair to block code changes for style/quality/whatever that is the same or better then the existing code base. If it's not making the existing codebase worse on average than don't block.
It depends on who the corworker is, how well I know them and how long the review is. My comments range from very polite to "???" "delete this" or "don't copy paste this".
Once I have worked with someone for a while (and perhaps have gone out for beers / coffee a few times), I write shorter comments.
At that point, they've usually figured out that I mean well, but am blunt.
For new hires, I spend a lot more time spent on "why" and tone.
If my team set guidelines for this, that'd be a strong signal of much, much deeper dysfunction and I'd consider switching jobs.
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.
My rules for reviews (which influence the language used) are:
* Criticise the code, not the author ("There's a bug here" vs. "You inserted a bug here");
* Reach common agreement that people shouldn't cling to code written ("sunk cost fallacy"). Code is liability, and it's okay to throw things away and restart from scratch, the value is in the learning;
* Don't use questions that invite discussion if you're not inviting a discussion, instead expose the reason you believe something should be different ("Should this be like X?" vs. "I think this should be like X for reasons A, B, C."). This avoids wasting time and reviews that turn into long discussions.
* Don't give orders ("Do this", "Do that") to your colleagues, since they are not your employees, and it doesn't help promote learning & growth by not exposing the reasoning behind the demand;
* Avoid bike-shedding about style/formatting/etc. If this is important where you work, automate with tools;
* Agree previously on what constitutes a non-passable review, if there's such a process.
* Criticise the code, not the author ("There's a bug here" vs. "You inserted a bug here");
* Reach common agreement that people shouldn't cling to code written ("sunk cost fallacy"). Code is liability, and it's okay to throw things away and restart from scratch, the value is in the learning;
* Don't use questions that invite discussion if you're not inviting a discussion, instead expose the reason you believe something should be different ("Should this be like X?" vs. "I think this should be like X for reasons A, B, C."). This avoids wasting time and reviews that turn into long discussions.
* Don't give orders ("Do this", "Do that") to your colleagues, since they are not your employees, and it doesn't help promote learning & growth by not exposing the reasoning behind the demand;
* Avoid bike-shedding about style/formatting/etc. If this is important where you work, automate with tools;
* Agree previously on what constitutes a non-passable review, if there's such a process.
I agree in general, though I find the second on clinging to code difficult when people counter with timelines. It can be difficult to express to juniors how to reason about taking a bit more time to do it "right".
Even when you express it clearly, many juniors won't understand and will feel frustrated. Some things can't be taught and must be experienced. Every junior developer must work on a crufty legacy codebase to develop understanding and habits for writing maintainable code.
Large companies pick experienced internal transfers for shiny new projects and hire externally for unglamorous legacy systems. Since all new grads are external hires, they end up starting out on legacy systems.
I recommend that junior devs start out at Amazon. They will learn a lot of important things in a year or two: the pain of technical debt, the value of automated tests, the pain of production issues when you're on call, that working overtime is not rewarded and does not solve job insecurity, and that switching companies is a great way to get more experience and earn more money.
Large companies pick experienced internal transfers for shiny new projects and hire externally for unglamorous legacy systems. Since all new grads are external hires, they end up starting out on legacy systems.
I recommend that junior devs start out at Amazon. They will learn a lot of important things in a year or two: the pain of technical debt, the value of automated tests, the pain of production issues when you're on call, that working overtime is not rewarded and does not solve job insecurity, and that switching companies is a great way to get more experience and earn more money.
> Avoid bike-shedding about style/formatting/etc. If this is important where you work, automate with tools;
I'd love some suggestions for how to get people to avoid this type of bike-shedding. While I'd love for us to use some tools to automate this, we have a huge codebase with existing code that doesn't follow the rules, and it's not in my ability to implement said tools. (Big company, lots of overhead, will affect a lot of people, don't have a lot of time, not my department, etc.)
One of the things I hate is having a different comment for every single place where I misplaced a star or ampersand. My natural tendency is to do it one way, but our style guidelines prefer it the other way, so it's something I miss frequently. Having 30 comments in a review that are all "move the star to the other side" over and over again just makes me hate the reviewer. (Particularly if they don't provide any useful feedback.) A single comment on the first one that says something like, "This doesn't match the style guidelines, and I see several others that don't match. Please fix them all," would be highly preferable. And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy!
I'd love some suggestions for how to get people to avoid this type of bike-shedding. While I'd love for us to use some tools to automate this, we have a huge codebase with existing code that doesn't follow the rules, and it's not in my ability to implement said tools. (Big company, lots of overhead, will affect a lot of people, don't have a lot of time, not my department, etc.)
One of the things I hate is having a different comment for every single place where I misplaced a star or ampersand. My natural tendency is to do it one way, but our style guidelines prefer it the other way, so it's something I miss frequently. Having 30 comments in a review that are all "move the star to the other side" over and over again just makes me hate the reviewer. (Particularly if they don't provide any useful feedback.) A single comment on the first one that says something like, "This doesn't match the style guidelines, and I see several others that don't match. Please fix them all," would be highly preferable. And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy!
Are you sure it's not in your ability? Whenever I introduced something like that people were in general indifferent-to-happy, as long as you did the majority of the job and they could somewhat easily run those tools (e.g. anything written in Java is more or less a lost cause because people cling to their IDEs)
Tools are easy. You add it to the CI/CD pipeline, it fails if it doesn’t meet the guideline, people have to run the tool to get the merge in. On first run it’s painful, so if you’re not starting a project, use an autoformatter so it’s not all manual work. If you really want, you can even make your CI pipeline push commits with autofixes
Of course if you don’t have a CI pipeline that’s easy to integrate steps like this, you’ve got major issues to deal with elsewhere…
Of course if you don’t have a CI pipeline that’s easy to integrate steps like this, you’ve got major issues to deal with elsewhere…
I don't have the permissions to add it to the CI/CD pipeline. I've filed bugs on our CI/CD to add it, but it's apparently not a high priority (you know, until someone sits down to review code, then suddenly it's the most important thing in the world).
If I were you, I’d be looking to move on from your company then. Developers should own the testing part of the pipeline at the very least.
Sorry, but formatting is on you. If you don't like the comments, don't commit inconsistent code. Set up a local auto-formatter with the right rules. If enough of the existing code you're working on doesn't conform, set the formatter to only run manually and get in the habit of running it before committing.
Yeah, I've been looking for something to do that, but haven't found something that fits into my workflow yet.
Everyone is talking about how to get the style guide integrated into your CI. But it sounds to me like your concern is that other reviewers frequently point out your style guide violations.
You could set up some local lint rules just for yourself. You'd need to set it up to only run against code that you changed. And if you don't have an automated style guide you'd have to come up with your own. But it seems like once you get over the hump you could just slowly add rules as you get tired of forgetting about them/people pointing them out to you.
> And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy!
This is a concerning attitude. If the team has agreed on a set of style guides to follow then you should follow them, argue to change them, or move on.
If you don't have time to adhere to the style guide now, why do you think you will have more time to go back and fix it later?
You could set up some local lint rules just for yourself. You'd need to set it up to only run against code that you changed. And if you don't have an automated style guide you'd have to come up with your own. But it seems like once you get over the hump you could just slowly add rules as you get tired of forgetting about them/people pointing them out to you.
> And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy!
This is a concerning attitude. If the team has agreed on a set of style guides to follow then you should follow them, argue to change them, or move on.
If you don't have time to adhere to the style guide now, why do you think you will have more time to go back and fix it later?
In addendum, if a couple of style guidelines don't apply to a couple of lines of code for a couple of people, would it have any meaning at all?
Their concern isn't that other reviewers point it out. It's that they think they shouldn't have to follow the guidelines:
> My natural tendency is to do it one way, but our style guidelines prefer it the other way
Which is fine if it's a reason it's an occasionally slip up. But apparently, in spite of knowing this tendency, they frequently submit code to review where they've messed up to such a degree that reading every instance is considered annoying. Think of the poor reviewer who had to put all the comments in!
> My natural tendency is to do it one way, but our style guidelines prefer it the other way
Which is fine if it's a reason it's an occasionally slip up. But apparently, in spite of knowing this tendency, they frequently submit code to review where they've messed up to such a degree that reading every instance is considered annoying. Think of the poor reviewer who had to put all the comments in!
Well that's a very uncharitable way to read what I wrote.
I previously worked on code bases where it had to be one way. It became pretty engrained in me. Now it needs to be the other way. I try my best to use the "new" style now, but because I'm human and busy, I sometimes forget. I spend most of my time focusing on the problem I'm working on and not how I'm placing characters on the line.
I'm not the only one, so I don't think I'm being a particularly bad programmer. We have one reviewer who's really concerned about star placement. It's sometimes the only thing he points out, and he points it out on a lot of different people's work, so I'm certainly not worse than anyone else on the team. As I say, I do try, but sometimes I forget.
The other thing is sometimes I didn't write the code in question. I hit the issue where I'm refactoring and I move a chunk of previously-written code that didn't follow the guidelines, but I didn't modify the code I moved, so now it looks like I "forgot" but in reality, I just moved some code around. How much time do we want to spend reformatting code instead of working on the actual problem we're trying to fix or implement? I would love if we could automate this, but see my other comment about the issues with that.
I think these are all understandable things that busy people have to deal with. It's not that I don't think I shouldn't have to follow the rules. It's that some rules are much more important than others, and differences in code that have the same readability but don't in any way change the functionality seem less important to me than things that can actually cause bugs.
I previously worked on code bases where it had to be one way. It became pretty engrained in me. Now it needs to be the other way. I try my best to use the "new" style now, but because I'm human and busy, I sometimes forget. I spend most of my time focusing on the problem I'm working on and not how I'm placing characters on the line.
I'm not the only one, so I don't think I'm being a particularly bad programmer. We have one reviewer who's really concerned about star placement. It's sometimes the only thing he points out, and he points it out on a lot of different people's work, so I'm certainly not worse than anyone else on the team. As I say, I do try, but sometimes I forget.
The other thing is sometimes I didn't write the code in question. I hit the issue where I'm refactoring and I move a chunk of previously-written code that didn't follow the guidelines, but I didn't modify the code I moved, so now it looks like I "forgot" but in reality, I just moved some code around. How much time do we want to spend reformatting code instead of working on the actual problem we're trying to fix or implement? I would love if we could automate this, but see my other comment about the issues with that.
I think these are all understandable things that busy people have to deal with. It's not that I don't think I shouldn't have to follow the rules. It's that some rules are much more important than others, and differences in code that have the same readability but don't in any way change the functionality seem less important to me than things that can actually cause bugs.
I haven't yet taken this concept to completion, but when I had a similar issue years ago I hacked out a cheesy program to filter out static analysis messages on lines not modified by the diff:
https://github.com/NateEag/diff-check
Maybe someone's built the complete version out there somewhere?
https://github.com/NateEag/diff-check
Maybe someone's built the complete version out there somewhere?
>(Particularly if they don't provide any useful feedback.)
If my eyes stumble on formatting violations every other line, you're probably also not going to get good feedback from me.
Code formatting isn't very important, yet it is.
If my eyes stumble on formatting violations every other line, you're probably also not going to get good feedback from me.
Code formatting isn't very important, yet it is.
I've been on a project where we dynamically generated tons of different Cypher queries. To keep those queries readable, consistent formatting was very important, and I had to keep bringing that up in code review.
I hated it. It absolutely should be automated, but I could not find any tools for formatting Cypher query strings inside Javascript code. I'd probably have to write it myself.
I hated it. It absolutely should be automated, but I could not find any tools for formatting Cypher query strings inside Javascript code. I'd probably have to write it myself.
Something as simple as a pre-commit hook that checks files that were changed in your commit and rejects the commit if the rules aren't followed is a very cheap and effective way to enforce a coding style.
For a python project for example, you could set pre-commit to check formatting, linting and typing, which means that when your code reaches the remote repository, before any CI is even started or any PR is openned (before you even push really), you know your code follows the rules that have been set for the project. Thus everyone knows you don't need to spend time in the code review looking for these mistakes and instead focus on the actual features.
pre-commit does not even need to be pushed to remote to work, so if your company has no plan to enforce it but it still helps you, you can still use it, unlike a CI.
As for this: > And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy!
It's true that one mistake is easy to fix later, but it's just as easy to fix now. If you postpone it once you will postpone it again when it's not highlighted in a PR diff, it's just a recipe for never fixing it. Every language has automatic linters/formaters that check (and even fix) your code according to rules you define, turning the work of finding these mistakes into a simple `lint ./src/` command. It's takes a small amount of time to setup but saves you a lot of time down the line.
For a python project for example, you could set pre-commit to check formatting, linting and typing, which means that when your code reaches the remote repository, before any CI is even started or any PR is openned (before you even push really), you know your code follows the rules that have been set for the project. Thus everyone knows you don't need to spend time in the code review looking for these mistakes and instead focus on the actual features.
pre-commit does not even need to be pushed to remote to work, so if your company has no plan to enforce it but it still helps you, you can still use it, unlike a CI.
As for this: > And if, god forbid, I miss one, just get over it. It can be fixed later. It's not nearly as important as having correct functionality in our app, and I'm pretty busy!
It's true that one mistake is easy to fix later, but it's just as easy to fix now. If you postpone it once you will postpone it again when it's not highlighted in a PR diff, it's just a recipe for never fixing it. Every language has automatic linters/formaters that check (and even fix) your code according to rules you define, turning the work of finding these mistakes into a simple `lint ./src/` command. It's takes a small amount of time to setup but saves you a lot of time down the line.
I love the idea of a pre-commit hook. I did actually implement this for a past project, and the rest of the team hated it. Whenever they thought it was being "unfair" to them, I'd get email about it because I wrote it.
The problem with my current project is that it has millions of lines of code and that code all has different styles. So even if you only check the lines that actually changed in a pre-commit hook, you can end up with different styles in the same file, which can be as bad as using the wrong style. At least if it's consistently wrong, working within a single file is doable.
> It's true that one mistake is easy to fix later, but it's just as easy to fix now.
Eh... maybe. I see your point, but I guess what I'm saying is "choose your battles." Every minor thing one points out like this takes time away from me fixing bigger issues, so be judicious in how much of a stickler you want to be for the rules. Yep, I can fix it now pretty easily, and then I won't be working on the next bug until it's fixed. And the way we have GitHub set up, if I make any change, I have to get at least 1-3 reviewers to re-review the code and re-approve it. That's the real time waster because everyone else if fixing the nit-picky things for their latest pull request. If they take too long, then I have merge conflicts and I have to address those before I can commit, and, oops! that causes the review clock to start over. We get to do it all again! Talk about Kafka-esque.
If we were starting a new project, I would probably demand that we start with some sort of pre-commit formatter that forces the appropriate style onto our code, but we're unlikely to be doing that for the foreseeable future, unfortunately.
The problem with my current project is that it has millions of lines of code and that code all has different styles. So even if you only check the lines that actually changed in a pre-commit hook, you can end up with different styles in the same file, which can be as bad as using the wrong style. At least if it's consistently wrong, working within a single file is doable.
> It's true that one mistake is easy to fix later, but it's just as easy to fix now.
Eh... maybe. I see your point, but I guess what I'm saying is "choose your battles." Every minor thing one points out like this takes time away from me fixing bigger issues, so be judicious in how much of a stickler you want to be for the rules. Yep, I can fix it now pretty easily, and then I won't be working on the next bug until it's fixed. And the way we have GitHub set up, if I make any change, I have to get at least 1-3 reviewers to re-review the code and re-approve it. That's the real time waster because everyone else if fixing the nit-picky things for their latest pull request. If they take too long, then I have merge conflicts and I have to address those before I can commit, and, oops! that causes the review clock to start over. We get to do it all again! Talk about Kafka-esque.
If we were starting a new project, I would probably demand that we start with some sort of pre-commit formatter that forces the appropriate style onto our code, but we're unlikely to be doing that for the foreseeable future, unfortunately.
That's good advice!
> * Don't give orders ("Do this", "Do that") to your colleagues, since they are not your employees, and it doesn't help promote learning & growth by not exposing the reasoning behind the demand;
I think this depends a lot. I do "readability" reviews at my company to make sure people apply good coding standards and adhere to the coding guidelines that we have. A lot of these are very "mechanical" in nature and when I am reviewing changes that have obvious coding standard mistakes I will write comments like a "linter" would. One of the most common examples:
> Use descriptive ("Uses") rather than imperative ("Use") style for function docstrings. See: <url-explaining-docstring-guidelines>
I think this type of "giving orders" in reviews is fine as long as it's clear this is an obvious misuse/misunderstanding/miss of the readability guidelines. For any other kind of comment I'll phrase it differently ("Is there a reason why you did X? I think doing Y might be more appropriate because <reasons>, what do you think?", etc) but for stuff that linter should've caught I don't think it's necessary to be verbose and "sweeten" the comments too much.
I think this depends a lot. I do "readability" reviews at my company to make sure people apply good coding standards and adhere to the coding guidelines that we have. A lot of these are very "mechanical" in nature and when I am reviewing changes that have obvious coding standard mistakes I will write comments like a "linter" would. One of the most common examples:
> Use descriptive ("Uses") rather than imperative ("Use") style for function docstrings. See: <url-explaining-docstring-guidelines>
I think this type of "giving orders" in reviews is fine as long as it's clear this is an obvious misuse/misunderstanding/miss of the readability guidelines. For any other kind of comment I'll phrase it differently ("Is there a reason why you did X? I think doing Y might be more appropriate because <reasons>, what do you think?", etc) but for stuff that linter should've caught I don't think it's necessary to be verbose and "sweeten" the comments too much.
> I do "readability" reviews at my company to make sure
> people apply good coding standards and adhere to the
> coding guidelines that we have.
Consider avoiding the entire formatting arguments as part of the code review process. There are excellent linters for every language that can run as part of the check-in process. GitHub + Actions can automate every aspect of this using all turkey stuff.
Automating where possible avoids all the arguments, actually enforces standards, and saves time for everyone. Using "Standard" linting rules (aka: The defaults) also help promote good practices for everyone.
Consider avoiding the entire formatting arguments as part of the code review process. There are excellent linters for every language that can run as part of the check-in process. GitHub + Actions can automate every aspect of this using all turkey stuff.
Automating where possible avoids all the arguments, actually enforces standards, and saves time for everyone. Using "Standard" linting rules (aka: The defaults) also help promote good practices for everyone.
Where I work there is a readability review like mason55's (probably the same company). Most of the comments I see are not about formatting. They're more like "Take parameters by std::string_view instead of const std::string&" or "Use a reference member instead of a pointer member if it'll never be null."
Applying a linter to a giant company-wide monorepo can be difficult because of decades-old code that predates certain style rules, or some teams that might have created their own rules. Even if the linter only applies to diffs, a small change to a line can trigger a lint warning on that line, and having a ton of those can be annoying.
Applying a linter to a giant company-wide monorepo can be difficult because of decades-old code that predates certain style rules, or some teams that might have created their own rules. Even if the linter only applies to diffs, a small change to a line can trigger a lint warning on that line, and having a ton of those can be annoying.
> Consider avoiding the entire formatting arguments as part of the code review process.
That is not going to help me if people send code to review that has formatting problems. We catch a lot of stuff using automated tools, but there's some stuff that you cannot catch. Some people declare every variable as global rather than private, and then I have to follow up with "This variable/constant should be private, any reason you marked it as external?" etc.
You can't just automate everything, and we also have our own specific code formatting guidelines that a lot of tools don't automatically detect by default. I have raised some of these points to our toolchain and engineer productivity teams but it's not my job to dig deep into our tooling and automation so until there's better support in our internal tools (we don't use github + actions) to detect that, I'll keep pointing out any formatting mishap that gets through our linters.
That is not going to help me if people send code to review that has formatting problems. We catch a lot of stuff using automated tools, but there's some stuff that you cannot catch. Some people declare every variable as global rather than private, and then I have to follow up with "This variable/constant should be private, any reason you marked it as external?" etc.
You can't just automate everything, and we also have our own specific code formatting guidelines that a lot of tools don't automatically detect by default. I have raised some of these points to our toolchain and engineer productivity teams but it's not my job to dig deep into our tooling and automation so until there's better support in our internal tools (we don't use github + actions) to detect that, I'll keep pointing out any formatting mishap that gets through our linters.
Consider extracting this to a separate function.
After reviewing my last few code review comments, this is my most frequent phrasing too. I get that questions are a way to soften the bluntness of a code review comment, but I've been hugged to death by this sort of niceness in the past. It was not fun.
"Consider ______" has the directness I, myself, enjoy, and whose response can take the form of a change, a reply, or a jumping off point for discussion. It indicates some action needs to take place without specifying exactly which one and leaves me open to having my mind changed if the response is "I've considered it and actually ..."
Still, if there is ever a flaw so great that the code simply will not work without a fix, I will phrase the fix in the form of a command.
"Consider ______" has the directness I, myself, enjoy, and whose response can take the form of a change, a reply, or a jumping off point for discussion. It indicates some action needs to take place without specifying exactly which one and leaves me open to having my mind changed if the response is "I've considered it and actually ..."
Still, if there is ever a flaw so great that the code simply will not work without a fix, I will phrase the fix in the form of a command.
It's about power dynamics.
You ask someone above you. You tell someone below you. You discuss with someone on the same level as you.
If you're smart, you'll quickly spot the assholes who think they're above others - you probably shouldn't comment on their code at all, other than 'great, looks good'.
If you think someone is below you, you're an asshole.
If you want to discuss, you need to include the word 'because'. 'Hey, can we do X, because I/we will need it for Y?' is the way I usually go about it. By the way, discussions are best had before anyone writes code, not after. So for me, I don't do code review - if someone's bad enough that they need their code reviewed on a regular basis, I will be leaving soon anyway.
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.
First decide what do you want:
* Is the change mandatory for your approval? Then be polite and firm with reasoning - "This is poorly readable and doesn't conform to our code style chapter 5. Please extract this into a separate function.". Don't mess around with "Should, Could, I would" starters when it's not a question - be clear what you're asking.
* Are you trying to start a debate? Then ask a question properly - "I think this is unreadable because of A, B, C. Do you think extracting this will help readability?" Again, be clear it's a question.
* Are you saying an opinion and you're not very hung up on it? Mark it as such - I tend to put "Optional" or "Nit:" (team term) in those cases. "Nitpick/Optional: I think this isn't readable, if you have a moment maybe extract the method?"
The worst you can do is veil your intent (order/question/opinion) into faux questions which will confuse people - especially if they're non-native english speakers. Adding background / explanation / context for your comment always helps too.
* 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.
> * Should we extract this to a separate function?
Reads very passive aggressive. No.
> * Could you extract this to a separate function?
Yes, I could.
> * I would extract this to a separate function.
Good for you.
> * This could be extracted to a separate function.
This one's OK as a style suggestion that you won't block the merge over.
> * This should be extracted to a separate function.
This is kinda, sorta OK, if you're going to block the merge if this isn't fixed.
> * Extract this to a separate function.
This is better because the tone matches the intent: it's a command. This gets addressed, or no merge. "This should..." is indirect—you're not stating what you actually mean. This phrasing is better.
4 is the best of these for optional suggestions, 6 is best if you consider the fix a requirement for a merge. In general, say what you actually mean—dancing around it leaves room for miscommunication and can give all kinds of bad impressions. Of course, that can also mean weakening language: don't command a change that you don't consider a big enough deal to block the merge.
[EDIT] And, as others have noted in the thread, of course expanding with justifications is always a good idea. "Do this" without a "why" should be avoided. But lead with what you want done, not the why—that follows.
Reads very passive aggressive. No.
> * Could you extract this to a separate function?
Yes, I could.
> * I would extract this to a separate function.
Good for you.
> * This could be extracted to a separate function.
This one's OK as a style suggestion that you won't block the merge over.
> * This should be extracted to a separate function.
This is kinda, sorta OK, if you're going to block the merge if this isn't fixed.
> * Extract this to a separate function.
This is better because the tone matches the intent: it's a command. This gets addressed, or no merge. "This should..." is indirect—you're not stating what you actually mean. This phrasing is better.
4 is the best of these for optional suggestions, 6 is best if you consider the fix a requirement for a merge. In general, say what you actually mean—dancing around it leaves room for miscommunication and can give all kinds of bad impressions. Of course, that can also mean weakening language: don't command a change that you don't consider a big enough deal to block the merge.
[EDIT] And, as others have noted in the thread, of course expanding with justifications is always a good idea. "Do this" without a "why" should be avoided. But lead with what you want done, not the why—that follows.
I agree with everything you said. But I’m Dutch.
If it's negative feedback or something that feels controversial I dont put it on a PR at all.
I try to do it in person or (remotely) send a private slack message.
People can be sensitive to negative feedback and one way of ameliorating that is not to give it in a public forum where everybody can see it.
I try to do it in person or (remotely) send a private slack message.
People can be sensitive to negative feedback and one way of ameliorating that is not to give it in a public forum where everybody can see it.
If this is on an open source community, it's bad advice IMHO. On open source communities, please make all feedback public. It is really important to make the history of the project public. There are many ways to write the feedback in a not-offensive way, many of the comments in this thread have good suggestions.
I was assuming a normal working environment.
OSS is a different kettle of fish. You dont have to worry about bruised egos nearly as much & the value of public feedback is higher.
OSS is a different kettle of fish. You dont have to worry about bruised egos nearly as much & the value of public feedback is higher.
Code reviews are primarily a way to detect bugs and to verify compliance with a set of rules.
Most comments are therefore expected to be 'negative' in some way.
Most comments are therefore expected to be 'negative' in some way.
My personal favourite is: "Have you considered X"
I also like to often prefix my suggestion with: "It might have missed something, but X", especially if it's a larger change.
I also like to often prefix my suggestion with: "It might have missed something, but X", especially if it's a larger change.
I hate it when people say "have you considered X?" and mean "do X or I will block your change." It's all about communicating what you mean.
I agree with this so much.
IMO if it's not mandatory I don't think it should be said at all in a code review.
IMO if it's not mandatory I don't think it should be said at all in a code review.
I absolutely get that. I was a bit in a hurry when I wrote the above comment, and therefore didn't get around to better detail how I use those sentences.
It's all about understanding the context and not talking down to the author.
When I see obvious improvements I use the "order" style and just state what needs to change. Because surely the author can see that the change will be for the better.
The other sentences are for bigger more complex changes where I'm curious about a certain decision. Most often I don't agree on the current design, but I would never write such a comment unless I'm ready to be convinced otherwise.
When someone has spend hours on some functionality and I spend just 5-10 minutes skimming through the code, there is a very real chance that the change I'm going to propose was already considered. It would be arrogant and condescending to just conclude that my idea is better, so I open the door for the authors explanation.
It's all about understanding the context and not talking down to the author.
When I see obvious improvements I use the "order" style and just state what needs to change. Because surely the author can see that the change will be for the better.
The other sentences are for bigger more complex changes where I'm curious about a certain decision. Most often I don't agree on the current design, but I would never write such a comment unless I'm ready to be convinced otherwise.
When someone has spend hours on some functionality and I spend just 5-10 minutes skimming through the code, there is a very real chance that the change I'm going to propose was already considered. It would be arrogant and condescending to just conclude that my idea is better, so I open the door for the authors explanation.
I always try to make suggestions as questions because it's less threatening, gives more agency to the author, and oftentimes ends up being a discussion on the merits. If the suggestion really is a blocker, I'll be a little more direct, but always always friendly.
Personally I have two tones I take in a code review comment: the "Should ... ?" form for when I want to raise awareness but otherwise leave it up to the other person, and the "I recommend ..." form for when I think there's a real issue. Especially in the latter case, I'll back it up with a real-world scenario of how it could cause a problem; either a bug, difficulty reading the code by future maintainers, or making it more likely to introduce a bug when the code gets refactored later. Finally, back-linking to documented standards if necessary to avoid the appearance of a me-against-you type of thing.
Personally, I think that the focus on on the phrasing itself, which as you mentioned are all more or less synonymous anyway, is letting the underlying challenge get away from you. You can safely use any of them, but that there's other stuff that can be done that's more important.
Any time you can convincingly explain exactly why you're suggesting something it makes the suggestion itself seem much more reasonable, and it's a valuable check on your own reasoning. From there, you can reword the explanation itself in creative and valuable ways to give feedback. If you get really good at this, then a lot of times you don't even need to ask for a specific change, you can just turn the problem you're seeing (I.e. 'explanation' from above) into a question. I'll give an example.
Let's say you want to say "Extract this to a separate function". Ok, now, ask yourself why? So you tell yourself, "Because we may want to use part of this function again, but not the rest of it, and the exact same behavior can be achieved with little to no additional effort by function composition (or whatever you do in your PL)" Ok, that seems reasonable, but maybe now it may be perceived as a little too abstract to someone else, especially if they're just trying to get their work done. But you can take solve that by taking the above 'explanation' and make it concrete. One good way to do this is to think of a counter example, where it clearly doesn't work how we want.
As an example, let's just pretend that they're doing something like parsing a thing from a database into some strongly typed thing, and then doing foo work on the strongly typed thing. You can ask yourself then, well if we were unit testing this, we won't care about the parsing part, we can just start with the strongly typed thing as the precondition of our test, and then isolate what we're testing to just be the foo functionality. Now you have a simple example that you can rephrase into your original feedback.
This thought exercise all leads to the feedback going from "extract this to a separate function" to "Does this mean that unit testing the foo feature requires a parsing step for each test case?"
Maybe that's a silly, contrived example, but I've found that if you even capable of going through this thought process then not only do people receive your feedback much better, but also you just tend to think of much better feedback altogether. After a lot of practice you end up being able to think this way very quickly and then you can focus on adjusting the wording based on your relationship with that person.
At the end of the day, it's not what we say that really matters, it's how someone feels they're being regarded by one their peers. If they sense contempt, then it won't matter how politely or expertly you word things. Having extremely well thought out why's in your back pocket, and being able to provide concrete counter examples just makes it clear you're not wielding your review power just to talk but to illuminate the problem to them. It also solves the problem of when you're the idiot (as happens to all of us from time to time), by forcing you to actually justify your feedback before you share it.
When I've done this, I've noticed that my reviews become faster, and I focus on stuff that people don't think about. It makes the code better and then people will actually seek out your review.
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.
The different ways you're expressing the question give different meanings to your review. It's not clear to me what you want: are you blocking the review on extracting the code? Does this have to be done now or could this be on a separate patch? After or before the series? Is there an actual good reason to extract it (like you plan on using it tomorrow) or is this just about making the code easier to read?
- Try to make this not personal (opposite of how I started the paragraph above!). Talk about the code, not about what the author did. The code does this, the code does that, instead of you wrote this, you wrote that. Also, it's our code, so "we should do this" instead of "you should do that" in case you can't use "the code should do that".
- Be clear on what's just nice-to-have and what is really blocking the review process.
- Explain to them why you think this should be extracted to a separate function. Maybe they have a good reason against it or a good counter argument.
- 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.
Should we,
Could we please
Could we please
Perhaps, silence would be a good choice on this issue?
To my view, coding is as much art as science, and it is limitlessly fascinating to me how different people find different ways of expressing ideas and solutions.
Also, the code review process is fraught with opportunities for insult, misunderstanding and unfortunate power dynamics. It is inherently difficult regardless of the actual content being reviewed.
On the other hand, if there is a significant issue here "I am having trouble following your thinking here. Perhaps dividing this up into smaller functions would help?" Might be a good review comment?
The style wars are very tempting to engage in, but they virtually never drive greater productivity, real quality improvements, or positive team dynamics.
If there are real reasons that this wants to be broken out into a separate function (reusability for instance), then make that clear in your suggestion.
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.
Static Analysis should be able to provide a "complexity" metric. If the method reaches a certain complexity, certain tools will consider it a bug. Refactoring is sometimes the only way to address these issues.
Remember to be problem oriented rather than solution oriented. If you just tell people "I think this should be the solution" or "you should change it to this solution", you aren't working with them, you're instructing them. And a key to teamwork, especially reviewing, is a cooperative spirit, not an instructional one.
If instead of saying what you would do, or what you think they should do, you express a concern about the code, the tone is completely different. Now it's just saying something like "do you think repeating this across files will be problematic?", which allows the other person to be the determiner. If they agree they may well do exactly what you would have suggested, and if they don't, it can start a discussion that helps bridge together how various team members code. It's win/win.
It also helps people understand what you aim for when writing code, which helps them keep in mind your needs in the future (and vice-versa for you to keep in mind their needs).
If instead of saying what you would do, or what you think they should do, you express a concern about the code, the tone is completely different. Now it's just saying something like "do you think repeating this across files will be problematic?", which allows the other person to be the determiner. If they agree they may well do exactly what you would have suggested, and if they don't, it can start a discussion that helps bridge together how various team members code. It's win/win.
It also helps people understand what you aim for when writing code, which helps them keep in mind your needs in the future (and vice-versa for you to keep in mind their needs).
> "do you think repeating this across files will be problematic?"
If you write that you clearly think it's problematic but hide it behind an unnatural question. Then either I do what you hint at or you will push / discuss until I agree. I am not the determiner. It sounds like you are schooling me with an awkward back and forth.
Honestly, to me this is beating around the bush and doesn't actually make me confident we can have a real discussion (in the context of that code review).
"Do you perhaps think that doing this <tylically discouraged coding practice> is problematic?" sounds like either you want to passively insult me, or you don't consider me a peer with whom to have an actual discussion on equal footing, or you don't have the guts to share your opinion without contortions.
If you write that you clearly think it's problematic but hide it behind an unnatural question. Then either I do what you hint at or you will push / discuss until I agree. I am not the determiner. It sounds like you are schooling me with an awkward back and forth.
Honestly, to me this is beating around the bush and doesn't actually make me confident we can have a real discussion (in the context of that code review).
"Do you perhaps think that doing this <tylically discouraged coding practice> is problematic?" sounds like either you want to passively insult me, or you don't consider me a peer with whom to have an actual discussion on equal footing, or you don't have the guts to share your opinion without contortions.
The wording above could be improved for sure. But the intention is to shift focus away from telling somehow how to solve a problem to forming consensus on whether there's a problem at all. And that starts with the reviewer expressing a concern rather than immediately providing a solution. "Is a change needed?" should come before "this is a needed change".
A better example might be "I'm seeing this same code in various places--do we benefit from keeping them distinct?" or "you took an inheritance approach here, but I'm concerned about the long-term impacts as the codebase changes. How do you think this code compares with a composition approach?".
If in doing so you would think that I'm being sly, that's unfortunate, but there's not much I can do in how you interpret stuff. I can't control for others assuming bad faith. And if there's bad faith, there are probably communication and/or trust problems amongst the team members that are broader than code reviews, and is something that should be tackled directly.
A better example might be "I'm seeing this same code in various places--do we benefit from keeping them distinct?" or "you took an inheritance approach here, but I'm concerned about the long-term impacts as the codebase changes. How do you think this code compares with a composition approach?".
If in doing so you would think that I'm being sly, that's unfortunate, but there's not much I can do in how you interpret stuff. I can't control for others assuming bad faith. And if there's bad faith, there are probably communication and/or trust problems amongst the team members that are broader than code reviews, and is something that should be tackled directly.
My favorite guide on this is https://phauer.com/2018/code-review-guidelines/
There are also some great things in the book, Debugging Teams, like the Humility-Respect-Trust (HRT) concept.
There are also some great things in the book, Debugging Teams, like the Humility-Respect-Trust (HRT) concept.
How about
* Why isn’t this extracted to a separate function?
* Why isn’t this extracted to a separate function?
It makes the assumption the reviewer is correct and shuts down any chance for a reasonable dialog.
Use number #6. If it's not necessary, then why say it at all in a code review? Everyones got other things they'd like to do.
All the "should" and "considers" are tickets that can go on the backlog after a merge has delivered functionality.
Then YAGNI will probably turn out to apply.
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.
All of this should be hammered out in a set of guidelines around code review so that ego/feelings don't need to come into play. Have style guides, code review checklists, and best practices documented.
The actual words used should be determined by the goal of code review and the relationship between reviewers.
Anything that can possibly be automated by CI should be. Style guide and test coverage should automatically fail PRs before a human needs to add any "nit"
That means code review should be a teaching or question moment (between a senior and junior, newer and veteran) or a discussion between peers.
From a junior to a senior -> why did you extract this? why didn't you do it this way?
From a senior to a junior -> typically, I would extract this because X. Check our best practices doc for a longer explanation.
Between peers or senior/junior (maybe one is newer) -> it isn't in our coding best practices document, but normally we extract this because X
Between peers -> what do you think about extracting this because X? Should this be in the coding best practices document?
The last is important because ideally you're updating your coding guide so this same review doesn't need to be done 10 times. And, if you don't think it should be documented as a best practice then don't bring it up in a review.
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's not a lot you can contribute unless you worked on the ticket. Notice how the only example you came up with was refactoring.
This was more important back in the day but now linters and formatters are mainstream.
The best time to code-review is at the beginning.
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.
The emphasis should be on clarity. Overall you want to leave the requester in no doubt about what it will take to get approval because the most frustrating thing for both of you will be ping pong.
Tone is very hard to judge in written form particularly when there might be cultural and language divides, in my experience though it doesn't really matter what style you use provided that you also remember to comment on things that are good and do not need changing. This is routinely forgotten and more than balances any perception of negative tone the requester might inadvertently infer.
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.
There are three main categories of code reviews:
1.) Things are mainly bad.
2.) Things are mainly good.
3.) Somewhere in between.
Within those, everything I see and flag as worthy of discussion is either:
1.) Something that violates our rules/style.
2.) Something I have a strong opinion on.
3.) Something I don’t understand.
Depending on which main category the review falls into, I’ll deal with these differently. If things are mostly bad and I don’t understand something, I’ll ask a lot of questions about readability and naming conventions. If something is mostly good and I don’t understand something, I’ll ask “Hey, can you explain lines 71-104?”
In general, I try to keep things friendly and positive because I don’t want people to dread the experience. Life is too short to dread a big part of your job.
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.
Review: a formal assessment or examination of something with the possibility or intention of instituting change if necessary.
None of your examples provide feedback as to why you want a change. That may be why you have been led to ask this question.
Consider:
* There is a potential overflow in this code. The library function xyz already does this and can log when the app is in debugging mode.
* This portion duplicates the same set of processes as over there. Refactor these into a single function, and we'll be good to go.
* While I don't have feedback yet on this function, it's too long for me to follow. It would be easier to read and for future maintenance if you refactor this part into a function.
* Since we're in closedown we can only take certain types of changes. If you refactor this into a separate function in the library, this change can be accepted.
* Or, I hate to be the process person, but the internal guidelines for this team call for all code to be structured the same way. Refactor this part into a separate function and I'll approve the PR.
There are lots of ways to provide feedback. I suggest stating the problem with the code and providing a solution. If that's the only possible solution to get past your review, state and don't ask. You can also give a carrot with "do this and I'll approve the merge."
How would you speak when sitting next to the person face-to-face? What tone do you want your boss to use when providing a performance review during a 1on1?
None of your examples provide feedback as to why you want a change. That may be why you have been led to ask this question.
Consider:
* There is a potential overflow in this code. The library function xyz already does this and can log when the app is in debugging mode.
* This portion duplicates the same set of processes as over there. Refactor these into a single function, and we'll be good to go.
* While I don't have feedback yet on this function, it's too long for me to follow. It would be easier to read and for future maintenance if you refactor this part into a function.
* Since we're in closedown we can only take certain types of changes. If you refactor this into a separate function in the library, this change can be accepted.
* Or, I hate to be the process person, but the internal guidelines for this team call for all code to be structured the same way. Refactor this part into a separate function and I'll approve the PR.
There are lots of ways to provide feedback. I suggest stating the problem with the code and providing a solution. If that's the only possible solution to get past your review, state and don't ask. You can also give a carrot with "do this and I'll approve the merge."
How would you speak when sitting next to the person face-to-face? What tone do you want your boss to use when providing a performance review during a 1on1?
I agree with this. Don't give demands, give reasoning and examples.
Just wanted to chime in and say that I still remember the first code reviews I received and while I didn't take any of the comments personally, they certainly didn't feel great to receive. (I should add that I don't begrudge the reviewers, they were nice people).
These example notes are wonderful. They feel like an editor's notes, not a graded exam. Something you'd get from a colleague who is collaborating with you and not some infallible black-box oracle.
These example notes are wonderful. They feel like an editor's notes, not a graded exam. Something you'd get from a colleague who is collaborating with you and not some infallible black-box oracle.
Thanks!
It's your standard Socratic Method[1] that's devised to help you (the learner) arrive at solutions on your own terms. Just don't push it too deep - others may become a little too annoyed, and, well, read up on Socrates to learn what happens next
[1] https://en.wikipedia.org/wiki/Socratic_method
[1] https://en.wikipedia.org/wiki/Socratic_method
Agree with adding additional reasoning, regardless of the actual tone.
I’ve received some feedback along the lines of “LOL Wut?” on a comment in code, which I really didn’t know how to take or respond to (suffice to say its meaning was vague, and generally unhelpful).
I’ve received some feedback along the lines of “LOL Wut?” on a comment in code, which I really didn’t know how to take or respond to (suffice to say its meaning was vague, and generally unhelpful).
Sometimes you catch people in low energy or bad moods. It doesn't excuse the behaviour but try not to take it personal.
My trick is to write the wtf in pending review then go and look later in the day if it's still valid. Often I just needed to eat something and I'll be over it.
My trick is to write the wtf in pending review then go and look later in the day if it's still valid. Often I just needed to eat something and I'll be over it.
I would talk it out with the person. Such comments aren't helpful at all, unless the mistake made on that line is obvious.
.. and sometimes after going through that exercise you might find you don't have a good reason for what you're asking for. :)
[deleted]
+1 the reasoning is the entire point and all of these examples are good.
Just telling people to make changes without telling why doesn't teach them anything - especially when sometimes what's being requested isn't actually better (or is at least subjective).
Just telling people to make changes without telling why doesn't teach them anything - especially when sometimes what's being requested isn't actually better (or is at least subjective).
my rule is to weigh whether or not its worth making a comment very carefully. nothing without a clear justification. never say 'I would have done it this other way' - unless its a good friend who you often discuss style issues with. comment on things that you feel would make a substantial difference on schedule, maintainability, or function - and provide a clear reasoning with which to argue against.
wasting your peers time trying to enforce your style isn't just non-productive, it sours everyone on the whole exercise.
wasting your peers time trying to enforce your style isn't just non-productive, it sours everyone on the whole exercise.
Where possible it can also be useful to point out if there are existing guidelines/checklists the submitter should be referencing to understand these preferences of the project. The more the contributors can code review themselves prior to submitting a PR the better.
And if such a resource doesn't exist or that topic is not in the current guidelines/checklists - offering to take up the task of creating or adding an topic to the list of things to check before submitting a PR can be helpful.
Eliminating nitpicky PR comments by having pretty verbose coding guidelines as well as pretty strict settings for automatic code linters/syntax checkers/static type validators means the PR feedback cycle is a lot more focused.
And for architectural discussion - draft PRs can be useful early on for feedback on design choices for more challenging features can be helpful.
And if such a resource doesn't exist or that topic is not in the current guidelines/checklists - offering to take up the task of creating or adding an topic to the list of things to check before submitting a PR can be helpful.
Eliminating nitpicky PR comments by having pretty verbose coding guidelines as well as pretty strict settings for automatic code linters/syntax checkers/static type validators means the PR feedback cycle is a lot more focused.
And for architectural discussion - draft PRs can be useful early on for feedback on design choices for more challenging features can be helpful.
This!
Walking on eggshells because every comment chain devolves into religious battles about style? Find a linter or style checker that can automate comments/fixes. Talk about those checks in your team meeting, get consensus (who cares what it is, just consensus), and make the tools enforce it. Or suggest your manager do this.
If discussing actual code problems in code reviews rankles people, just escalate to your manager.
If you need a special code review human language lexicon because people can faint or throw punches, it's a management problem. Because that's the kind of thing that makes other people, not the troublemakers, leave the company.
Walking on eggshells because every comment chain devolves into religious battles about style? Find a linter or style checker that can automate comments/fixes. Talk about those checks in your team meeting, get consensus (who cares what it is, just consensus), and make the tools enforce it. Or suggest your manager do this.
If discussing actual code problems in code reviews rankles people, just escalate to your manager.
If you need a special code review human language lexicon because people can faint or throw punches, it's a management problem. Because that's the kind of thing that makes other people, not the troublemakers, leave the company.
I ignore style check nitpick comments.
Fix your linter and autoformatter.
You don't pay me to lint code.
Fix your linter and autoformatter.
You don't pay me to lint code.
I've long wanted to write a blog post on applying what I learned from effective communications books to code reviews.
Your comment mirrored something I wrote in another thread about the problems with the Socratic method in general[1]:
"If you have a concern, then express the concern openly before asking your question. This will make it clear to the recipient what your intent is, and they will not have to guess."
The worst comment I see in code reviews (and sadly, all too often) is:
"Why did you do it this way?"
I have no idea why I'm being asked this. The answer is "Because it solved the problem."
Even this is problematic:
"Why did you do it this way instead of X?"
Possible (unhelpful) answers:
"Because I didn't think of X." (I still don't know if you want me to change the code and why).
"Because it solved the problem."
Your examples are good ones on how to ask this question "This could have been solved via X, which has the benefits Y and Z compared to your approach. I suggest changing this to use X, unless your approach has advantages that I'm unaware of."
Probably about half the times I get something like this: Yes, my method did have advantages the reviewer is not aware of, and we then have a discussion.
[1] https://news.ycombinator.com/item?id=31889518
Your comment mirrored something I wrote in another thread about the problems with the Socratic method in general[1]:
"If you have a concern, then express the concern openly before asking your question. This will make it clear to the recipient what your intent is, and they will not have to guess."
The worst comment I see in code reviews (and sadly, all too often) is:
"Why did you do it this way?"
I have no idea why I'm being asked this. The answer is "Because it solved the problem."
Even this is problematic:
"Why did you do it this way instead of X?"
Possible (unhelpful) answers:
"Because I didn't think of X." (I still don't know if you want me to change the code and why).
"Because it solved the problem."
Your examples are good ones on how to ask this question "This could have been solved via X, which has the benefits Y and Z compared to your approach. I suggest changing this to use X, unless your approach has advantages that I'm unaware of."
Probably about half the times I get something like this: Yes, my method did have advantages the reviewer is not aware of, and we then have a discussion.
[1] https://news.ycombinator.com/item?id=31889518
> "Why did you do it this way?"
While the question is literally (and without any other thought) what I have sometimes in mind, usually I ask something like:
I feel I might be missing some insight or context, could you walk me through your rationale?
Because if I don't get some code change I may very well be missing something. And in any case by talking we're only going to improve our mutual understanding, both of the codebase and of each other's thought process.
While the question is literally (and without any other thought) what I have sometimes in mind, usually I ask something like:
I feel I might be missing some insight or context, could you walk me through your rationale?
Because if I don't get some code change I may very well be missing something. And in any case by talking we're only going to improve our mutual understanding, both of the codebase and of each other's thought process.
> Why did you do it this way instead of X?
Argh, yes, that's one line that might make me just walk away from a PR as a new/junior/casual contributor.
You, the reviewer, are an expert in the system. Likely you are the or one of the most expert people in the entire world on this exact thing. You know X exists and why to use it. As you should, because you put it there. You also should know that people who aren't experts (like me) don't know about it, simply because they didn't use it, in this PR, when they should have. Why don't they know it? Probably because you haven't used it consistently in your own code, or it's not documented. This newbie has cobbled this PR together from what sense I can make of this project. Probably 90% is guessed from code I found in there already.
What wouldn't wind me up?
"I think a better way to do this is X. It's better because Y. Or have I missed a specific reason for X?"
Note two things: 1) explanation to a noob of reason Y, which may well be valuable, not only to the noob, but also in the record of the project in general. 2) The indication that the noob might at least have had a logical approach, and they're not an idiot, just a noob.
Afterwards, if this seems like something the noob should have known from the codebase, consider that you, the maintainer, have failed to make it clear.
Of course, if I'm also supposed to be an expert, e.g. a co-maintainer, then it's different. I should know X. Which means either it's a brain fart, or I actually do have a reason. In which case I should have commented on the code, because if another contributor can't tell the intention in the PR, then they can't tell in a year when no one can remember why it went that way.
Argh, yes, that's one line that might make me just walk away from a PR as a new/junior/casual contributor.
You, the reviewer, are an expert in the system. Likely you are the or one of the most expert people in the entire world on this exact thing. You know X exists and why to use it. As you should, because you put it there. You also should know that people who aren't experts (like me) don't know about it, simply because they didn't use it, in this PR, when they should have. Why don't they know it? Probably because you haven't used it consistently in your own code, or it's not documented. This newbie has cobbled this PR together from what sense I can make of this project. Probably 90% is guessed from code I found in there already.
What wouldn't wind me up?
"I think a better way to do this is X. It's better because Y. Or have I missed a specific reason for X?"
Note two things: 1) explanation to a noob of reason Y, which may well be valuable, not only to the noob, but also in the record of the project in general. 2) The indication that the noob might at least have had a logical approach, and they're not an idiot, just a noob.
Afterwards, if this seems like something the noob should have known from the codebase, consider that you, the maintainer, have failed to make it clear.
Of course, if I'm also supposed to be an expert, e.g. a co-maintainer, then it's different. I should know X. Which means either it's a brain fart, or I actually do have a reason. In which case I should have commented on the code, because if another contributor can't tell the intention in the PR, then they can't tell in a year when no one can remember why it went that way.
> You, the reviewer, are an expert in the system. ... You also should know that people who aren't experts (like me) don't know about it, simply because they didn't use it, in this PR, when they should have.
While this happens, I find that it's very common that even a junior developer has some insights the expert reviewer doesn't by virtue of the fact that he/she has spent more time on this specific problem than the reviewer has. As a reviewer, it's always better to assume you are not the expert.
> Of course, if I'm also supposed to be an expert, e.g. a co-maintainer, then it's different. I should know X. Which means either it's a brain fart, or I actually do have a reason. In which case I should have commented on the code, because if another contributor can't tell the intention in the PR, then they can't tell in a year when no one can remember why it went that way.
I disagree, and your example is a good case of not considering all the possibilities.
Yes, in certain cases, X may be the obviously better approach, and a comment would be a good idea. In many/most cases, there are N approaches, and you happened to pick one. If your approach has advantages over X, I don't see a need to justify it as a comment in the code because you then should put comments to justify it over the (N - 2) other solutions.
At the end of the day, the burden is on the reviewer to specify why he prefers X. Only if he presents a case ("X is better because of reason Y") should the developer feel obliged to justify the decision.
While this happens, I find that it's very common that even a junior developer has some insights the expert reviewer doesn't by virtue of the fact that he/she has spent more time on this specific problem than the reviewer has. As a reviewer, it's always better to assume you are not the expert.
> Of course, if I'm also supposed to be an expert, e.g. a co-maintainer, then it's different. I should know X. Which means either it's a brain fart, or I actually do have a reason. In which case I should have commented on the code, because if another contributor can't tell the intention in the PR, then they can't tell in a year when no one can remember why it went that way.
I disagree, and your example is a good case of not considering all the possibilities.
Yes, in certain cases, X may be the obviously better approach, and a comment would be a good idea. In many/most cases, there are N approaches, and you happened to pick one. If your approach has advantages over X, I don't see a need to justify it as a comment in the code because you then should put comments to justify it over the (N - 2) other solutions.
At the end of the day, the burden is on the reviewer to specify why he prefers X. Only if he presents a case ("X is better because of reason Y") should the developer feel obliged to justify the decision.
> In many/most cases, there are N approaches, and you happened to pick one.
I'm not sure that's always true. A lot of the time, while there might be, in a vacuum, many ways, in the context of a specific project (or part of), there's usually one "most right" way to do it, considering local style, idioms, norms and conventions. Admittedly, this might be more true in some languages and applications.
Sometimes there are multiple ways. Usually this means that one or more disparate legacy ways are extant, but there's a preferred way to move towards.
Having PRs storm off in "exciting" new directions, technically correct or not, is usually a bad idea once that merges and is now everyone else's problem.
If all alternatives are equally valid logically, it's likely they have practical implications that differ. Perhaps one might consider a std::set better than a std:: vector in the abstract for your task, but maybe you know something important about the expected number of items or memory access patterns. This is when you should comment if that's not obvious to someone who isn't literally just done writing the change.
> At the end of the day, the burden is on the reviewer to specify why he prefers X.
If a reviewer is going to pull me up on truly equivalent things that have no impact on correctness or maintainability, then I'm probably not going to voluntarily be a co-maintainer with them. If I've consented to be a co-maintainer, then I have either mutual respect for the others, or I'm being paid enough to make it worth it. And indeed, I have refused maintainership offers in projects with ":eyeroll: why didn't you just magically intuit X" cultures.
I'm not sure that's always true. A lot of the time, while there might be, in a vacuum, many ways, in the context of a specific project (or part of), there's usually one "most right" way to do it, considering local style, idioms, norms and conventions. Admittedly, this might be more true in some languages and applications.
Sometimes there are multiple ways. Usually this means that one or more disparate legacy ways are extant, but there's a preferred way to move towards.
Having PRs storm off in "exciting" new directions, technically correct or not, is usually a bad idea once that merges and is now everyone else's problem.
If all alternatives are equally valid logically, it's likely they have practical implications that differ. Perhaps one might consider a std::set better than a std:: vector in the abstract for your task, but maybe you know something important about the expected number of items or memory access patterns. This is when you should comment if that's not obvious to someone who isn't literally just done writing the change.
> At the end of the day, the burden is on the reviewer to specify why he prefers X.
If a reviewer is going to pull me up on truly equivalent things that have no impact on correctness or maintainability, then I'm probably not going to voluntarily be a co-maintainer with them. If I've consented to be a co-maintainer, then I have either mutual respect for the others, or I'm being paid enough to make it worth it. And indeed, I have refused maintainership offers in projects with ":eyeroll: why didn't you just magically intuit X" cultures.
Seeing an open source project's reviewer begin with "Why did you do it this way instead of X?" is the the most shocking shutdown I have seen this year.
Buddy, you let that issue languish for a year. How about let my junior dev friend have a go without being a nitwit reviewer?
Buddy, you let that issue languish for a year. How about let my junior dev friend have a go without being a nitwit reviewer?
> Seeing an open source project's reviewer begin with "Why did you do it this way instead of X?" is the the most shocking shutdown I have seen this year.
Thing is, in most cases it's not meant to be nasty at all. Their intent usually is to point out they think there's a better way, and are either trying to say you should do it that way, or are trying to invite you to give your reasons. The problem is they're phrasing it in a manner where there are multiple interpretations to their intent.
Given that commenters responded negatively to it - beyond what I had originally meant when I highlighted it - makes it a good example of how not to communicate.
Thing is, in most cases it's not meant to be nasty at all. Their intent usually is to point out they think there's a better way, and are either trying to say you should do it that way, or are trying to invite you to give your reasons. The problem is they're phrasing it in a manner where there are multiple interpretations to their intent.
Given that commenters responded negatively to it - beyond what I had originally meant when I highlighted it - makes it a good example of how not to communicate.
Yep: the operative word being “because”, absent in all the OP’s sentences.
I agree with the tone of your suggestions, but your suggestions still take quite some time to read. They can easily be shortened without losing any information. For example, no need to to apologize for being the process person (being overly apologetic decreases the value of apologies anyway).
Also for me it feels like you're seeing code reviews as a senior/junior thing. It's more than that, no?
Also for me it feels like you're seeing code reviews as a senior/junior thing. It's more than that, no?
These work and sound great, but stating them without a "please" can come off as a little brusque. Please add a "please" before the orders, and I'll accept the advice.
These suggestions hit the nail on the head. Additionally, the reviewer also becomes a better dev because they’re forced to really think about and articulate why they think the code smells.
Even if my coworker’s code doesn’t feel right to me, I’ll sometimes forgo commenting if I can’t articulate why. My goal is to avoid bugs and deliver features, not to force my code style and opinions on others.
Even if my coworker’s code doesn’t feel right to me, I’ll sometimes forgo commenting if I can’t articulate why. My goal is to avoid bugs and deliver features, not to force my code style and opinions on others.
The original post seemed to be concerned about tone, not content.
That’s the point: the replies are saying that the content is more of an issue than the tone, and so OP should focus on the hints for the former.
How do we know the OP had any issues coming up with valuable and insightful comments? Tone can have an impact - being particularly rude is not likely to result in good outcomes (but maybe it could in some situations). Why aren't we exploring what was originally asked?
Because all of the versions have the same problem regardless of tone. If the assumptions are in error and the OP had been leaving off the justifications suggested, they can reply and say so! (But, as it turns out, didn’t.)
collaborative learning for all tone
I think setting team goals are important before anything. For example if the goal is to create the most maintainable and best version of the code in the long term I think this opens up doors for feedback not being taken personally as long as the person giving the feedback isn't dropping lines like "why didn't you just do xyz instead?".
I tend to provide more open ended questions like "what do you think about ..." where "..." could be a few options to take. In my mind I've most likely picked one based on personal preference but I like proposing a few choices to spark some discussion amongst team mates and give everyone a chance to contribute. Often times throwing out a few options will result in someone coming up with another option that only became apparent after they've seen a few alternatives (which is a good thing).
It also lets more folks make decisions and that's a very important thing. If you keep making every decision then folks won't feel like they can contribute anything because "why bother". I remember an old TNG episode (S3E21) around this with "Broccoli" when he was trying to integrate on the engineering team but Jordi kept alienating him. Picard dropped some really good life advice in that episode!
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!
I do code reviews as the meat of my job and have personally benefited from GitLab's handbook article on this topic [1]
> 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...
- 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...
I agree with "Don't defend a point to win an argument or double-down on a mistake.". I disagree with the juxtaposition that ego is bad, i.e. "no ego". Ego is defined by Oxford as "a person's sense of self-esteem or self-importance.", I agree.
Ego isn't bad. To use it in a context directly indicating it's bad is wrong. If the world agrees to do away with the word ego then it must be replaced by something that also means "a person's sense of self-esteem or self-importance.".
A coder without any ego is a coder who doesn't care, at all, about anything.
Ego isn't bad. To use it in a context directly indicating it's bad is wrong. If the world agrees to do away with the word ego then it must be replaced by something that also means "a person's sense of self-esteem or self-importance.".
A coder without any ego is a coder who doesn't care, at all, about anything.
It feels unnecessarily pedantic to debate the usage of a word in a common English idiom, especially with an outdated definition from Oxford.
It would be clearer to just say, "don't be an asshole" but that comes off as crass in professional communications.
It would be clearer to just say, "don't be an asshole" but that comes off as crass in professional communications.
On the contrary, HN is the place to debate the usage of words. Precise word choice is how to effectively get your point across. Words should have more definitions than the obvious two ("something universally good" and "something universally stupid, annoying, embarrassing, and smelling like fart") and it's not a shame to talk about subtleties of their definitions.
A bit related - do you ever go from PR comments to DMs? Sometimes when discussions are needed, it feels much easier to just chat 1 on 1. Especially if it's a small team and everyone's close. Though that prevents others from experiencing the "interaction".
If its not just asking for clarification on a comment then I really try to stick to the PR, so that the context is captured. If a back and forth is needed then I make sure to include a recap somewhere in the PR.
Obviously tone is hard in text, and it's worse in multilingual teams. What's been very effective for my current team is adding explicit context as a comment prefix with standardised terms:
- Nit for "it doesn't actually matter"
- Suggestion for "this approach might be better, what do you think?"
- Question for "help me understand this"
- Requested Change for "this is actually a blocker"
We include this expectation in our working agreement, stick to it rigorously for reviews where reviewer and reviewee haven't established a strong rapport yet, and use it as appropriate beyond that. So far we've had no confusion about tone in reviews.
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.
Default to short, polite statements, but make it clearly suggestive if its optional.
e.g. "Extract this to a separate function please" if you aren't asking. "Should this be a separate function" otherwise.
This also assumes that theres a certain protocol to PRs in terms of veto power. I've always followed the rule that the writer must make all requested updates by reviewer unless explicitly challenged. In other words, you can't silently not change something they mentioned, but you can say things like "thats out of scope for this update" or "I think this design is better because XYZ" and the writer is able to mark as resolved at that point.
Sometimes this ends up with a conversation that needs to happen where the reviewer disagrees with the writer, but honestly it rarely does at least on the teams I've been on. It would really have to be a contentious point to escalate to that level rather than just being a difference of opinion.
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.
"How would you feel about extracting this into a separate function? It might be better because the logic seems quite self-contained, and we can then apply the extra business logic immediately after this with a few if-elses."
I try to give critiques in this way, as it gives the person that wrote the code ample opportunity to defend their approach, but also gives explanation of why your suggestion might be better.
I 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.
> "Extract this to a separate function please"
This sounds really bad. Even if you’re their boss. You’re dealing with professionals and part of that is considering their input.
I’d start with the assumption that they know what they’re doing and might have had a good reason and phrase it from there. Even if not true it sets the right tone of respect.
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.
While some people don't like hearing it: It is also partly up to the author of the code to ensure that reviews are handled professionally.
If you're fluent in English, there's a clear difference in the tone of the listed comment forms. If you're working with colleagues that may struggle a little with English, you can easily read to much into the tone of a code review. The reviewer may not mean to come of direct or even aggressive, but because they literally just translated directly from German or Finnish the tone will be way off.
It's also up to the author of the code to assume good faith, and look beyond the language used. Try to be kind when reviewing the code of others, and assume that the reviewers is trying to help.
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.
Your examples lack a reasoning or explanation as to why code should be abstracted to a separate function.
To a senior dev they might not need an explanation to justify the action of extracting the coded. It could just be oversight.
But to junior and intermediate you might need to explain the reasoning for requesting code to be abstracted.
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.
Some of my coworkers have a convention of prefixing messages with [request], [suggestion], [question], etc. in order to make the intent extremely explicit. This is particularly useful if you have people coming from multiple cultures with different levels of forthrightness.
I like, "I would extract this to a separate function."
Unless you're asking a question, posing it as a question comes off as passive-aggressive, even if it's not what you intended. A simple declarative statement of, "I would do it this way", is probably best. You don't know why he did it the other way, and he might have had a very good reason (for example, to be sure the subroutine would be inlined).
The best tone to use is one that is neutral and efficient; prefer active voice but avoid imperative mood unless you're 100% sure you're right, in which case it's usually fine.
It probably goes without saying, but you should assume the change was made in good faith. It's possible that the other programmer hasn't seen before what you consider to be good practices; it's also possible that she's great at her job but just made a mistake. It's also possible that you're wrong. So, for this reason, it's always best to be conservative with tone and stay as factual as you can.
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.
I’d say the last two are best, but I’m Dutch.
I second guess myself and edit a lot, especially relative to how much we actually do code review, so I'm interested to see others' thoughts.
I would say though that a lot depends on team dynamic & hierarchy. For example, I have no direct reports, so I'm never going to use the imperative as in 'extract this to a separate function' - taken literally, I don't have the authority to tell anyone to do that.
Most often I think I favour 'I would', and give a reason. I.e. I found this a bit surprising, because I would have done it differently.
Along the same lines I also ask (honest) 'is there a particular reason' or 'why not' type questions. Something else seems most obvious to me, but maybe they thought of (even tried) and dismissed that.
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.
Typically I say something like:
“What would be the advantage of using X instead?” If I’m not entirely sure or fine either way.
If I think X is really the way to go I’ll say something like:
“Doing X would have these benefits. The benefit of Y is such and such. So X is preferred for …”
“What would be the advantage of using X instead?” If I’m not entirely sure or fine either way.
If I think X is really the way to go I’ll say something like:
“Doing X would have these benefits. The benefit of Y is such and such. So X is preferred for …”
"I think we should…"
"Maybe it would be better…"
"IMO we should do …, what do you think?"
You really want to be on this person side and help them achieve. That's your job after all
"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
To add to the other points, remember you can also leave positive feedback.
Don't force a compliment for no reason because it will come across as insincere... but if you see something well done/implemented, don't be afraid to call it out.
It helps balance the tone sometimes, and is especially helpful with junior engineers who are well served not just learning what's wrong, but what's right.
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.
Actually reaching a level of team culture where you can be as brief, blunt and to the point as possible is a wonderful thing.
There is no offence to be taken, because the deeply held respect for each other as team mates is already there and taken as a given. You trust each other to know that any comments are addressed directly to the code, rather than to the person behind them. All input is valued because everyone knows the goal is just to make the code better and help each other out, rather than criticise an individual. Discussion can be frank, open, unbiased and non-confrontational.
However, because that takes a long time to develop and is somewhat of a rarity, I've had quite good luck with the phrasing:
"Have you considered X here?"
It has a number of advantages:
* It doesn't imply that an author should have done it a different way
* It doesn't imply that an author has missed something you think they should have covered
* It's a good starter for a conversation, rather than a conflict
* If the answer is "no", it provides a good prompt for an author to come up with a suggestion first
* If the answer is "yes", they can explain why they ended up with the solution they did, which is visible to everyone who wants to look at the PR. That can also be a good prompt to add a comment
There is no offence to be taken, because the deeply held respect for each other as team mates is already there and taken as a given. You trust each other to know that any comments are addressed directly to the code, rather than to the person behind them. All input is valued because everyone knows the goal is just to make the code better and help each other out, rather than criticise an individual. Discussion can be frank, open, unbiased and non-confrontational.
However, because that takes a long time to develop and is somewhat of a rarity, I've had quite good luck with the phrasing:
"Have you considered X here?"
It has a number of advantages:
* It doesn't imply that an author should have done it a different way
* It doesn't imply that an author has missed something you think they should have covered
* It's a good starter for a conversation, rather than a conflict
* If the answer is "no", it provides a good prompt for an author to come up with a suggestion first
* If the answer is "yes", they can explain why they ended up with the solution they did, which is visible to everyone who wants to look at the PR. That can also be a good prompt to add a comment
Another method: prepend "nit: " to comments that improve the code but are not calling for changing the main logic. As a reviewer, you are simply acknowledging that this is a nitpick :)
I keep a running list of articles I found useful over the years. There are many great articles about Code Reviews out there. Consider this a more general answer to your question:
Feedback Ladders: How We Encode Code Reviews at Net-lify
https://www.netlify.com/blog/2020/03/05/feedback-ladders-how...
How to Make Good Code Reviews Better
https://stackoverflow.blog/2019/09/30/how-to-make-good-code-...
Best Practices for Code Review
https://smartbear.com/learn/code-review/best-practices-for-p...
Code Review Guidelines for Humans
https://phauer.com/2018/code-review-guidelines/
Unlearning toxic behaviors in a code review culture
https://medium.com/@sandya.sankarram/unlearning-toxic-behavi...
How to Do Code Reviews Like a Human
https://mtlynch.io/human-code-reviews-2/
Stop Nitpicking in Code Reviews
https://blog.danlew.net/2021/02/23/stop-nitpicking-in-code-r...
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...
I adopt, and appreciate when others adopt a neutral/brutal tone.
"These lines have been repeated a number of places and probably ought to be pulled into a function called something like xyz in module zyq"
Generally your tone doesn't matter as long as it's not mean. "This is the 10th time I've told you declare constants FOR EVERY MAGIC NUMBER," does not belong in a code review; it belongs in feedback.
However there is something a bit dangling here which is linting and static analysis. If code has been repeated and genuinely ought to be pulled into a function, static analysis ought to be able to find this.
If you have already dealt with the broad class of possible change requests that can be caught by static analysis, the nature of the reviews should get a lot more straight forward. It will stay at a higher level and naturally tend toward verbiage like "I wonder if we ..." or, "I was thinking if ..."
In cases where static analysis would not find it for e.g. in the middle of a function doing something, we have several lines of code collecting data for some analytics event which is never repeated, but still confusing; I would say something like "For readability, could we pull the analytics bit into a separate function?"
Generally; if you can use "we" instead of "you" or "I"; I would prefer "we" in any team setting.
"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.
>> There are lots of ways to provide feedback. I suggest stating the problem with the code and providing a solution. If that's the only possible solution to get past your review, state and don't ask. You can also give a carrot with "do this and I'll approve the merge."
Yes. This. Clarity is a prerequisite for tact.
Also, be consistent in your tone and style across reviews in a given project or org -- no matter who you're talking to. For instance, on one development project I was informed that all new tickets should use the language "shall" instead of "should" or "will." Consistent language gives everyone a common reference point and helps keep people from feeling "singled out."
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."
The tone also deeply depends on the culture of the receiver.
Depending on whether their cultural background is North American, Scandinavian, German, French, Italian or Chinese (to pick from the very situations I have faced), and especially if they are more junior, you cannot expect the same outcome from the same feedback, without additional context/care in the communication.
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.
You've done a good job here, but I might suggest stepping back even a bit further. If you want reviews to work, they must be in the spirit of "we all improve and learn together" rather than "bad dog! bad!" The tone you set in why you are doing them, who is involved, how they run makes all the difference in the world. What you want is to create a culture where it is safe to point out something that could have ramifications, and to promote a culture where there can be dialog about what's best for the health of the team and products you support. More often than not though, I've seen these meetings turn into "beatings shall continue until morale improves" sessions.
Just my $0.02.
Just my $0.02.
Lots of good suggestions.
I don't think there are simple answers, and it's something you'll have to figure out separately for every reviewee, and sometimes every review.
You have to consider power dynamics, preexisting relationships, your own time, the importance of the change, and cultural differences.
If the power dynamics are too far apart, the right answer is often "don't". A high-level engineer or architect reviewing a relatively junior coder's patch should not be commenting on anything involving taste or stylistic suggestions. They should be reviewing for functionality, security, compatibility, etc. And only bring up maintainability or performance when it really makes a difference.
Some suboptimal code will get landed as a result, but it's better than creating a culture where the senior's personal preferences are always going to win out. When there's a power imbalance, nuance gets lost—the junior either has to bend to the senior's preferences even when they have a good reason to do things differently, or they have to go to a lot of effort to justify their position. Which is fine in some cases, but it's a waste of time and effort for things that don't matter as much. (This isn't about avoiding hurting people's feelings, by the way, though it does serve that purpose as well.)
I also agree that pointing out positive qualities of a patch can make up for quite a few critical comments. Explicitly constructive criticism (eg sketching out an alternative, or doing some work to provide data or justification for a comment) can also "buy" a smaller amount of more negative-sounding comments.
And of course, if you're swapping patches with someone regularly, there's no need to overthink it: state the problems you see, the things you're not sure about that concern you, the suggestions you have, etc., using as clear and concise language as possible.
In all cases, do not make the reader guess your actual opinion or intention. If you're couching a criticism in gentler language to avoid making them feel bad, that's fine, but don't play games or leave out important pieces. There's always a way to coach or criticize honestly, and you'll do yourself a lot of good by finding it.
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.
The tone of a code review suggestion reflects an underlying assumption about what a code review is (or should be). Often developers disagree on this point. It seems like we fall into one of two camps.
1. Reviewing code is like proofreading a paper. Suggestions are welcome, but there isn't an expectation that all suggestions be followed. The author of the code makes the final decision based on their objectives.
2. A code review is a gate that is closed until all comments have been fully addressed. The author must comply to the liking of all who choose to comment, or they are not authorized to continue.
The tone of review comments will naturally follow from their understanding. Understanding #1 yields comments aimed at persuasion. (Should we, Could we, You might consider) While #2 yields more compulsory language (Change ..., You should). Or passive aggressive language that is meant to be compulsory but softened to sound less aggressive (I would, Could you, shouldn't you).
It is my opinion that understanding #1 is most appropriate. It prevents unnecessary bottlenecks and is more respectful or differing opinions. If developers are peers, than #1 is the understanding the most accurately reflects that.
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.
My usual approach is to point out the problem I'm trying to solve rather than suggest a specific solution. I also like to phrase as a question about practicality. Sometimes the answer to that question is "No", and I want to make sure that even someone very junior is comfortable giving me that answer.
- This code looks pretty similar to this other code; is there a way it can be deduplicated?
- I don't understand this code; I think it's because there are too many conceptual steps. Is there some way to structure it for better readability?
- Is there a way to unit test this specific behavior?
Sometimes this style doesn't work (e.g. correcting a comment that has become incorrect due to the change), and in those cases I usually just say "Please do this." I also try to point out things I like when they are present.
- 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.
I mostly only ever use the form: "This should be ... " with a possible explanation why. We're all mature enough to not take it personally, so I'm not going to beat around the bush by writing a lot of proza.
The imperative of the last example ("Do this") leaves no room for argument (because I might be wrong).
Either that or I ask for clarification of a piece of code: "Why did you do this instead of this?".
Like already stated by someone else; your approach really should depend on the team you're in. I'd take a less direct approach with junior devs who tend to be a bit more insecure (and stubborn).
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).
"if we extract this to another function we can then add a test like this" with an example. Gotta say what the value add is. The value add could just be that it's more like the rest of the codebase! "In the past we usually extract functions like this link optional second link"
Across the different teams and orgs I have worked at, there's always been 1 constant during code reviews: Each comment provided an explanation of why some change was being debated. Every engineer knew the review is not a judgement of their coding prowess. Rather each discussion was purely on the merits of that line of code. Sure, there can be disagreements, but it was never judgemental. Usually folks are amenable when there is good justification to the feedback in my experience. It goes both ways too: I personally don't care if some of my nits are not fixed.
Btw, since I think it is relevant to this discussion, we're working on making code reviews deeper than text based diffs. Check out (DiffLens)[https://github.com/marketplace/difflens] if you're code base is primarily TS, JS and/or CSS to get language aware diffs on your GitHub pull requests. We've found that it makes understanding code changes much easier
Btw, since I think it is relevant to this discussion, we're working on making code reviews deeper than text based diffs. Check out (DiffLens)[https://github.com/marketplace/difflens] if you're code base is primarily TS, JS and/or CSS to get language aware diffs on your GitHub pull requests. We've found that it makes understanding code changes much easier
I find that I rarely use direct commands in code reviews, even ones expressed indirectly (e.g. "can yo do X" instead of "do X").
If there are blockers in a PR, I'll add reasoning for why it is a problem (e.g. "this exposes us to vulnerability X", "in this edge case, what would happen is that ...", "I tested it and appears that there is a bug", etc.) If it's a team-agreed style issue (that isn't caught by the linter), I'd rather say "we use convention so and so in this code base" than "please change this to this style".
The reasoning is that I feel that I work with professionals and while they may need feedback, they don't really need commands. I can make suggestions as to how they can fix the issue I raised, but ultimately it's up to them to fix it and if they find an even better way that I didn't think of, why should I limit their problem solving capabilities artificially?
I find that most people I've worked with use a similar style (more centered around the code than around power dynamics), and those people who used a lot of "please do X" style comments were usually also pretty defensive about their code or ideas, would get uneasy when you changed some of their code ("why did you refactor this? please change it back") etc.
Now there may be some people (inexperienced, or just idiots) who really need some more forceful language, but you shouldn't necessarily start out on that default assumption.
That covers the blocker-style comments. Everything else should probably be done in either a "I'm opening up a discussion here and would be interested in your opinion" or a "this is just a nitpick, feel free to ignore" kind of style.
---
For the specific example, depending on the reasoning for why you might want the change:
- "The linter is complaining that this function is too long and we typically stick to those rules. I think lines 20-30 could be extracted into a separate function, for example."
- "I found it a bit hard to understand this function initially because it does a number of things at once (such as...). Maybe we could split it up?"
- "This section of code is repeated between here and that other function and I think we should keep the implementations in sync, so I would recommend extracting that common logic."
- "We've actually had to do something similar in other situations (see here and here) and your solution seems better than what we came up with before, so what do you think about extracting this functionality into a function and calling it from all those places?"
and so on (and it doesn't need to be as verbose if you know that you have more shared context, for example).
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).
Looks good to me.
The key is to understand the value and importance of what you are suggesting. “Remove this SQL injection.” Vs “consider moving this functionality to the superclass”. One is a must do and not doing it is unacceptable. The other is something that may improve the code, but isn’t hurting anything if you don’t.
Smatbear has a great article on code reviews https://smartbear.com/blog/avoiding-the-politics-of-code-rev...
He has the following suggestions:
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.I think it depends on your teams' expectations for code review. It seems like perhaps your team doesn't define what is acceptable/constructive/valued, etc from reviewers.
Personally I prefer the first one on this list: it gives the author the opportunity to refuse the suggestion. Perhaps its a frivolous request and the PR is already been open for far too long. Maybe the author would prefer to leave the duplication there until there's more justification for extracting it to a function. Who can say?
Not all feedback is necessarily good or useful.
Personally I don't bother with suggestions like this where it's more of a style preference unless there is a style guide to adhere to where "extracting this to a function" would clearly connect to a guideline.
I tend to focus on verifying the that the author did their homework: what evidence/proof did they submit that ensures me the change is correct wrt. specs/requirements/tasks? If that evidence/proof is sufficient that's all I care about: it makes it easier to manage a higher volume of review requests.
I have a pet peeve for suggestions that are "trivial" and based on, "well I would have written it this way," as I don't find them terribly useful most of the time. They rarely affect the specification of the program and their claims to readability/maintainability are often dubious and superficial. Bike shedding is a huge waste of time in the review process. At least when worded with, "Should we..." there's a chance for the author to explain whether they had already considered that or accept the suggestion as helpful and make the change.
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.
Don't forget a big part of this is the dependent on the image of you the recipient has in their head.
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.
Depending on the situation, one of the following three:
> Should we extract this to a separate function?
When you are not familiar with the codebase, potential reuse, and want the submitter to clarify rationale for the approach.
> This should be extracted to a separate function.
When you are the authority but you're open to a discussion. You should include the "why" aspect in this situation.
> Extract this to a separate function.
When you are the authority and there will be no discussion. If you want you can include the "why" but it's a waste of time. You are the law.
The rest are sugar coated versions of the same thing.
"I think...", "I would...", "Maybe we...", are unnecessary additions. Clearly it's what you would do as you're the one giving the review.
If people are too thin-skinned to take direct feedback then they should work on improving that before submitting more code. Life is too short to waste time picking words that try not offend when you can spend that time actually producing something of value.
> 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.
Based on a great article we read almost 15 years ago ("Effective Code Reviews Without the Pain": https://www.developer.com/guides/effective-code-reviews-with...), we start most code review comments with "Did you consider...?" This makes it non-confrontational and non-judgmental, as the author can easily reply that, no, they hadn't considered that and thanks for the suggestion.
Everyone in the company knows it's sort of a gimmick, and we even make fun of it a bit, but even so it still works!
"Did you consider extracting this code into a separate well-named function with clear inputs and outputs, to make the original function smaller and easier to reason about and maintain?"
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?"
As someone on the junior end, the tone is less relevant here than the content, and there is not a lot of content. Why do you want it extracted?
Will we re-use it? Do you just not like blocks of code bigger than X (been given this reason many times)? Do you just feel the need to comment (a lot of devs do and I appreciate it when they are upfront about that rationale and I have done this myself to show that I am paying attention)? Are you concerned about readability?
I need to know for future code reviews, so I can avoid the issue in the future.
Will we re-use it? Do you just not like blocks of code bigger than X (been given this reason many times)? Do you just feel the need to comment (a lot of devs do and I appreciate it when they are upfront about that rationale and I have done this myself to show that I am paying attention)? Are you concerned about readability?
I need to know for future code reviews, so I can avoid the issue in the future.
I like SonarQube for removing ambiguity. If SonarQube is set to fail the build at lower than a certain percentage of coverage or too low of a grade in Maintenance then it gets fixed before the PR even gets filed.
Assuming the code is processed by SonarQube for regular branch builds, which it should be.
Assuming the code is processed by SonarQube for regular branch builds, which it should be.
I think none of those, one that is focused on teaching and explaining your reasoning would be better. You can always do it humbly because the author may have some info you don’t that they came across when implementing, which is why it is the way it is.
People can go ahead mod me down for this, but I honestly don't care at this point. Here's the best ways to avoid this:
1.) Quit doing PRs for feedback. Start doing pair or mob programming. Bonus points if you ditch PRs completely and do https://trunkbaseddevelopment.com/ instead while you're at it. Asynchronous code review via PRs is waste (in the Lean sense). 2.) If you still want PRs despite pairing & mobbing (or you're about to tell me you've never done it, you're team won't and/or whatever insert lame excuse here as to slam/dismiss it without trying), spend a faster 5 min via screen share and audio AND CAMERA! Do the PR like an old skool code review session live/remote + camera, but have the author ANNOTATE for their own notes what you've reviewed when they go back to revise. 80% of human communication is non-verbal - interact w/ voice & camera. Help them learn & build confidence, then you won't worry about your writing tone. 3.)
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.)
I've learned that commands not phrased as such usually aggravate people the most - especially #1 from your list or questions where the one questioning knows the answer and wants to suggest something.
If I don't know how to phrase my comments I just schedule a call and we go over the code together. Much less time spent than on comment ping-pong.
If the code generally works, but other aspects, like maintainability, would benefit from some rework I use the phrase "You can...", e.g. "The second argument to the `map` callback is the element index. You can use it to create this range instead of using an external variable incremented on each step."
The recipient has the possibility, but is not actually forced to do anything, just like with code suggestions. In the vast majority of cases they follow my advice though.
If there's an error or a kludge I use:
-Code suggestions - no comment, so nothing to get upset over and if the recipient is feeling particularly irritable, they can just ignore it.
-Direct language in imperative form, e.g. "make sure that X, otherwise this won't work as intended", "avoid X in Y", "use Z here (documentation link)".
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)".
[deleted]
I'm sure this will piss people off: but almost all of the 'code reviews' I've been a part of were a waste of time. People trying to force stylistic changes to how a program works (OPs post is an example) aren't contributing anything constructive. What is worthwhile is trying to find critical security, logical, or performance bugs and reporting those. If you have taste / style concerns about how someone does something yet their solution is still fine then I'd suggest not wasting peoples time. There is nothing worse than nit picking.
All of these are equivalent if you have showed your team that you trust them and value their work.
If not, then only the first version is acceptable.
If not, then only the first version is acceptable.
Harsh but stick a joke in there so it looks like you aren't a dick.
* This shall be extracted to a separate function.
For code reviews we try to automate as much of the nit-picky things as possible. So for example in our Python code we run checks using: black, isort, flake8, mypy, and pylint.
For reviewing. I try not to use the word "you" as I feel it can make people feel defensive. So for your options above I would like:
Should this be extracted into a separate function?
As a code reviewer I may think it is obvious that it should be in a separate function or something else that seems obvious. But often times the author has already tried that and had issues with doing that. Thus they did it the way it is because of that.
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.
When it comes to feedback, ensure that you give the engineer recieving the feedback the maximum potential to grow to ensure the engineer does a better job next time.
Its not the tone but rather the feedback style according that should be adapted to the knowledge/motivation of the engineer in the situation. For example by using the https://situational.com/blog/the-four-leadership-styles-of-s... herustic:
1: Engineer is junior in the context and insecure/unmotivated: Do X 2: Engineer is junior in the context and motivated/secure: Take the decision for the engineer and explain why 3: Engineer is senior in the context but insecure: Coach by open ended questions: how large of a function do you think is appropriate? 4: Engineer is senior and motivated/secure: From your perspective how should the function be and how should we do this in the future to reach our goals.
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.
Code review is a form of communication, and thus depends very much on culture.
This can both be company culture and origin culture of the developer you're communicating with.
For example, sometimes I write comments like "I don't particularly like this because" plus an explanation, plus "but I don't see any significant better way, so I'm fine with it for now". Some of my colleagues are fine with that, maybe they add a code comment like "TODO: find a way to fix problem X", or they just acknowledge and don't change anything. And then there are developers I work with that come from a different country, and they'll spend another half day or even two days coming up with a better solution, or don't dare to click on "resolve thread".
I think their culture is really geared towards avoiding verbal criticism, and if I even bother to write something negative (and maybe I'm also senior to them, in the org chart), it must be really meaningful and must be addressed.
So for some of these "foreign" developers I only leave comments where I expect something to change; for others that whose culture I better understand, I sometimes leave comments that are more conversational.
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.
Couple things I try to keep in mind when giving PR feedback specifically.
0) Give feedback with respect and just like as if I were sitting with someone face to face telling them these things. And keep it about the code, not the author.
1) Remember that I could always be wrong (whether from misreading something, not having the whole pictures, etc. - many reasons my suggestion may not be the right one).
2) Communicate how strongly I feel about a change - I use `nit:` to indicate opinions or annoyances that are basically "I wouldn't do it this way, but up to you whether to change it", slightly stronger language for things I think require changes, up to "I feel strongly about this, let's chat about it" for things I would not approve the PR before they (or my mind) are changed.
3) Phrase as requests and/or suggestions (could, would, prefer) over command (do X)
4) As others have mentioned, give the reasoning. Personally, I prefer request first, reasoning second, but I see many people expressing preference for it the other way. My reasoning is that, while the reviewee may read the reasoning once, the request is what they may come back for when fixing things, so ensure it is the first thing seen so someone doesn't have to parse through the reasoning again to find the specific request.
5) Provide examples if it's clearer to communicate that way as compared to writing it out.
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’ve introduced Must, Should, Could at every company I’ve been at (except the one where I picked it up myself) and it works wonders.
Prefix every suggestion with M, S or C and then just write the suggested change as a plain statement. The prefix handles the severity and importance without you having to worry about tone.
Coulds can be ignored by the coder author with no explanation as to why they are ignoring but if they have time or want to implement the change then they can. Shoulds can only be ignored with a reason and the reviewer has to agree or the change should be made. Musts have to be implemented and should be used rarely. The only person who can over rule a must is a department head, lead or CTO type person.
This massively reduces friction in the CR process.
Article here: https://allthecode.co/blog/post/how-to-get-better-at-code-re...
Prefix every suggestion with M, S or C and then just write the suggested change as a plain statement. The prefix handles the severity and importance without you having to worry about tone.
Coulds can be ignored by the coder author with no explanation as to why they are ignoring but if they have time or want to implement the change then they can. Shoulds can only be ignored with a reason and the reviewer has to agree or the change should be made. Musts have to be implemented and should be used rarely. The only person who can over rule a must is a department head, lead or CTO type person.
This massively reduces friction in the CR process.
Article here: https://allthecode.co/blog/post/how-to-get-better-at-code-re...
Yeah, that is great.
Learned something similar on a previous job. Prefix it with "#must", "#should", "#could", "#would".
We've also extended it with some other things that you may want to comment, but are not necessarily actionable. "#wont", "#idea", "#tip", "#question", "#risk", are pretty much self-explanatory.
"#fun": an ugly workaround that could've worked; some funny situation that the change could cause; ...
"#domain-knowledge"/"#debrief": really nice if you are working with people that are more junior, or not that familiar with this codebase in particular. May help other reviewers understand better why the author made certain decisions.
Learned something similar on a previous job. Prefix it with "#must", "#should", "#could", "#would".
We've also extended it with some other things that you may want to comment, but are not necessarily actionable. "#wont", "#idea", "#tip", "#question", "#risk", are pretty much self-explanatory.
"#fun": an ugly workaround that could've worked; some funny situation that the change could cause; ...
"#domain-knowledge"/"#debrief": really nice if you are working with people that are more junior, or not that familiar with this codebase in particular. May help other reviewers understand better why the author made certain decisions.
We do something very similar:
* NICE: Optional change. PR is approved.
* SHOULD: Highly suggested change. PR is neither approved nor rejected.
* MUST: Must be fixed. PR is rejected.
This also helps the PR author to know which of my comments have to be addressed to resolve my rejection.
* NICE: Optional change. PR is approved.
* SHOULD: Highly suggested change. PR is neither approved nor rejected.
* MUST: Must be fixed. PR is rejected.
This also helps the PR author to know which of my comments have to be addressed to resolve my rejection.
The word "must" should only be used in the Military. Anywhere else, it is profusely out of place. In its stead, I recommend the term "requirement" and then cite an actual reference by the PO or someone officially representing the business requirements.
Can you explain why? I’ve definitely seen must used in non military contexts. It also seems really strict to limit musts to things specifically laid out in requirements, unless I’m just used to dealing with really loose requirements. My gut is that you’d run into two categories of shoulds, one which is a much bigger deal than the other.
And protocol specifications
I saw another person talk about Red/Amber/Green — where Red is "fix this, it's wrong"; Amber is "here's a consideration, do with this what you will" and Green is "I don't know but I want to ask questions to learn".
So "Green: Why did you do it this way?" immediately comes across differently to "Red: Why did you do it this way?"
So "Green: Why did you do it this way?" immediately comes across differently to "Red: Why did you do it this way?"
The clear declarative: "Extract this to a separate function."
Everything thing else can be misinterpreted as a suggestion, dig, ect.
Actually an interview question to work with us. We need code, not egos. Write and be wrong, correct and continue. Do not sweat errors. None of us are perfect, and neither is this review.
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.
Complementary to other answers, I would suggest introducing this technique: https://dev.to/allthecode/moscow-the-best-code-review-techni... However, share this w/ the reviewee first!
I know some devs that are anxious if they don't fix _every_ single comment in their PR, even if I'm just suggesting something as "nice to have". This helped to mark which comments are actual priority.
Also, make sure you're using code formatter in your CI, to avoid reviewing the code style, as this can start some unnecessary wars and frustration.
Also, I suggest sharing this article https://mtlynch.io/code-review-love/ with your colleagues, especially step 8:
> As the author, you ultimately control your reaction to feedback. Treat your reviewer’s notes as an objective discussion about the code, not your personal worth as a human.
To some degree, as a reviewer you can keep your tone friendly, but ultimately _every_ sentence can be took as personal attack, and you can't really do anything about that. Keep that in mind, don't focus _too much_ on the tone, and don't let that prevent you from giving a comprehensive review.
One other thing I personally do, because I'm famous for giving _really_ thorough reviews, is to make sure from time to time that the other person is fine w/ that. Simple DM "I hope you don't feel too overwhelmed by my CR?" helps, especially that often people respond that they actually approve I'm so thorough. If you worry people get offended, it is easy way to make sure if these worries are valid or not.
I 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 feel like this is a problem ML could solve for assisted feedback focused on positive tone, ml models scans your response and offers suggestions on phrasing.
Trying to never use the word "you", it seems a bit aggressive. Using "we" or "one" instead, or rephrasing to get rid of pronouns completely.
In addition to other comments saying to add reasoning, I’m a big fan of the ‘MoSCoW’ method (must/should/could/would) to remove ambiguity around if things are a question vs a command vs a thought. ‘You must do x’ sets a clear condition before you approve a PR. ‘I would have done y’ shares perspective and experience without blocking when you think you might be just bike shedding.
I’ll usually link something like this page to the repo and align on the meanings of each phrase.
https://en.m.wikipedia.org/wiki/MoSCoW_method
I’ll usually link something like this page to the repo and align on the meanings of each phrase.
https://en.m.wikipedia.org/wiki/MoSCoW_method
My suggestion would be typical: concise, professional, and when straight-forward, specific. The key is to remember that the nature of a code review is first to improve product, and perhaps secondarily to improve process (ops or procedure) and perhaps thirdly, people improvement in the form of collective learning.
Thus, if we're talking about refactoring a line-of-code then just writing the refactored line-of-code is probably the most clear and concise. If it's more involved than that, then it may be worth taking offline. In the example you've provided here, I'd suggest the best language is likely: Refactoring this block as a separate function provides the following benefits: 1. blah, 2. blah blah, and 3. blah blah blah and no side effects. The only additional clarity that's helpful is to say whether it's optional, required, or some other directive.
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.
"Don't repeat yourself (DRY)."
I think the only correct answer is "it depends"
It depends on the relationship with the person who's code you are reviewing, it depends on the relative seniority differences, it depends on the project, it depends on the rest of the team culture, it depends on the rest of the company culture, it depends on how long you've had a working relationship with this person, it depends on who else might see the review.
I've had working relationships with people where it was beneficial/easier to be super curt and to the point, because there was sufficient 2-sided trust that best interests were at heart. I've had working relationships where I had to take having "open-ended genuine curiosity" to the extreme because of fear of feelings being hurt.
That being said, by default I take a question-asking tone to the code review process, with the intention of possibly learning something. It opens up the door to "being wrong" and allowing things to progress without incessant arguing. There are times when this approach isn't appropriate, but I think it's a reasonable default. Again, it depends.
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.
only suggest improvements, and provide reasons if possible, never tell anyone they should do something in a certain way unless it’s an obvious bug. I fucking hate know it alls that confuse their opinions and preferences with some absolute truth.
I tend to be overly “nice” in code reviews and phrase things more like suggestions and provide some justification for them:
> have you considered x? It is standard practice in y to do z.
> should we do x instead? I think it would be better in the long run because y.
If there is a more serious issue, I prefer DMing the author and hashing things out synchronously.
I think it is important to phrase things gently as much as possible, as tone is hard to read from text, and being overly harsh might discourage other teammates from taking risks and asking for feedback.
> 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.
A - for juniors
1 - This / should / could etc etc make sense only if the code is duplicated. If it's long, better to make a sub-function and use those in main body of the function for better readability / maintenance.
2 - As for tones, you are the senior. Don't sugarcoat juniors, tell them the truth green in their faces. You'll be appreciated more in the future by them because you're helping them grow, not running for mayor office and you need to be PC. Also, don't be rude / asshole, they still need your help though.
3 - I found out, the best is to talk to them like you talk to your friends.
B - for seniors
No sub-points here, seniors who make blatant mistakes gets treated like juniors (see section A). Those who make honest mistakes, well I found out they only need you for rubberducking because mid-review they'll realize and correct / change course on the spot.
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.
Tone: if you write, cultural differences will always remove any tone; the subtleties will be lost in translation. If you talk to the people, the tone of the voice and the body language is more important than the words.
isnt the first one good for everything, why even bother with the other options?
Never use should. It's confusing and inherently negative. It's also not very clear. Compare should with could in your examples. Could is very clear, a direct question to the read.
I would extract this to a separate function.
My tone normally is "I believe this should be extracted to a separate function, and here's WHY".
The WHY is to typically refer to some best practice, coding standards, something as neutral as possible.
I use the combination "believe" and "should" as it's friendly but still has some hint of authority. Another tip is to not just post negative review comments, I also comment on things that went well.
When I see somebody repeatedly make the same mistake, or very severe mistakes, I schedule a session to discuss the topic, as there's likely a knowledge gap or misunderstanding. Invest in people like that and make it clear that this improves life for both themselves and the reviewer. Win-win.
People can be very diverse, of course. Still, if you need to sugar coat feedback too much and are afraid of a possible backlash or drama, that's unhealthy. The exchange of good faith professional feedback is part of business.
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.
Several other people made good points about the overall review structure that might well be more important. But to answer your actual question about tone:
There are two axes I think about here when deciding on tone. Why am I reviewing this code? And why did this person write the code?
For the me axis, at one extreme I'm paid to review this code, and I designed or co-designed the software that's being modified in the reviewed change. Architectural decisions were mine. At the other extreme, I'm just another volunteer, this system I'm presumably interested and somewhat knowledgeable about (otherwise why review code for it?) but I may not even be an expert, and certainly can't be said to "own" it in any way.
For the other person axis, at one extreme they are assigned to work on my project, this is their job, at the other extreme it's some volunteer who has worked with this ten years longer than me and maybe designed it and it's good of them to ask me to review it, they could just push commit.
#6 "Extract this to a separate function" is a command, so it's not appropriate unless either they're an employee and I'm supposed to be mentoring them, or they sent unsolicited contributions to some project where I'm an expert and frankly I'd rather not have their contribution than take it as it stands. If it's a project that invites contributions (even very informally) then this is always the wrong tone.
#1 "Should we extract this to a separate function?" is a question, and so it's not appropriate if I am expected to provide guidance (e.g. in mentoring or when reviewing code from a brand new team member) but absolutely appropriate if I'm reviewing code by somebody who knows the project much better.
#3 "I would extract this as a separate function" is always appropriate if you are sure you would do that work. However, if you are sure you would do that work, consider just doing it instead. In the scenario where it's a junior employee being mentored maybe there's pedagogic value in them extracting it so write either #5 or #6 depending on whether they seem to need the explicit instruction or whether it's implied in the usual workings of your environment. In the scenario where you're a new volunteer maybe the author has a good reason it should not be extracted and so it's worth asking. But in the middle case why aren't you doing it? If it's just not worth your effort the same is true for the author.
#4 "This could be extracted to a separate function" is redundant and so is #2 "Could you extract this to a separate function" the answers are "duh" and "hopefully you can do so or else you should learn this language". So overall I think I see three tones I would use (#1 #5 and #6), but in quite different circumstances, two tones I think are too vague/ redundant (#2 and #4), and one tone which is probably never useful (#3) because it suggests I should do work instead, in which case why not just do the work and say you did it "also I extracted this as a separate function, [link]..."?
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]..."?
Here's a trick: always approve the code review, unless it's an actual bug, a security risk, or obvious quality issue. Then you can use whatever tone you want without much damage.
"approved, but I think this should be in a separate function"
"approved, but I think this should be in a separate function"
This is similar to how my team operates.
On my team, the PR author is ultimately responsible for their work. All review comments are considered suggestions. The Approve button is used only to indicate completion of the review. If a reviewer has a problem with the merged work, they can open a new case describing the problem.
We train all developers to have sufficient competency to work on any part of the code. If some code is too complex for a developer to understand, it is simplified. Documentation is prioritized and included as part of the work effort.
We have worked together this way for about 7 years, with high developer satisfaction and low turnover. The quality of the product has steadily improved during that time.
On my team, the PR author is ultimately responsible for their work. All review comments are considered suggestions. The Approve button is used only to indicate completion of the review. If a reviewer has a problem with the merged work, they can open a new case describing the problem.
We train all developers to have sufficient competency to work on any part of the code. If some code is too complex for a developer to understand, it is simplified. Documentation is prioritized and included as part of the work effort.
We have worked together this way for about 7 years, with high developer satisfaction and low turnover. The quality of the product has steadily improved during that time.
sounds kinda nice...where do you work...?
What's the point of reviews if author can unconditionally merge the PR without addressing the comments?
I'd feel very low to none incentive to spend time first understanding the problem and then reviewing the solution to the problem if I knew that my opinion can be disregarded just like that. Seems like a waste of time from reviewer POV.
I'd feel very low to none incentive to spend time first understanding the problem and then reviewing the solution to the problem if I knew that my opinion can be disregarded just like that. Seems like a waste of time from reviewer POV.
For non-critical stylistic or refactoring I prefix with "Consider".
> Consider extracting this to a separate function to make this more re-usable.
This gives the developer the option to disregard the comment. Essentially I am trusting that they will consider it. It shows that I trust their judgment instead of ordering them.
For stuff that is more important but not critical I prefix with "I'd recommend".
I'd recommend doing [current approach] [migration statement] [your suggestion]. It will [benefit].
> I'd recommend testing UI components in a similar way that a user would interact with it. It will make your tests more resilient and prevent other developers from breaking your code.
> I'd recommend moving this logic into it's own function, it will allow this code to be used by future developers and allow you to test it easier.
This both declares the recommendation and explains why it was recommended.
For stuff that is critical, I might just write it more directly:
> We need to extract this into it's own function so it doesn't lead to/cause X.
> Consider extracting this to a separate function to make this more re-usable.
This gives the developer the option to disregard the comment. Essentially I am trusting that they will consider it. It shows that I trust their judgment instead of ordering them.
For stuff that is more important but not critical I prefix with "I'd recommend".
I'd recommend doing [current approach] [migration statement] [your suggestion]. It will [benefit].
> I'd recommend testing UI components in a similar way that a user would interact with it. It will make your tests more resilient and prevent other developers from breaking your code.
> I'd recommend moving this logic into it's own function, it will allow this code to be used by future developers and allow you to test it easier.
This both declares the recommendation and explains why it was recommended.
For stuff that is critical, I might just write it more directly:
> We need to extract this into it's own function so it doesn't lead to/cause X.
+1 to phrasing blocking/mandatory change requests as "We" rather than "You"
Make observations but don't try to be an arbiter: Say "This function is long." but don't say "This function is too long."
You can soften that language by using phrasing like: "This function has grown long."
Explaining how to resolve a problem if the solution is obvious can be perceived as rude. Avoid. Don't tell other programmers how they have to solve a problem either - it's their job to figure that out - make suggestions instead.
If you want to make a suggestion, explain how it addresses the problem and if possible put an emphasis on improving things for the sake of others: "Extracting this to a separate function would make it easier to grasp."
Brief, helpful, and you're not appearing like you want to hold anyone's hand or micromanage them.
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.
It depends on so many things. One sliver of input I’ll add to this discussion is that I like applying the Socratic method for some cases where the goal is not just to write correct code but to help them learn how to examine code.
“What might happen if the input is a null value?”
Sometimes they realize things on their own after one or two questions. Sometimes many. But it’s always so neutral and keeps them in the driver’s seat.
“What might happen if the input is a null value?”
Sometimes they realize things on their own after one or two questions. Sometimes many. But it’s always so neutral and keeps them in the driver’s seat.
I prefer "Should we..." both as a coder and when I review code.
Complement some aspect of the implementation, then politely rip it apart with enthusiasm
I don't think there's a clear answer to this question.
Stereotypical corporate Americans of course would use the first two, but in Australia using language that tiptoes around peoples' feelings too much (e.g. making sure to point out positives even when someone clearly fucked up) can make it look like you don't trust them or think they're incompetent.
The only other time we'd hear that level of "compassion" is when we have way too much to drink and are acting like a knob.
I've heard Europeans make similar comments about American corporate norms confusing things.
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.
I would approve the PR and then send them one of my own to do the refactor.
It's remarkable how this change of perspective improves one's clarity on which code polishes aren't actually worth the opportunity cost.
It's remarkable how this change of perspective improves one's clarity on which code polishes aren't actually worth the opportunity cost.
I just give it straight. If they cant take it, then tough luck. Just yesterday I reviewed one where someone had used tabs instead of space. How can anyone in their sane mind use tabs instead of spaces. Chased that PR author out of there and closed the PR.
How can anyone in their right mind use spaces instead of tabs?
We have no guidelines where I work. People have different styles. I think the only one I particularly dislike is
* Please extract this to a separate function.
And I think my favorite is
* Should we extract this to a separate function?
Asking the person for their opinion seems more productive than commanding them to do something. If you word it like a command, then you're basically stating that you know better than the author and the author has underperformed. That's my opinion at least.
* Please extract this to a separate function.
And I think my favorite is
* Should we extract this to a separate function?
Asking the person for their opinion seems more productive than commanding them to do something. If you word it like a command, then you're basically stating that you know better than the author and the author has underperformed. That's my opinion at least.
Fwiw, I used to use respect-by-default until I encountered (multiple times) people who were too emotionally unhealthy and insecure about their abilities to handle it.
I literally had someone write me an impassioned email claiming that questions about why he made a design choice "served only to humiliate him, not an interrogative purpose", and that "when he did something wrong, I should just tell him what to do instead of asking him why he made that choice". This was beyond insane to me: I would hate it if someone spent 15m reviewing a design I had spent days on, and left no space for the possibility that they missed something.
Sometimes there really is a well-established relationship in which both people are aware that one person has much stronger knowledge of how to write healthy code than the other. While my instinct was to still approach the other person as a peer, experience has taught me that it often causes much _more_ friction to maintain this pretense.
I literally had someone write me an impassioned email claiming that questions about why he made a design choice "served only to humiliate him, not an interrogative purpose", and that "when he did something wrong, I should just tell him what to do instead of asking him why he made that choice". This was beyond insane to me: I would hate it if someone spent 15m reviewing a design I had spent days on, and left no space for the possibility that they missed something.
Sometimes there really is a well-established relationship in which both people are aware that one person has much stronger knowledge of how to write healthy code than the other. While my instinct was to still approach the other person as a peer, experience has taught me that it often causes much _more_ friction to maintain this pretense.
Wow that's crazy! Was it cultural issues for example foreign born coworker who doesn't understand how respect in the USA works in text form?
That's an interesting point about high skill/experience/knowledge inequality leading to unequal code output. It seems that the less skilled person wants you to sign off on the code in which case they turn their brain off a little and defer to you. I think approaching the matter as a peer suggests you expect this person to learn from you and so you are giving them room to grow, explore and ask questions but this means they need to commit more time to solving problems because clearly if you just did the work for them then you'd arrive at a solution much quicker. Sometimes it feels like we are under so much pressure to produce that there isn't time to really master something just let your coworker flex their strength and the problem goes away.
Thinking back on your experience would you say that the extra friction comes from time constraints?
That's an interesting point about high skill/experience/knowledge inequality leading to unequal code output. It seems that the less skilled person wants you to sign off on the code in which case they turn their brain off a little and defer to you. I think approaching the matter as a peer suggests you expect this person to learn from you and so you are giving them room to grow, explore and ask questions but this means they need to commit more time to solving problems because clearly if you just did the work for them then you'd arrive at a solution much quicker. Sometimes it feels like we are under so much pressure to produce that there isn't time to really master something just let your coworker flex their strength and the problem goes away.
Thinking back on your experience would you say that the extra friction comes from time constraints?
> Was it cultural issues for example foreign born coworker who doesn't understand how respect in the USA works in text form?
No, this was a white guy born in America. I think it was just a self-esteem thing: he didn't feel worthy of being treated as a peer, and wasn't emotionally healthy enough to handle the confrontation of one's mistakes that good engineering requires.
> That's an interesting point about high skill/experience/knowledge inequality leading to unequal code output. It seems that the less skilled person wants you to sign off on the code in which case they turn their brain off a little and defer to you.
I actually mean this less negatively. In my current job, I work with extremely talented and intelligent colleagues on an applied research system that also requires some heavy-duty engineering. Writing high-quality code is only one of the many skills required to be effective, and many of them handily beat me at any number of skills. There's a no-ego recognition that my ability to write and review clean, safe code massively outstrips theirs, in the same way that I recognize coworkers with (eg) ML or even execution expertise that outstrips my own.
In this context, I have relationships where both of us are extremely clear that my review of their code is not a meeting of peers, but a (relative) domain expert reviewing someone else's work. Again, these colleagues are intelligent and driven, so they are hoping to grow from these reviews and will absolutely push back if they disagree or don't understand a suggestion. But there's a baseline of mutual respect and a clear-eyed understanding of the asymmetry in expertise.
This means that I can use assertive statements to communicate confidence that my view is correct without offending ("This docstring implies that the blonker is a plumbus. Clarify that it's a blorp"). Conversely, I'll use less assertive language for suggestions that are ambiguous or subjective: "Hm, I think it might be cleaner if we move the foobar into the bazqux. Wdyt?"
I don't think any of this implies that they are turning off their brain and deferring to me; I think it's just a shared prior that my suggestions are likely to be correct and that they will agree with them on sight. I do my best to add brief reasoning to shorten the number of roundtrips due to disagreements or gaps in understanding, and I'll occasionally say, "If you still disagree, I don't feel too strongly about this".
> Thinking back on your experience would you say that the extra friction comes from time constraints?
IMO it comes purely from the aforementioned unhandled self-esteem issues. It's not been generally applicable to my current colleagues, whom all have my deepest respect. It's a relief and a reduction in mental load to be able to communicate directly, but using flowery language doesn't actually cost much walltime.
No, this was a white guy born in America. I think it was just a self-esteem thing: he didn't feel worthy of being treated as a peer, and wasn't emotionally healthy enough to handle the confrontation of one's mistakes that good engineering requires.
> That's an interesting point about high skill/experience/knowledge inequality leading to unequal code output. It seems that the less skilled person wants you to sign off on the code in which case they turn their brain off a little and defer to you.
I actually mean this less negatively. In my current job, I work with extremely talented and intelligent colleagues on an applied research system that also requires some heavy-duty engineering. Writing high-quality code is only one of the many skills required to be effective, and many of them handily beat me at any number of skills. There's a no-ego recognition that my ability to write and review clean, safe code massively outstrips theirs, in the same way that I recognize coworkers with (eg) ML or even execution expertise that outstrips my own.
In this context, I have relationships where both of us are extremely clear that my review of their code is not a meeting of peers, but a (relative) domain expert reviewing someone else's work. Again, these colleagues are intelligent and driven, so they are hoping to grow from these reviews and will absolutely push back if they disagree or don't understand a suggestion. But there's a baseline of mutual respect and a clear-eyed understanding of the asymmetry in expertise.
This means that I can use assertive statements to communicate confidence that my view is correct without offending ("This docstring implies that the blonker is a plumbus. Clarify that it's a blorp"). Conversely, I'll use less assertive language for suggestions that are ambiguous or subjective: "Hm, I think it might be cleaner if we move the foobar into the bazqux. Wdyt?"
I don't think any of this implies that they are turning off their brain and deferring to me; I think it's just a shared prior that my suggestions are likely to be correct and that they will agree with them on sight. I do my best to add brief reasoning to shorten the number of roundtrips due to disagreements or gaps in understanding, and I'll occasionally say, "If you still disagree, I don't feel too strongly about this".
> Thinking back on your experience would you say that the extra friction comes from time constraints?
IMO it comes purely from the aforementioned unhandled self-esteem issues. It's not been generally applicable to my current colleagues, whom all have my deepest respect. It's a relief and a reduction in mental load to be able to communicate directly, but using flowery language doesn't actually cost much walltime.
Thanks for the considerate response!
I think the no-ego environment does a lot of work towards being more effective and increasing self-esteem. It's just so conducive to learning! But learning can be difficult especially if someone who's smart has never really been challenged as I can imagine being the case if you're doing advanced research where everyone has so much deep expertise.
I think the no-ego environment does a lot of work towards being more effective and increasing self-esteem. It's just so conducive to learning! But learning can be difficult especially if someone who's smart has never really been challenged as I can imagine being the case if you're doing advanced research where everyone has so much deep expertise.
Yea, it's weird. I don't think I've actually really encountered someone smart and experienced who has thus problem. They tend to be secure enough in their own intelligence that honest consideration of their mistakes and gaps doesn't send them spiraling into insecurity and ego-protective backlashes.
I can only speak to my limited dataset, but it's a lot more correlated with self-esteem than actual skills IME.
I can only speak to my limited dataset, but it's a lot more correlated with self-esteem than actual skills IME.
[deleted]
I think it depends on how confident you are regarding the change and how important the code is. If something needs to be done, don't use language that suggests it isn't important. Similarly, don't made a big deal out of things that don't matter.
None of the above - I'd explain why I think the way the code was written was problematic (e.g. function logic can't easily be readily comprehended in one go, consider how you could break this down). But to a large degree my "tone" and level of detail would depend on the seniority of the author.
If you really insist on style review, how about defining a metric which allows the autonomous analysis of the code and autonomous recommendation？ Then you can put the rationale in a wiki and maintain stats on how often everyone gets caught. If the thing is so stylistic you can't spec it as code, why waste people's time reviewing？
I try to be polite, yet precise. Remember that the author likely put a ton of work into their code. I usually let them know that if they want to collaborate further, we can huddle on slack. I push others to do the same for me and have done so when needed.
I guess I will say this: Code review isn't about who can write the best code. Nobody can write the best code. Rather, code reviews help push the team to ensure that only the best code gets pushed.
I guess the only other advice I will give is to never criticize code without suggesting an alternative approach. Present that approach in the pull request and not privately. Accept criticism on the behalf of the code writer as well as others. Understand that no single developer will have the perfect solution to a problem, even when comparing juniors to seniors. I've been in this business for more than 20 years, and I still get surprised by fresh blood. If you aren't surprised, you aren't growing as an individual developer.
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.
Big thing for code reviews with junior developers: Show them where they've done something right. They'll keep doing it that way. Ideally this is done in person, and when you give them a "good job on this", it really takes the sting out of anything negative.
I usually prioritize diplomacy over everything else. You are probably better than me at this. The fact that I found an issue you didn't is probably just coincidence.
I could even be wrong. Maybe it's not an issue at all.
So I'll say "Could we move this out to a separate method? recountFooToilets seems pretty sepf contained and reusable" or just "I'd extract that method".
I don't want to say anything that implies I know better than you.
I'm pretty big on best practices, and a lot of people aren't, so when I find something like that I try to frame it as my problem, not their problem.
Especially with wheel reinvention, in-house implemented algorithms, use of low level primitives when the language provides high level things, lack of unit tests, or any other lone genius/cowboy coder/three star stuff.
I try to use the kind of tone Rust's compiler uses. "I don't have any reason to believe there's anything wrong here, but I don't know how to evaluate it, I don't have any experience with this style, it's above my skill level, and probably above some others too. Doing X is the tried and true standard, anyone can understand it, it will be easier to maintain".
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".
I’ve used a template for about 23y now:
“Instead of doing A here, let us instead use a B approach, otherwise there is a risk of C happening.”
And for shit: “This isn’t great because X. A refactor needs to take Y into consideration, which will result in Z-quality code.”
It’s all case by case, though, in reality. If I know someone we’ll, I might ask why they went with a certain style or a particular algorithm and just talk them through to grokking the error. If they’re somebody who is newer to the field, I usually point them towards a linter, especially if what I’m reviewing would’ve been flagged by said linter.
“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.
All of the above are fine. If both parties know that the review is about the code and not the author then tone is more or less irrelevant.
I’d prefer something like this: “I think we should extract this logic/code/etc into a separate function for better testing, less code duplication, and better maintainability.”
You only have to say that a few times before you can use the short hand: “Extract this into a function.” because you stated reasons before.
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'm fine with all of these. I think the way you phrase it could communicate how confident you are that this should be extracted to a separate function.
But I see code review as a conversation. Quite often it calls for a bit more discussion, and, depending on how the team deals with this, I sometimes reach out to the developer whose code I reviewed over a separate channel to discuss the issue a bit more.
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.
I think it depends on your level of seniority and the level of seniority of whom you're reviewing code for. It also depends on if the code is indisputably incorrect or if there is some grey area.
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.
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.
As a candidate I wouldn't care what you sound like. You are just the guy providing me the question and being the obstacle to my higher TC, not bothered about your tone.
I would extract this to a separate function.
I'm not sure if the question asked is even the right question (what tone to use isn't the problem). The code review should educate the developer submitting it if they don't know the points you are making. If they do know the points, I would tell them to review their own PR before another person since it helps everybody. We shouldn't be asking for approval for code that's not even double checked.. that throws the responsibility for catching bugs on the reviewer and not even the writer.
If they already self reviewed the code and it has problems, you can write a more detailed message explaining why there's a better way to do things. This way it doesn't come off negatively because the comment is a constructive suggestion. You may also have to compromise some lower priority feedback for development velocity as long as the submitter understands and can improve in the future.
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.
[deleted]
* 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?