1. 34
  1.  

  2. 29

    You shouldn’t be having visceral reactions to peoples’ coding mistakes anyway.

    No. Emotions are part of how humans work. Of course, these emotions can lead to various kinds of communication, and in a team setting choosing kind and useful communication is important.

    I would prefer the author say “Your code review need not contain raw emotional language.”

    P.S. The word should is a minefield in writing.

    1. 5

      Agreed. And it’s not just mistakes — I remember (almost 20 years ago) being really steamed by a coworker’s commit that added a new feature by spicing a bunch of awful kludges into clean code I’d written. It felt like an insult, like sabotage. I went next door and started yelling at him. Not one of my finest moments.

      1. 3

        At least the awkward memory will keep you from doing the same in the future, and as long as he didn’t give up and go to law school there’s probably no long-term harm ;-)

      2. 3

        I would prefer the author say “Your code review need not contain raw emotional language.”

        Outside of obvious emotive words, what makes a statement “raw emotional language”?

        Whowhich particular group of people (amidst an international community of tech people, where emotional appraisal varies among cultures) – gets to objectively qualify it as so?

        1. 2

          Who … gets to objectively qualify it as [“raw emotional language”]?

          You added “objective” to this question. I would not expect such judgments to be objective.

          One possible rephrasing of the question is “Who gets to decide if there is raw emotional language?”. Still, that phrasing is limiting, because the question may imply:

          1. there is (a) one person or group…
          2. … making (b) “the decision”

          However, there doesn’t have to be one person or group acting. Also, there doesn’t need to be one decision made. There is more flexibility than that.

          Here is a better phrasing of a set of questions: “How would this suggestion (to avoid raw emotional language) play out? Would it be a guideline? Would it be enforced? If so, how?”. Rather than write at length, I’ll just say this becomes a decision for teams to figure out; i.e. managers, human resources, and employees.

          1. 1

            Your better phrasing still doesn’t address the question of what makes something (such as a statement in a code review) “raw emotional language”.

            In a diverse team comprised of people from multiple backgrounds across the planet, many will have differing definitions given the cultural differences in emotional appraisals (especially of language), thus there is no globally uniform notion as “raw emotional language” (beyond emotive words) that everyone will sincerely agree upon … and so ultimately – if one were to enforce, be it as a guideline or a hard rule, this nebulous idea at all (aka. “play out”) – it would have to come down to whichever person/group (usually in authority) gets to choose what is/ what is not considered “raw emotional language” (at the expense of going against a subset of their employees).

            1. 1

              Overall, I think you are losing essential context here. Remember the context: my feedback is offered at a high level with regards to the original blog post. Please read it in this context. Nothing there is perfect, and there are no claims that enforcement should be harsh or rigid. The post does not suggest these guidelines have to be imposed.

              In a diverse team comprised of people from multiple backgrounds across the planet, many will have differing definitions given the cultural differences in emotional appraisals (especially of language), thus there is no globally uniform notion as “raw emotional language” (beyond emotive words) that everyone will sincerely agree upon.

              You’re making a false dichotomy here. There is likely to be significant common ground.

              In areas where there may be differences of experience or opinion, having these kinds of discussion will be valuable. It is very useful for a team to be aware of how their language affects each other. It can help build common understanding and some shared norms.

              Yes, even after these discussions, some areas of disagreement may remain – such is the nature of humanity. Having such differences is not a convincing justification for throwing out an imperfect goal altogether.

              this nebulous idea at all (aka. “play out”) – it would have to come down to whichever person/group (usually in authority) gets to choose what is/ what is not considered “raw emotional language” (at the expense of going against a subset of their employees).

              It sounds to me like you are asking for the impossible – a completely unambiguous definition, applicable across all regions on earth, where all employees sincerely agree at a particular time.* I think it would be helpful if you accept that this is not going to happen perfectly, and this is ok. However, it is still better to strive to not include raw emotional language in a code review. Some imperfections in figuring this out do not justify throwing out the goal of being calm and rational in a code review.

              * Remember that people can adapt and change.

              1. 1

                I do understand that you are giving a high level and general view … however my comment is primarily in response to a very specific code review criterion proposed (be it prescribed as a guideline or imposed as a hard/ rigid rule), viz:

                Your code review need not contain raw emotional language.

                which you also repeat but in a different form as,

                it is still better to strive to not include raw emotional language in a code review

                Hence my as yet unanswered query of just what would constitute this “raw emotional language” (beyond emotive words).

                Now if, as you say, there is likely to be “significant common ground” in regards to emotional appraisal of statements in code reviews among a diverse team comprised of people from multiple backgrounds across the planet – such as from the Americas, Europe, Africa, Asia, Russia, Australia, etc – then surely it would be straightforward to directly answer that as yet answered query?

                It sounds to me like you are asking for the impossible – a completely unambiguous definition, applicable across all regions on earth, where all employees sincerely agree at a particular time.

                Which is why I say that if one were to prescribe this as a guideline or impose it as hard rule – it would ultimately have to come down to whichever person/group (usually in authority*) gets to choose what is/ what is not considered “raw emotional language” (at the expense of going against the rest). However if it be as loose and time-varying in meaning as you say it is, then nobody nowhere at no point in time will have any idea of what “raw emotional language” is, thus making the whole guideline/ rule null and void to begin with.

                * I use this word in its sociological meaning (and regardless of organizational hierarchies).

                1. 1

                  What?

                  1. 1

                    What?

                    What is it exactly that you are asking here?

      3. 26

        Calling reviewers’ behavior “toxic behavior” seems pretty toxic itself. It immediately puts it in an us-vs-them hostile framing. If “toxic” is removed from the title, it would be less likely to rile people up. The “unhelpful” framing is better, but still violates the guideline against passing opinion off as fact. How about “Review behaviors I think are counterproductive”?

        1. 7

          I think “toxic” is a pretty reasonable descriptor in this context. Behaviors which are unhelpful, counterproductive, adversarial, and have a net negative effect on culture and trust are IMO bad enough to be classified as “toxic”, especially since they have a tendency to spread around unchecked.

          1. 4

            Real-world example of this in a general context (not just code reviews).

            1. 3

              Much like “problematic,” the word “toxic” seems to have become a signifier of a certain kind of holier-than-thou preaching, and, well…. problematic.

              1. 1

                Yes, the word “toxic” can be misused. It can be used to bash people over the head, overgeneralize, and proclaim one’s self-importance, none of which tend to be useful.

                Still, I think there is a useful core to the word: calling attention to individual behaviors that might be narrowly defensible (e.g. technically correct) while also being harmful to a group.

                1. 2

                  I would agree it’s widely misused where “harmful” would do (but that typically requires being willing & able to enunciate the specific harm, whereas “toxic” tends - for whatever reason - to get a pass on that front).

              2. 2

                Fair enough. Whatever we choose – “toxic” or “unhelpful” – these words tend to imply a value judgment about what is important to a group.

                It immediately puts it in an us-vs-them hostile framing.

                Well said. It depends on it a value is shared between two people:

                • On one hand, if two people share a value, pointing out a value misalignment may be useful. The other person might think “I value X, and I didn’t mean to take an action that compromised it.” and be willing to discuss and/or change behavior.

                • On the other hand, if two people disagree on a value (or its relative importance), calling out another person for failing to live up to a value often starts a conversation on the wrong foot.*

                * In my view, it takes a certain patience and skill to be insulted or judged by another person, and calmly understand the other person’s motivations without being bothered. There are great opportunities to turn a situation around. Hostile people often are surprised by people that engage without cowering or yelling back.

              3. 19

                Was a bit surprised to not see this helpful thing mentioned:

                Add positive feedback as well.

                It feels that adding “great tests”, “whoa, didn’t think such a nice solution is possible”, “nice idiom!”, etc comments (where they are true, of course), makes the overall review more productive. Thinking fast and slow also mentions that people learn better when rewarded for positive behavior, than when punished for negative.

                1. 16

                  I don’t understand why people are voting this as off topic.

                  1. 11

                    People don’t realize that software is made of people? :-)

                    1. 5

                      I’m guessing it is a reaction to the language and/or mentailty presented in the article, that can be percieved as “HR”/buzz word-y. I can relate to it, even if I don’t think it is off-topic.

                      1. 2

                        It uses words like “toxic” and discusses developers’ emotional states, which to some people is a signifier of the ways of the Bad Tribe™.

                      2. 14

                        Unhelpful behavior #4: asking judgmental questions Avoid asking judgmental questions like: Why didn’t you just do ___ here?”

                        If I trust your programming ability and you do something non-obvious, I’m going to ask why because I’m going to assume you have a good reason and I would like to learn.

                        1. 14

                          I think she’s only talking about the flip side of that, where you’re blaming someone for doing it wrong.

                          It occurs to me that it can be hard to tell these apart. So when asking the sincere version you refer to, one should probably try to word it less ambiguously. Maybe “Just curious why you’re doing this instead of ___?” Although maybe that just comes off as subtler sarcasm. Sigh, communication is hard.

                          1. 10

                            Yes, hedge words can be very helpful. Also: I-statements, opinion rather than fact, approaching with curiosity and uncertainty rather than confident declaration… And yeah, this can require writing more words and taking more time to phrase things, but I’ve found it easier over time. I find myself writing things like “I think I would normally prefer X here – is there some benefit to this approach instead?” If that comes across as sarcasm, there’s probably an existing trust problem that needs addressing.

                            (Sometimes it turns out their code has some hidden upside, in which case I gently suggest a quick code comment or even a test, if appropriate. If I’m asking as a reviewer, the next person to touch the code will probably wonder as well.)

                            Another approach that I’ve seen is to prefix each comment with an explicit description of what kind of comment it is: [seeking clarity] Why is … / [suggesting alternative] What about … / [request] Can you add a comment about why …

                            All this said, I’ve only ever encountered one person who I’ve had a bad interaction with during code reviews in either direction. The people I’ve worked with have been very mature and empathetic. There’s one person who has done one of the behaviors on the list, but not enough to bother me greatly, and I believe that if I push back a bit they’ll be happy to discuss it and work out shared norms. So uh, also “work with nice people”. Pro tip.

                          2. 12

                            I think the keyword is just

                            “Why did you do it this way?” vs. “Why didn’t you just do it this other way?” are extremely different questions.

                            1. 6

                              This. “The Gentle Art of Verbal Self Defense” devotes an entire chapter to “just”.

                            2. 4

                              If it’s not obvious why something is being done, or why it’s being done a certain way, I often point out that it is deserving of a code comment. This offers the opportunity to reflect on whether it’s a good idea or fundamentally unclear and in need of documentation, without being overtly judgmental.

                              1. 6

                                An experienced developer will probably rethink whether the code can be a bit clearer, but a junior/less experienced developer (or an experienced developer who is distracted/tired/etc) might take it at face value and just document the code (which sometimes is the best solution, of course, but certainly not always).

                              2. 3

                                There are much better ways of asking that doesn’t use the judgemental “why didn’t you just ___” formulation, and also shows your intent (wanting to learn) much better. “Please help me understand why you did this instead of __” for example. Or “Did you consider __?” if brevity is key. (It’s even shorter than the original.)

                              3. 9

                                It’s good to be aware of how our words impact others, but it’s also important to not forget about technical precision just because we are in consideration of our emotions. I mostly want to add nuance about unhelpful behaviors.

                                In #1, will the suggested change improve performance? To prove it, make the change locally and run the measurement. We should be careful to not simply take our opinions and phrase them with extra words in the hope that they will sound less opinionated; we should avoid rationalizing. The author does provide the useful advice (helpful behavior #7, in fact) of canonicalizing opinions, using tools like linters and formatters to remove opportunities for bikeshedding.

                                In #3, I think that it is worth asking why any code maintenance team would tolerate maintenance of messy code. Preventative maintenance can massively lower the costs of working on a codebase; I was raised with the adage, “always leave camp cleaner than you found it.” It is indeed unfair to ask people to probe into nasty modules in order to surgically implement a single feature or bugfix; however, it is also unfair to check in such nasty modules in the first place, and code review is the only place where such nastiness can be examined before check-in.

                                In #5, we should try to understand sarcasm as a coping mechanism for the fact that we will never produce anything perfect, and the accumulation of errata is not just inevitable, but bizarre. I cannot find the precise phrasing, but I recall a datasheet which documented a register as taking values from 0 to 7 (3 bits), and helpfully indicated that if one ever wanted to write 7 to the register, then one should just write 6 instead, because the hardware always turned 6 into 7. I don’t think that I have ever been able to engage with this without at least a little sarcasm. That said, the examples given are not just sarcastic about software, but actively attack contributors, which is inexcusable.

                                In #6, I think that #5 should be applied. Emoji are very easy to misinterpret; famous examples include ‘PISTOL’ (U+1F52B) and ‘AUBERGINE’ (U+1F346), popularly known as the “gun emoji” and “eggplant emoji” respectively. Signifying that code looks good or giving praise with emoji is just as susceptible to misinterpretation via sarcastic usage, like with the “hooray emoji” ‘PARTY POPPER’ (U+1F389) on GitHub.

                                In #7, this is a UX problem on GitHub. I have used code review systems which track each individual comment in a pull request as a living note which updates as new changes are added. In these systems, it is not ambiguous whether a comment has been addressed; each change explicitly comes with the opportunity for the author to acknowledge feedback.

                                I would consider another helpful behavior: When a review gets long, add an introductory paragraph. Even a single sentence can set the tone and expectations for the author reading the review, and keep otherwise-painfully-dry nitpicking from being rude.

                                I didn’t reply to every section because some sections had no technical problems whatsoever, and I agree with the non-technical thrust of the article. LGTM 🎉

                                1. 8

                                  In #5, we should try to understand sarcasm as a coping mechanism for the fact that we will never produce anything perfect, and the accumulation of errata is not just inevitable, but bizarre.

                                  As a very verbal person who’s quick with a comeback, I learned early that deploying sarcasm can be incredibly satisfying - and deeply hurtful. It’s a bit like terms of endearment - best reserved for people you know very well.

                                2. 9

                                  Quite a few of these points are about expectations and interpretations; for example is “this component should be stateless” really passing off opinion as fact? Well, depends on how you read it; personally I’d think that the “I think it would be better if” is implied in a good-faith interpretation of it, and doesn’t really need to be typed out. The same applies to “judgemental” questions like “why didn’t you just do ___ here?”.

                                  If it’s a stranger on a random open source project then I tend to be a bit more verbose to avoid misunderstandings. If it’s a coworker with whom I have a good repertoire then I can be more concise and brief. There is value in being concise too; less work for me to type, and less work for you to read.

                                  Looking for the worst possible meaning in comments is also toxic.

                                  1. 2

                                    I tends to write such comments as “If you remove ___ this function will not affect/be affected by behaviour/changes in ___”.

                                  2. 5

                                    My personal failing: #3, asking people to fix something they didn’t cause because I happened to spot it adjacent to their change in their PR. I shall endeavour to stop this.

                                    For number 4, I often comment freely during the review, but look over my comments again before submitting. If it feels like a lot of comments, I often delete the ones I care less about and focus on a few that I think have the biggest impact. Often addressing those will naturally address the minor comments as well.

                                    1. 5

                                      Interesting post. I kind of disagree that

                                      This component should be stateless.

                                      is worse than their proposed fix of

                                      Since this component doesn’t have any lifecycle methods or state, it could be made a stateless functional component. This will improve performance and readability. Here is some documentation.

                                      This just rubs me the wrong way. The revised comment doesn’t actually ask the person to do anything. And if you’re not going to ask them to do anything, then why leave a comment at all? I think it’s important to actually make it apparent that this is something you’re flagging and would like to see fixed in order to approve their PR. Not to mention, doesn’t this just make “This will improve performance and readability” the part that’s passing off an opinion as fact?

                                      “Could” is such a meaningless, wishy-washy word here. Of course it could - are you telling me you think it’s better? Are you just throwing out a random option with some documentation you want me to read, while not being totally invested anyway, and you want me to defend what I’ve done?

                                      It feels a bit condescending if anything, like “oh, I guess I just wasn’t informed enough” or else I would have known about this obviously better approach.

                                      But anyway, if the author here takes issue with the idea of stating opinions as facts, then perhaps a better way is a simple:

                                      Please make this component stateless.

                                      At other times, it’s appropriate to not make a direct request, perhaps when you’re somewhat junior or you’re not positive yourself and just brainstorming a suggestion. But then, contrary to the article’s version, you should make it explicit that it’s a suggestion and you’re soliciting feedback:

                                      What do you think about making this component stateless? Often times that improves performance and readability, though I’m not positive that’s the case here.

                                      I like to think of it in terms of “emotional labor”. Basically, make it easy on the author to respond to your PR, and don’t waste their time. If you feel strongly that your way is better, make the explicit request and the person can simply implement that change without thinking about it too much, even if they’re personally on the fence about whether it’s actually better. If you’re really not sure, leave it a question or give them an easy out, so the person can choose to not make your change quickly. But don’t leave it ambiguous about how invested you are in the comment. “This could have been done this way” is not super useful, less so with a bunch of documentation to read. Now the author is in the position of trying to infer how invested you are in the comment, whether you actually are asking for a change, or asking a question about their approach vs yours, and whether you should try to defend yours or what. There’s rarely an unequivocally “best” approach, and so an informational request-less comment invites the person to try to re-litigate for themselves the pros and cons of two different approaches they likely already considered.

                                      1. 5

                                        If you notice someone being unhelpful during code reviews, consider letting them know (if you feel safe in your role/company doing so) and be direct.

                                        My experience : I’ve dealt this with, and it was difficult. The abrasive person had more seniority (based on time served at the organization, not based on experience).

                                        1. 3

                                          Github’s “suggest” feature where you can simply suggest a fix for a number of lines and the author can commit that change from the web UI can save you from trying to phrase your change suggestion in words and you have an actual snippet of code to discuss further.

                                          1. 2

                                            The software world is full of gordon ramseys and ramsey is a mirco managing fraud.

                                            1. 3

                                              I don’t follow the connection between your comment and the post. Would you please explain?

                                              1. 4

                                                Sure. Have you seen kitchen nightmares ? Toxic reviewers are like that.

                                            Stories with similar links:

                                            1. Unlearning Toxic Behaviors in a Code Review Culture via GeoffWozniak 3 years ago | 23 points | 4 comments