5 ms·

I find the use of "Toxic" to be somewhat inflammatory and hyperbolic. I would say this would more accurately be titled as "Improving your Code Review Interperso

by zumu 9y ago

I find the use of "Toxic" to be somewhat inflammatory and hyperbolic. I would say this would more accurately be titled as "Improving your Code Review Interpersonal Skills". I wholeheartedly agree with the suggestions -- they are all great ways to be sensitive to your fellow coworkers, and I tend to favor this style. However, I know a lot of great developers who have a more direct style of communication. Also, I know several people who might interpret similar comments as patronizing or passive aggressive.

mpweiher 9y ago
In fact, the article seems to be more guilty of the transgressions discussed than the transgressions discussed. For example: "passing opinion as fact". And of course, calling these things "toxic". In fact, I disagree. Sometimes a little tough-love is needed, and this is the code we are talking about, not the person. I'd suggest the author look up "egoless programming". Her suggestions seem to pander to ego-full programming, and this will mask underlying issues and allow them to fester, causing significantly greater problems in the future. UPDATE: And of course, something unqualified is always an opinion (for a hilarious artistic rendition of this point, see the judge on The Good Wife who requires lawyers to append “in my opinion” to everything they say)
- burntsushi 9y ago
  Can you state specifically which of the transgressions this article mentions that you would consider appropriate tough love, and how exactly they improve the situation? Examples would help.
  - mpweiher 9y ago
    (a) "transgressions", not transgressions. (b) I am not going to break it down for you like that, because that would not be useful, missing the forest for the trees. I think the whole approach of the article is mistaken and harmful, particularly stated as absolutely, "toxically" and without evidence as it is. I've participated in code-reviews in roles as contributor, manager and mentor, with widely different teams in a variety of companies. The best teams that produced the best code were the ones that were the most "ruthless". Now of course: "hard on problems/code, soft on people", but with the first part being the priority. Because the machine doesn't care about your feelings, and if you screwed up, you don't benefit from not being told that you screwed up. And by tightly constraining the format to "helpful suggestion", you lose the range of feedback, from "awesome" via "yeah whatever" or "maybe try this" to "nope" and "what on earth were you thinking?". Because it's important to not forget that the range always exists, compressing it into a lossy channel just muddles it. Good teams have the rapport that makes the entire range of feedback possible and helpful. If that's not possible, it's an indication of a problem in the team. The more I am exposed to those sorts of teams, the more "toxic" I feel they are. It also incorrectly shifts responsibility for negative feedback from the cause (the bad code) to the bearers of the bad news, and with it the burden for avoiding it. In order to avoid negative emotions due to negative feedback, don't police the feedback, write better code! (Of course that assumes the feedback is roughly appropriate and proportional, a nuance that the article completely misses) And once again, that has to be tempered with your role. As a manager, for example, it is important to take yourself back a bit, to let people code in their style even if you think you can do better (and particularly if you know you can). As a mentor, on the other hand, you are specifically there to help people grow, so comments at the level of "yes, you could do it this way, but I think this might be better" are more called for, because they are there as helpful guidance, without the implication of "you must do it or else" that goes with the formal power position (and will be understood that way even if you don't mean it). And the most important part is fostering a team culture where there is a level of trust that feedback can be as harsh as necessary, where people actually want the toughest possible feedback. To create the best code and the best coders. See also: The Coddling of the American Mind https://www.theatlantic.com/magazine/archive/2015/09/the-coddling-of-the-american-mind/399356/ https://www.theatlantic.com/magazine/archive/2015/09/the-cod...
    - burntsushi 9y ago
      I will say that my comment was an attempt at making your concerns more concrete because they honestly don't make a lot of sense to me, but I feel like you just made them even more abstract. Based on the content of your comment and the seeming certainty of your opinions, I suspect we probably couldn't disagree more.
      - mpweiher 9y ago
        I have made things more concrete, just not torn it into little unrecognizable micro-fragments that miss the point. Can you be more concrete as to what you disagree with? So far, you have said nothing of substance, just asked me to do the work for you and then made a broad and dismissive claim.
        burntsushi 9y ago
        I was interested in an answer to my question to understand your perspective. You won't give me an answer. That's okay, but so is my desire to set parameters on conversations that I engage in.
        mpweiher 9y ago
        Those two are contradictory: the answers to your very constrained questions would not have given you my perspective. My answer gave you my perspective. Anyway, I don't let others dictate "parameters" to me...as that is not a conversation. And as far as I can see, you are part of this problem.
  - gurkendoktor 9y ago
    I'm not sure if this is related but maybe egoless programming is what I am thinking about here... I felt uneasy reading the OP. I follow much of the advice myself, and I can see how it is necessary to be diplomatic when there are tensions in the team. However, I much prefer it when people give me their raw and direct thoughts on the code. "indentation wrong", "really hard to follow this code", "why does this class exist?"; now we're two developers looking at code, and we both want it to be as neat as possible. If I can tell that the reviewer goes out of their way to be diplomatic (e.g. by following the OP or by randomly praising code that is not broken), they're obviously worried about hurting me; and suddenly we are not managing my code, but me! Why? Am I on the brink of being fired? Did I ever overreact? Am I too attached to my source code? It also becomes impossible to tell how strongly the other side actually feels about their suggestions when everything is posed as a "what do you think" question. It could be a cultural thing too? I'm German and American politeness often makes me paranoid because every flowery statement could be criticism in disguise.
    - mpweiher 9y ago
      Very much this. > American politeness What's interesting is that Americans don't notice/understand that their expectations are not universal in the least bit, but very, very culturally specific, not shared by most of the rest of the world and, quite frankly, rather broken.
    - burntsushi 9y ago
      Thanks for at least providing examples. From my perspective, you didn't really understand the OP. The point isn't to use "flowery" language. The point is to use more constructive feedback to avoid wasting valuable time and resources. > It could be a cultural thing too? I'm German No. I work with people all over the globe in open source. In my experience, humans everywhere appreciate helpful honesty.
    - jdmichal 9y ago
      I think that all your example comments are entirely appropriate. And, in the case of, "Why does this class exist?", I think it's extremely important that such questions are asked. In my opinion, knowledge transfer is one of the primary functions of code review in the first place. And that question is as much about knowledge transfer as it is about the code. Maybe there's a consideration that led you to create that class, and this is a chance for that other person to learn it. And maybe you will learn something back about another potential solution. I think the real negative that needs to be avoided is using code reviews to attempt to force people to do things exactly the way you would do them. I think many of the articles points kind of go towards this, but indirectly. Diversity of thought and approach is a good thing, and learning to live code that's not done in the fashion that I think is ideal was a lesson that took quite a while for me. EDIT: And, to be clear, "Why does this class exist?" is a potential lead-in to such a problematic viewpoint. The problem happens when, after explanation of the class' existence, the response turns into, "You should remove this class." That's where it transitions away from knowledge transfer and into forceful monoculture of approach.
