I feel obligated to mention that this strategy, while it often works to some degree, sometimes doesn't work. For people like me, phrasings like that sound passive-aggressive.

I prefer the strategy of wrapping my sentences in things like, "I think" or "I prefer".

So, I'd write, "This loop doesn't seem to terminate." or "I think that this condition needs a test." It's an implicit admission that I could be wrong and an invitation to push back. Just as often, I'll walk over and say, "Could you explain why you did that to me?" This is especially acceptable when it's agreed that a good review requires the reviewer understand what's going on.

I feel the way I communicate has been influenced strongly by voice training - I've noticed in particular I say "I feel" a lot in similar circumstances. (Apparently, valuing emotions above thought is some paragon of femininity? I'm not sure I agree with that.)

I am reluctant to go for the direct "wtf?!" response, because as above, it bruises egos and people get defensive - and that is unhelpful to actually resolving issues. I tend to phrase it as "Perhaps I'm confused, but...". (One advantage of this is that sometimes, I am wrong, and this gives someone an opportunity to explain why, and in the process hopefully re-examining their thought process and testing it just in case.) I think I caught that pattern, as well as excessive usage of "though", from a good friend. One interpretation of this is that perhaps I've spent nigh-on 2½ decades being confused. <g>

There is, however, a place for the genuine WTF. I reserve it for "there's no way you could be that stupid" situations, like someone cheerfully telling me about the 256-bit encryption they used, and my discovering it's RSA-256.

The management-by-Perkele has however seemed to work to keep the Linux kernel clean up 'til now, although they don't really want to insult people.

The success of the approach mentioned (ask instead of assert) depends a lot on the culture of the receiving developer. Also, if there's a language barrier, being subtle can actually muddy the waters instead of help the situation.

You can phrase things directly, even adding minimizing language (to save face), but be clear about the problem during the review.

>For people like me, phrasings like that sound passive-aggressive.

I second this notion. When you have doubt whether a loop terminates the question is appropriate but next to something as obvious as "for(;;) {}" it will simply sound insulting to the recipient's intelligence.

I prefer, "I don't understand this code/how this code will XXX"

I do this too, and I also overuse the phrase "I speculate" to tag when I'm, well, speculating, I wish other people did too because too often people unintentionally present their speculations as fact

Seconded

Also, think that the programmer might have already thought of that issue. Does that loop really never terminates? Isn't that test obvious/really needed?

"Are you out of your mind???" ;)

But in all seriousness, projecting issues as character flaws is a really destructive thing to do, not just in programming but in any relationship. It's a really good habit to avoid it whenever possible.

I read somewhere (I think it was Blink) that marriage counselors could tell which marriages would fail by observing those that generalized around their spouses character rather arguing about the issues, ie "You always..."

Siracusa said something similar on Accidental Tech Podcast a while back, and it really struck me because it made me look back on every argument I've ever had in a different light. He said he always phrased disagreements in such a way as to leave the person he was disagreeing with out of it. In general, always voice disagreement as "the thing you said is wrong" rather than "you're wrong". The former is about the point of disagreement, the latter is about the person.

It's not even about being diplomatic or being less likely to hurt someone's feelings (although you get that for free), it's really just about clarity as to what's being discussed. The most confident, secure person in the world might still get defensive when confronted with "you're wrong", because there's still a whiff of it being about questioning that person's credibility, as opposed to the thing that you think they're wrong about.

That's very true! I've found that the key to having a constructive argument is always doing your best not to hurt their ego, assign blame or back them into a corner where they'll lose face (always leave an out). Otherwise they'll slip into "defensive mode".

That together with the old chestnut "seek first to understand then to be understood" to ensure that you respect and understands their position are real game changers.

Its funny and scary how important these kind of social and interpersonal skills are working in any corporation. Yet often are lacking and leads to a lot of unnecessary friction.

It's something I almost wished was taught in schools, if only so they could recognize the patterns later when they go into working life.

I read somewhere that counselors who generalize relationships were least likely to be helpful.

