10 ms·
While I agree with the core of "DON’T: Criticize the person. ", I'm not sure I agree with the example. I get that avoiding "you" and "your" can help with setti
by quelltext 7y ago
While I agree with the core of "DON’T: Criticize the person. ",
I'm not sure I agree with the example. I get that avoiding "you" and "your" can help with setting the tone, but ultimately it matters more what is said in particular. The DO example to me seems overly distant, almost snarky and amplifying criticism in my mind, like somebody explicitly telling you "don't take it personally":
"This concurrency model appears to
be adding complexity to the
system without any visible
performance benefit." just screams "I think this is shit but I'm not gonna tell you more directly because we have a policy not to do that". Might be only me, maybe.
In my experience, I have received a lot of comments with "you" and "your" on them but they were still constructive. But when I received very sterile ones like the DO example I typically felt a bit weird, guilty, like someone actively avoiding to tell me what they think, or colleagues using extremely formal/polite language. A lot of things can carry weird undertones, depending on context.
- turdnagel 7y agoI came here to post the exact same thing. I find using "we" is a good workaround. And it's not just a euphemistic way of saying "you" — as a code reviewer, I know I'm also responsible for catching bugs or performance regressions, and by saying "we" I feel I'm expressing that shared responsibility.
- singron 7y agoYou can also use "I". Trying to avoid pronouns can make it sound like a high school lab report with excessive passive voice (e.g. "The baking soda was mixed with the vinegar" vs. "I mixed the baking soda with the vinegar"). In this example, I might say "This seems very complicated to me. Would a simpler model work?". If I had something simpler in mind specifically I would suggest it.
- pavlov 7y agoIt does come off as passive-aggressive. The tone is so final and closed to all discussion. If I were on the receiving end of that feedback, I'd much rather have the reviewer write something like: "Can you walk me through the rationale for this concurrency model? It seems complex for what we're trying to achieve at this stage."
- xxxtentachyon 7y agoThis is exactly the tone I try to strike, especially with junior colleagues. I never want to make someone’s decision seem unambiguously wrong-headed when it comes to subjective engineering matters. That just breaks someone’s confidence.
- Aeolun 7y agoEven if their decisions are sometimes unambiguously wrong-headed. It’s just an experience thing, but I’m trying to figure out how to tell them ‘trust me, I’ve made this mistake before, please don’t do it’, without trying to explain the 2 or 3 years of experience that prompt it.
- stevens32 7y agoExplaining the experience would make it a learning moment, "trust me don't do this" doesn't do much to legitimately motivate an alternative choice
- asdfasgasdgasdg 7y agoHopefully you are able to summarize -- that's what I try to do. Just asking for people to blindly trust you is a big ask, for two reasons. First, experienced people often develop mistaken beliefs. They try something and it happens to work, even though it might even be wrong in principle. Without any rationale, how is the person you're talking to going to judge whether the beliefs you've developed are correct? Second, even if they're willing to trust you, just getting the rule handed down does not grow them as an engineer. I might prefer to make a mistake and learn than take the right path without understanding the reasons. In fact, I have done so, intentionally. So, again, IMO, the goal you've mentioned in this comment ("figure out how to tell them `trust me...`") is an incorrect goal, and one you should not pursue.
- gregmac 7y agoIt's a good opportunity to teach them. You could ask questions that lead them to the same conclusion (with the benefit of hindsight from your experience): "What happens if this system is terminated before the final write call completes?" "How does this handle someone accidentally starting another instance of the application at the same time?" "If this external service we use is down, what happens?" You could also walk through the story of your prior mistake, draw parallels to the new code, and then discuss how to avoid it.
- gregmac 7y ago> "This concurrency model appears to be adding complexity to the system without any visible performance benefit." just screams "I think this is shit but I'm not gonna tell you more directly because we have a policy not to do that". Might be only me, maybe. After the knee-jerk "this is shit" reaction, the slightly deeper thought is probably along the lines of "There are better ways to do this that would simplify the code (reducing the chances of bugs, makes it easier to maintain) but it would basically require a complete rewrite". I think the challenge here is what you don't know is: Did the person writing the code just not see a simpler way? Did they iterate to this, and realize it's overly-complex but don't want to spend the time to redo it now? Do they think this is actually the best approach, and has some other merits that make it better than other approaches? How have others approached this in the past? I've definitely been in the situation where I had to review code like this, and I've definitely agonized over how to carefully phrase "I think this is shit" in a nicer way. If my review would really be "Sorry but I pretty much think we need to take a completely different approach and rewrite 95% of this" then I will typically have the conversation in person to discuss it, as it's easier to be clear I'm not criticizing the person and also avoid misunderstanding something (either direction). Sometimes it turns out I was missing something, but maybe there's still an intermediate way that reduces complexity but still addresses the problems the original did.
- asdfman123 7y agoYeah, if someone is just flat out wrong, it's better to talk in person about it, walk through the details, and tell them why you disagree with them or show them their mistakes.
- asdfman123 7y agoYou have to realize everyone is coming from different backgrounds with different communication styles. I think of it as everyone has a different "volume" that they listen at. Some people grew up in families that were more confrontational, and they're at a higher volume. They're comfortable with more confrontational communication styles. For others, from quieter backgrounds, if someone says, "Why the hell did you do it this way?" it's going to be a slap in the face because, hey, no one in my family talks to me that way, and I'm not going to let YOU do it. It comes across as very, very hostile. I think everyone though should learn to communicate non-aggressively, though, because it suits the needs of most people. When you start doing it it feels weird or passive aggressive, but you get used to it and it leads to much less friction. There are some people who might argue "the quieter types need to be less sensitive" but that's not how it works. If I'm a quieter type (and I am), I'm going to leave the company instead of have people yell at me, or it's going to severely affect my productivity. And honestly, I question if any team can really thrive in a very aggressive environment, where it becomes more about ego and dominance that the quality of ideas at the table. Google did a study that showed the only factor that determined how well a team did was feelings of psychological safety, and a huge part of that is just talking to each other respectfully.
- concordDance 7y agoFor some people (such as myself), hearing very formal commentary makes me feel very psychologicall unsafe. I'd much rather an angry rant because it feels like it's coming from someone who is my equal, rather than a representative of The Company who can ruin me.
- asdfman123 7y agoI don't think it has to be that dramatic. The key is just avoiding the word "you" and not using laden words like "stupid" and "shitty." You're still free to say stuff like: ---------------- "I really fundamentally disagree with this approach and know it will lead to problems done the road. Let's talk through it in person." Or "If we do this maintaining it in the future could be really hard. I veto it." Or "This isn't going to work with our XYZ system. Please fix that and submit it again." ---------------- You can say a lot of things rather directly that way. You DON'T need to say things like: ---------------- "This is a f*ING awful idea!" Or "What were you thinking?" Or "This is horrible. Don't check in crap to our code base, think through things first." ---------------- Is it too great of an imposition to ask people to phrase things the first way? Honest question.
- GuB-42 7y agoThis example is bad because it shows two things at once: the "do" comment is impersonal and more descriptive. I somehow interpreted it as "describe the issue better", which is generally good advice, but not exactly the point they are showing. Anyways, I think that "you" has a place in comments. It is when the comment is addressed to the person. Obvious but let me explain. For example if I know that some developer is particularly good at optimization, and I see some approach that looks slow, I will say, "why did you use this approach?". This is, in a sense, a mark or respect. "this approach appears to be slow" puts you in place of the teacher, but in some cases, I know that the person I am reviewing is better than me and I'd rather be the student. "why did you do that?" means "if it isn't a mistake, please enlighten me".
- musicale 7y ago"This concurrency model appears to be adding complexity to the system without any visible performance benefit" is pretty hilariously cruel though. Personally I might prefer "hmm, why did you do it this way?"
- BurningFrog 7y ago> just screams "I think this is shit but I'm not gonna tell you more directly because we have a policy not to do that". Maybe, but there is a huge difference between "I think this code is shit" and "I think you are shit".
- BeetleB 7y agoI'll reply to your comment and simultaneously address a lot of the subcomments. Most of what I say is based on what I've read in communications books. One day I hope to write a long post or a series of posts on the (mostly common) advice in the various books I've read. Disclaimer: I'm not good at these communications tactics live, and often don't put the effort in while posting online, so don't use my comment history as a judge of my knowledge ;-) > While I agree with the core of "DON’T: Criticize the person. ", I'm not sure I agree with the example. The example is "Why are you using this approach? You’re adding unnecessary complexity." To which a perfectly valid response is "Because it solves the problem, and I disagree that the complexity is unnecessary." Not a very constructive conversation, but completely valid. If you're not going to do the work to explain your concerns, I'm not going to do the work to satisfy you. A code review is not an arena where the reviewers get to ask and you the author are compelled to answer every concern. It is a dialogue, and a 2 way street. I do agree with you and I don't like their suggestion either: "This concurrency model appears to be adding complexity to the system without any visible performance benefit." Well, umm, that's just your opinion, dude. What do you want me to do about it? The reviewer did not specify what/where the complexity is, nor an alternative. And worst of all, (s)he is not even asking a question. As the reader, I don't feel compelled to respond. If the reviewer has a better approach handy, by all means suggest it and point out its pros/cons, etc. Regarding "you" statements, there is almost always a way to phrase them as "I" statements, but the important catch is that when you do, you usually should request something. The example in the blog post didn't, and it very much comes off as someone's opinion. What am I supposed to do with it? There's no reason not to be direct. But there are different ways of being direct, and you'll be a lot more successful if you give your perspective. nerdponx said: > "Why use XYZ pattern instead of ABC? This seems more complicated than necessary right now." At least this asks a question and gives an alternative. It still lacks any substance on why the reviewer thinks it is complicated (although in some contexts the difference in complexity between XYZ and ABC may be obvious). Also, in my experience, statements like "complicated", "complex" are often tactics to simply force the reviewer's preferred coding style, and this is a lousy tactic. If I were the author I would ask them to clarify why they think it is complicated. If I were to think of the worst code review experiences I've had, it's where the reviewer is simply pushing for his/her preferred style. Some people like continue statements in for loops, others prefer heavily nested loops. Neither is better. Either have a policy about them in the team or don't highlight it. I don't want to keep switching styles back and forth depending on who will review my code. > There's also a useful communication/pedagogy technique I learned many years ago called "praise, correct, praise". Agree with other commenters that this can explode in your face. It is also a tactic that is recommended against in some of the books I've read. In the long run you also lose credibility where people don't take your legitimate praises seriously, and get defensive any time you praise them. The tactic that is recommended is "Agree, and disagree." State first on the things you agree with the other person on, and then say "And I also think ..." and state your disagreement. And obviously, don't say "But..." > Oftentimes I’ll say “It seems like <bad thing>, I would probably <different approach>.” This is OK if you are just expressing an opinion and don't require the author to respond or to change it. If, OTOH, you really want this changed, then this is a poor way of pointing it out. As a reader, all I see is an opinion. We all have them, and differences in opinion are fine. A difference in opinion alone is not an obligation to discuss it. > "Can you walk me through the rationale for this concurrency model? It seems complex for what we're trying to achieve at this stage." I understand that you're seeing it as complex, but I did it this way because it solved the problem and I did not find it as complex. If you have an alternative solution that solves the problem that you think is not as complex, feel free to suggest it. I think asking for a rationale without suggesting an alternative means the usual (truthful and valid) response is "Because it solved the problem." There are many scenarios: Maybe I couldn't think of any other approach. Maybe I thought of 5 of them and had reasons to select this one. Do you want me to list a lengthy response examining each of the approaches I thought off with a justification on why I didn't select it? I shouldn't have to do that much work. It's much easier to respond if you proposed something and I'll address that. My response may well be "Thanks - I hadn't thought of that approach and agree with you." As a general rule, do not ask questions that could require a lengthy answer unless you are willing to offer something on your side. It requires almost no effort on your part to ask a question, but it always requires effort - sometimes a lot - for the other person to formulate a non-dismissive response. Don't make the other person work hard without good reason. If you're going to ask a question, put in the effort to explain your perspective. Never, ever, assume that the author has an obligation to respond to you. The Socratic approach, frankly, is a poor approach. It works in a very limited arena (e.g. teacher/student), and code reviews are not part of that arena. I learned this the hard way over many years where I'd ask very innocent questions, and occasionally someone would snap/shout at me. Only when I read the books did I understand: When asking a question, you have to make your concerns clear at the outset. You have to make the person understand why answering is a good idea. And you should never feel entitled to a response (and your words/tone/posture has to match). Finally, as a general rule, be humble as a reviewer. Very often, the author knows more about the code or domain (or both) than you. And very often the author will have already considered your approach and has a very good reason not to go with it. It's perfectly fair game to point out that if the author had such a reason it would be good to put a comment about it as other people reading the code will share your concerns. But acting as if you know better is already sabotaging the conversation. I would respond with "I do not understand the rationale behind the approach you picked. In my opinion approach XYZ is preferred and here are the benefits (list them all, including complexity). Did you happen to consider XYZ? If you did I'd like to hear your thoughts on the pros/cons of your approach over XYZ." Paranoid people may read more in my response than there is, but you can't control that too much.