jimbokun 9y ago
Puke emojis are not "more direct style of communication". Passing value judgments on code and the coder are not "more direct style of communication". Directly saying what changes might improve the code, and why the code is better with those changes is direct communication.
- kelnos 9y ago
  Likewise for sarcasm and surprise-questions -- "Why didn't you just do X?" is less direct than "You should probably do X instead, because Y". I'll admit to being guilty of some of these on occasion; must do better.
le-mark 9y ago
I think 'toxic' is highly appropriate, I've quit one job in particular because of horrendous code review culture. I think the article is spot on, on just about every point. But one point I believe is missing is a discussion of "appropriate level of abstraction". This article has a terrific discussion on this point[1]. [1] https://dev.to/lpasqualis/the-5-problem-solving-skills-of-great-software-developers-4e6 https://dev.to/lpasqualis/the-5-problem-solving-skills-of-gr...
bashinator 9y ago
Very likely the headline and the article were written by different people. And yeah, it’s a good article.
dang 9y ago
Ok, we've replaced 'toxic' with 'unhelpful' in the title above, since the article body uses that term.

by zumu 9y ago

mpweiher 9y ago

In fact, the article seems to be more guilty of the transgressions discussed than the transgressions discussed. For example: "passing opinion as fact". And of course, calling these things "toxic". In fact, I disagree. Sometimes a little tough-love is needed, and this is the code we are talking about, not the person. I'd suggest the author look up "egoless programming". Her suggestions seem to pander to ego-full programming, and this will mask underlying issues and allow them to fester, causing significantly greater problems in the future. UPDATE: And of course, something unqualified is always an opinion (for a hilarious artistic rendition of this point, see the judge on The Good Wife who requires lawyers to append “in my opinion” to everything they say)

