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.
Ask HN: What tone to use in code review suggestions?
211–220 of 309 posts
Re: Ask HN: What tone to use in code review suggestions?
#212I would extract this to a separate function.
Re: Ask HN: What tone to use in code review suggestions?
#213My 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.
Re: Ask HN: What tone to use in code review suggestions?
#214Several 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]..."?
Re: Ask HN: What tone to use in code review suggestions?
#215Here'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"
Re: Ask HN: What tone to use in code review suggestions?
#216Earlier quoted context omitted.
> 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 be…
> socially retarded I used this term all the time growing up and I'm trying to stop because it's definitely offensive to some people. Have a good day.
No post body was provided.
Re: Ask HN: What tone to use in code review suggestions?
#217I 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…
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
Re: Ask HN: What tone to use in code review suggestions?
#218For 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.
Re: Ask HN: What tone to use in code review suggestions?
#219Earlier quoted context omitted.
The way you told the story it comes off as much more recent. > I'd hope that I've grown quite a bit I hope so too, because "I hadn't had my coffee yet har har" is a pretty thin excuse.
> because "I hadn't had my coffee yet har har" is a pretty thin excuse. I'm pretty blunt/rude in the mornings, within half an hour of waking up, even these days. Back then, I did not know that about myself -- or rather, just how rude I came across. Most people I interact with never see me like that, but it generally takes me 10-15 minutes to get some coffee going. Hence why I said "before coffee" since I assume nearl…
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.
Re: Ask HN: What tone to use in code review suggestions?
#220Make 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.