In general I agree with your point. I just want to point out one particular edge case where it can be misapplied.

The one thing I always want to be careful of with questions is to make sure they don't come off condescending. One example of an anti-pattern with questions is to ask

"Why did you do <obvious mistake>?"

In that case, clearly they didn't mean to make the obvious mistake. Asking the question implies that they did it intentionally, when surely what actually happened is they overlooked the error. Instead, I just point out the mistake. "I think you forgot to check for null here." comes across way better than "Why didn't you check for null here?"

The general principle I find useful is to try to say exactly what you mean. When you spot an obvious mistake, there's not really a question to be asked. You are trying to note the mistake, so just use a statement to note the mistake. When it's less obvious, or it's a judgment call, a question is great, because that's exactly what you're trying to do: ask whether another approach might be better.

Few things in a professional setting annoy me more than what you're describing. I left the default date on a form once and got it back with a note that said "why did you put this date here? Today's date is XYZ."

You got me. I thought it was 2004. Thanks for correcting me, now I know it's actually 2019. Pretty shocking that I'm actually 15 years older than I thought! I guess I'm a huge moron!

For the cases where a question would be too condescending, or things that are mostly a matter of taste, I phrase them like you did: using the words 'I think'. Still leaves open the possibility that I missed something obvious, or that it's ok to disagree with me.

I think you're right that in case of obvious mistake, like a typo, a question would sound condescending (like "I think you can't handle feedback" instead of "let's make sure we understand each other's thinking").

My variant on this tends to go:

"It looks like this <code block/method/module> will cause XYZ to happen. Is that what you intended?"

Simple example (Python):

    def foo(x):
        print(x.lower())

"If x is None this will raise an exception. Is that an expected outcome, and/or can we assume x is always non-None?"

One thing I like about this approach is that it first describes your understanding of a particular code section, which helps illuminate any assumptions we might hold (and which might be incorrect). This is a good practice generally, IMHO, with or without the followup question. A guy I used to work with would make non-judgemental comments that just summarized his understanding of what was happening in the code, and it was helpful because when the comment was off, it illustrated a way in which the code was perhaps unclear or incorrect.

Thank you.

Of all the variants proposed in this topic, this is the only one that puts any burden whatsoever on the speaker in the first place (in a good way).

Demonstrating that you've made an actual attempt to understand what your colleague is doing is far more genuine and likely to be received as such. Unlike the mealy-mouthed carefully-phrased examples that just communicate "I care more office politics than actually ensuring this code is high quality," your approach hits a trifecta of signaling respect, actually identifying the concerning behavior of the code, and inviting all the types of productive responses:

You allow the person to graciously admit an error without particularly losing face (they can simply say "nice catch, thank you" and fix it themselves). You provide an inroad to correcting your OWN potentially false assumption -- very helpful, for example, when a junior dev whose work is being reviewed is actually in the right, but politically might be unable to demonstrate that. And your question about the mechanics opens the door to a collaborative discussion in which both sides contribute to the final codebase.

> Why did you do it this way?

I think that's a high load question (in terms of time and energy required to answer). It's best used sparingly. Instead point out a specific problem and supply an alternative that does not have that problem.

I use this approach a lot too, but I’ve found that some people consider it condescending so know your audience I guess.

It's condescending if your feedback is just bike shedding. Like if you think this module should be broken out into it's own namespace it's on you to explain why as opposed to "Have you considered putting this in a namespace?" -- most likely I did, and I didn't think it was important and now you are asking me to write up why I preferred 6 of one over half a dozen of the other and in non-violent communication forms on top of it. If there is a good reason, then just say it, otherwise it's super frustrating and time consuming.

BUT, if you are going to do that, please do it in your first pass in review, don't do it in your subsequent pass after I've already responded to your feedback. This is just inconsiderate of my time.

I follow the opposite rule. Never use a question unless you genuinely don't know the answer. For example the PR contains a bug, which was obviously not noticed by the submitter. The answer to "why did you do it this way?" is "I didn't think of all possible scenarios". The answer to "What would happen if blah happens?" is "Well shit, clearly everything will break!" . The answer to "Have you considered that foo might be null?" is "No I god damn haven't". By asking a question you're asking the submitter to publically admit their mistake (since not answering questions on a PR is rude) and apologize for something that all programmers do naturally (writing buggy code).

Instead a proper comment is "This breaks if blah happens and foo is null" at which point the submitter just fixes the bug. It achieves a) by being a neutral statement, b) naturally by making the submitter rethink their assumptions without having to apologize for that and if the reviewer was in fact wrong and the comment is incorrect, c) would happen due to the submitter explaining why this situation can't happen (and possibly adding a code comment to that effect).

Agreed. I also find myself just attaching a question mark to most comments unless I'm very sure of them.

E.g. "Add a comment about foo bar?"

I love this approach because it starts from the position that the code writer knows what they are doing, instead of assuming they don't.

I think this is just generally good advice anywhere when you find yourself providing feedback or constructive criticism, especially if there's a potential for a power dynamic. You absolutely nailed all the right reasons why it's effective, and I do it all the time.

The only real downside I run into is that sometimes the questions get answered too earnestly, and I end up with the person on the other end helpfully trying to educate me rather than engaging the question or issue directly. In every case, though, I could and should have phrased myself more clearly. It's a constant learning process, but the system works.

BeetleB makes some great points in this thread. Answering some questions is complicated if you want to be thoughtful, it places a big burden on the author.

How do you ensure the other party doesn't interpret "Why did you do it this way?" as an aggressive question?

Use a laughing emoticon? It’ll probably come off as condescending, but not likely agressive.