burntsushi 9y ago
Can you state specifically which of the transgressions this article mentions that you would consider appropriate tough love, and how exactly they improve the situation? Examples would help.
- mpweiher 9y ago
  (a) "transgressions", not transgressions. (b) I am not going to break it down for you like that, because that would not be useful, missing the forest for the trees. I think the whole approach of the article is mistaken and harmful, particularly stated as absolutely, "toxically" and without evidence as it is. I've participated in code-reviews in roles as contributor, manager and mentor, with widely different teams in a variety of companies. The best teams that produced the best code were the ones that were the most "ruthless". Now of course: "hard on problems/code, soft on people", but with the first part being the priority. Because the machine doesn't care about your feelings, and if you screwed up, you don't benefit from not being told that you screwed up. And by tightly constraining the format to "helpful suggestion", you lose the range of feedback, from "awesome" via "yeah whatever" or "maybe try this" to "nope" and "what on earth were you thinking?". Because it's important to not forget that the range always exists, compressing it into a lossy channel just muddles it. Good teams have the rapport that makes the entire range of feedback possible and helpful. If that's not possible, it's an indication of a problem in the team. The more I am exposed to those sorts of teams, the more "toxic" I feel they are. It also incorrectly shifts responsibility for negative feedback from the cause (the bad code) to the bearers of the bad news, and with it the burden for avoiding it. In order to avoid negative emotions due to negative feedback, don't police the feedback, write better code! (Of course that assumes the feedback is roughly appropriate and proportional, a nuance that the article completely misses) And once again, that has to be tempered with your role. As a manager, for example, it is important to take yourself back a bit, to let people code in their style even if you think you can do better (and particularly if you know you can). As a mentor, on the other hand, you are specifically there to help people grow, so comments at the level of "yes, you could do it this way, but I think this might be better" are more called for, because they are there as helpful guidance, without the implication of "you must do it or else" that goes with the formal power position (and will be understood that way even if you don't mean it). And the most important part is fostering a team culture where there is a level of trust that feedback can be as harsh as necessary, where people actually want the toughest possible feedback. To create the best code and the best coders. See also: The Coddling of the American Mind https://www.theatlantic.com/magazine/archive/2015/09/the-coddling-of-the-american-mind/399356/ https://www.theatlantic.com/magazine/archive/2015/09/the-cod...
  - burntsushi 9y ago
    I will say that my comment was an attempt at making your concerns more concrete because they honestly don't make a lot of sense to me, but I feel like you just made them even more abstract. Based on the content of your comment and the seeming certainty of your opinions, I suspect we probably couldn't disagree more.
    - mpweiher 9y ago
      I have made things more concrete, just not torn it into little unrecognizable micro-fragments that miss the point. Can you be more concrete as to what you disagree with? So far, you have said nothing of substance, just asked me to do the work for you and then made a broad and dismissive claim.
      - burntsushi 9y ago
        I was interested in an answer to my question to understand your perspective. You won't give me an answer. That's okay, but so is my desire to set parameters on conversations that I engage in.
        mpweiher 9y ago
        Those two are contradictory: the answers to your very constrained questions would not have given you my perspective. My answer gave you my perspective. Anyway, I don't let others dictate "parameters" to me...as that is not a conversation. And as far as I can see, you are part of this problem.
