Live data from Hacker News

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

news.ycombinator.com

51–60 of 309 posts

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

#51

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.

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.

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

#52

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…

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.

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

#53

[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/

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.

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

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

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

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

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

#56
post #46

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.

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.

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

#57

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.

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

#58
post #29

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 se…

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.

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

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

Post reply on HN