I was curious about what I read in Blink so I looked it up and it seems to be a bit of the usual Gladwell hyperbole even if it was based in some kind of studies. Needless to say marriages probably can't be simplified that much but it still is a pretty good general rule

http://www.slate.com/articles/double_x/doublex/2010/03/can_y...

I follow this advice because I've found from experience that often there is a good reason, or at least _a_ reason, that I don't see. Maybe the loop does somehow terminate, through an obscure mechanism. Or maybe it's not supposed to terminate. Or maybe I missed something obvious.

I make mistakes all the time. I probably make mistakes more often than I do things right. Therefore, asking questions rather than making bold claims is usually betting the right way.

This doesn't mean you have to be a pushover. If you don't see something, you can say, "I'm sorry if I'm being a little slow here, but I still don't understand. Can you explain how XYZ works in more detail?" Most people are very happy to help you understand.

> I follow this advice because I've found from experience that often there is a good reason, or at least _a_ reason, that I don't see.

Exactly. I pulled in devs from two teams recently to discuss confusing modelling of domain objects on one end of our app vs. the other end, as my team were trying to deal with both. The naming involved was completely contradictory, which always makes fitting a system into your head so much harder when you have to mentally translate.

Turns out that each team had very valid reasons for modelling the entities as they did - the front-end team implemented the entities as understood by the business guys who would be using the GUI to configure them, and the other implemented the entities as understood by the API spec they were ultimately being transmitted over.

The initial response of "this is so dumb" was very easy and very wrong. Much of my personal development as a dev has been learning to suppress that initial dismissive response when encountering 'bad' code, instead asking why the code is as it is.

There's always a bit of a fundamental attribution error[1] in "your code is awful" discussions with devs.