- gurkendoktor 9y ago
  I'm not sure if this is related but maybe egoless programming is what I am thinking about here... I felt uneasy reading the OP. I follow much of the advice myself, and I can see how it is necessary to be diplomatic when there are tensions in the team. However, I much prefer it when people give me their raw and direct thoughts on the code. "indentation wrong", "really hard to follow this code", "why does this class exist?"; now we're two developers looking at code, and we both want it to be as neat as possible. If I can tell that the reviewer goes out of their way to be diplomatic (e.g. by following the OP or by randomly praising code that is not broken), they're obviously worried about hurting me; and suddenly we are not managing my code, but me! Why? Am I on the brink of being fired? Did I ever overreact? Am I too attached to my source code? It also becomes impossible to tell how strongly the other side actually feels about their suggestions when everything is posed as a "what do you think" question. It could be a cultural thing too? I'm German and American politeness often makes me paranoid because every flowery statement could be criticism in disguise.
  - mpweiher 9y ago
    Very much this. > American politeness What's interesting is that Americans don't notice/understand that their expectations are not universal in the least bit, but very, very culturally specific, not shared by most of the rest of the world and, quite frankly, rather broken.
  - burntsushi 9y ago
    Thanks for at least providing examples. From my perspective, you didn't really understand the OP. The point isn't to use "flowery" language. The point is to use more constructive feedback to avoid wasting valuable time and resources. > It could be a cultural thing too? I'm German No. I work with people all over the globe in open source. In my experience, humans everywhere appreciate helpful honesty.
  - jdmichal 9y ago
    I think that all your example comments are entirely appropriate. And, in the case of, "Why does this class exist?", I think it's extremely important that such questions are asked. In my opinion, knowledge transfer is one of the primary functions of code review in the first place. And that question is as much about knowledge transfer as it is about the code. Maybe there's a consideration that led you to create that class, and this is a chance for that other person to learn it. And maybe you will learn something back about another potential solution. I think the real negative that needs to be avoided is using code reviews to attempt to force people to do things exactly the way you would do them. I think many of the articles points kind of go towards this, but indirectly. Diversity of thought and approach is a good thing, and learning to live code that's not done in the fashion that I think is ideal was a lesson that took quite a while for me. EDIT: And, to be clear, "Why does this class exist?" is a potential lead-in to such a problematic viewpoint. The problem happens when, after explanation of the class' existence, the response turns into, "You should remove this class." That's where it transitions away from knowledge transfer and into forceful monoculture of approach.

jimbokun 9y ago

Puke emojis are not "more direct style of communication". Passing value judgments on code and the coder are not "more direct style of communication". Directly saying what changes might improve the code, and why the code is better with those changes is direct communication.

kelnos 9y ago
Likewise for sarcasm and surprise-questions -- "Why didn't you just do X?" is less direct than "You should probably do X instead, because Y". I'll admit to being guilty of some of these on occasion; must do better.

le-mark 9y ago

I think 'toxic' is highly appropriate, I've quit one job in particular because of horrendous code review culture. I think the article is spot on, on just about every point. But one point I believe is missing is a discussion of "appropriate level of abstraction". This article has a terrific discussion on this point[1]. [1] https://dev.to/lpasqualis/the-5-problem-solving-skills-of-great-software-developers-4e6 https://dev.to/lpasqualis/the-5-problem-solving-skills-of-gr...

bashinator 9y ago

Very likely the headline and the article were written by different people. And yeah, it’s a good article.

dang 9y ago

Ok, we've replaced 'toxic' with 'unhelpful' in the title above, since the article body uses that term.