Ask HN: What tone to use in code review suggestions?

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.

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.

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.

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.

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