(Although I'd say that this points to the issues that can arise from having two teams implement two separate parts of a business process.)

[1]: http://en.wikipedia.org/wiki/Fundamental_attribution_error

This, a thousand times, but I believe it also requires an intention of humility behind the question. There are still plenty of ways to phrase a critique as a question and come across as a jerk. People who are intentionally jerks will find a way to do it no matter what, though.

Asking a question leaves open the possibility that you, the person issuing the critique, are wrong. From my experience direct factual critiques are fine as long as they're correct. Some people might mind being told that a loop never terminates (even when that is trivially the case), but those people have their own ego issue to work on.

I feel the issue comes when your critique is either flat out incorrect or when it misses the context of why seemingly incorrect behavior might actually be desirable. Asking it as a question is an implicit exercise of the 'reasonable person principle'. That tiny shift in voice turns an accusation into a conversation that effectively starts with "My expectations have been violated by what I'm reading here. Have I missed something or was this intentional? If it's intentional, can you help me understand why this is desirable behavior?"

> This, a thousand times, but I believe it also requires an intention of humility behind the question.

Yeah, this is very important. Otherwise, using a question instead of a statement is patronizing.

I've encountered engineers who takes things too literally for this to work:

Me: "should this be documented in the team wiki?" Engineer: "Probably" "should we document it now before we forget?" "Okay" "um, do you want to do it? I could do it for you after I'm done with my tasks if you need help" "okay, you do it"

whoosh hint not taken. It's not rare either since the stereotype of socially awkward literal engineers is not entirely undeserved.

That's different. You're describing an attempt to get a particular answer to a completely subjective question via hinting, whereas in the GP comment the recommendation is to ask questions about matters which have objective answers (e.g., "does this loop terminate?").

Don't volunteer for that! Of course they will say yes, just as if someone asked if you wanted them to document something, or just as customers always say yes if you ask if they want a feature!

Just tell them, "... You should document this in team wiki before we merge this."

One approach I took in a recent project was keeping the documentation in documentation folders under each project, using markdown.

It was good because documentation was tied to the source control and easy to locate and read when you were working on the project.

It's easy that wiki's become neglected and forgotten if there's not someone that "cracks the whip" so to speak so I think any way to lessen the friction and bring it into the coding environment is a good one

What's the engineer doing at the time? What are you doing? Why shouldn't you be the one writing the docs?

Pointing out a bug in the code is not a comment on the programmer. Programs have bugs! Humans make mistakes. Commenting on a review with 'you made a mistake' is needlessly confrontational. but the alternative is wasteful: bending over backward to avoid even suggesting that the code might have a bug. If the reviewee's ego is based on never making mistakes, then the review will hurt their feelings regardless of how the comments are phrased.

Yes, that is exactly what I do too. When I just started I treated code reviews as a way to show my knowledge and feel superior, which reflected the environment I was in. Once I moved to a new job with a jelled team and a positive sharing environment I noticed the use of questions in CR, and now it is my default mode.

If there is an oversight your part, the amount of rebounded embarrassment is far smaller. Bad feelings on both sides are minimised no matter what the outcome with your suggestion.

If you continually do this though it could certainly come off as passive aggressive. I feel there's a line to draw; if you have 100 different items to go over, it's going to quickly become annoying if all 100 are phrased as questions. Sometimes you just need to say 'this should be in a function.'

I think the key is that if you ask a question, you should be willing to accept "no" as the answer. Sometimes the answer is "not now" or "no", and perhaps adding a TODO or a comment explaining why not.

Particularly well-explained code shouldn't raise any questions.

I suppose so. Most often I don't harp on stylistic choices that don't affect the integrity of programs. I might encourage another coder to lint their code, for example, but I won't specifically single any of those out. I just won't waste time asking questions if I find ten places the program could error out on.

I agree. My preferred phrasing uses “we” (we are working on this code together) instead of “you” (you are working on this code and I am telling you off) and is usually a sort of polite suggestion with concrete justification. For example:

Another thread could have modified ‘x’ between when we test the condition and when we take the lock, so we should test that ‘x’ is ‘NULL’ if ‘y’ is true here, and if not, sleep and retry.

I've been criticized as being too harsh in code review before, and I'll try this.

I'm not sure how to apply it in all cases though. Frequently the issues I see are coding standard violations... use of exceptions, bad use of virtual or function pointers, ...

How do you say 'we dont use throw here' as a question?

You can ask a leading question or series of questions about what bad thing(s) might happen down the road as a result of what they did - which could hopefully lead to them having a greater understanding of the 'why' of the best practice, and less likely to repeat it in the future.

Well, the biggest why of exceptions is that we want to be able to compile with exceptions turned off.

And there are lots of reasons why not to use virtual functions or function pointers, but aside from 'it kills performance if used too much', they're fairly subtle. But really, the issue is typically with something like this the solution is 'rewrite it', which is brutal no matter how you phrase it.

Something like a period of pair programming/mentorship might ease the transition (we only do code reviews on new hires), but it's tough to justify.

If they really need dynamic dispatch, we prefer using an enum because of serialization, efficient sorting by type, code readability (this is debatable), etc., but really even this isn't what we want, because most of the time we don't need dynamic dispatch. Doing this wrong in one place is unlikely to be a huge deal, but if we can't trust them to write code that performs well... it's bad.

You could leave a top-level comment: "Have you checked all this code matches our coding standards?"

If they say yes you could start getting more specific - "does this bit of code match our coding standards for exceptions?"

The first is probably the right approach, as the second comes off as unnecessarily condescending to me.

This is a great tool in my experience as well.

I would add that it is not only useful to phrase your criticism as a question, it is also useful to try to think through the possible answers, because there may be a logical one.

Doing this can save a lot of embarrassment as well; it can be a rotten feeling to give a harsh assertive criticism, only to find out that you were completely incorrect (not that has ever happened to me!).

I also follow this rule or I try and state things as if they are my opinion. So instead of:-

> This method should not be static.

I would say

> I think this method should not be static.

I will also follow up with a reason. For example:-

* I think this method should be static as this class is instatiable.

Whether I give a reason or not depends on the experience of the person whose work I am reviewing.

I agree with the spirit of the parent comment, but do those examples make sense?

Moderators at research and question social networks need to learn to critique and codify questions instead of people, and repress the aggressive tendency to escalate people based on how intelligently they form terms alone against help.