Unlearning toxic behaviors in a code review culture
medium.freecodecamp.org
medium.freecodecamp.org
1. Carefully read and grok a small-to-medium sized module. 2. Notice that although the style is not the "official" style of that language, that it is very consistent and legible. 3. Contribute a small change in that exact same style, such that no one would know that it was a different author if they did not look at the git blame. 4. Get a condescending comment from the first author asking that you, "read and follow the official BLANK style guide".
I've spent way too much time getting feedback that I either disagree with and end up arguing over, or feels completely pedantic or time-inconsiderate (e.g. loosely-specified comments about code style during a tight deadline), that could be eliminated by a few things:
1. identifying and documenting the value that the code review is intended to provide.
2. constraining criticism to those that actively provide the intended value.
3. where a decision point arises based on reviewer/author disagreement, create a consensus decision within the team and document it in a standards doc so that the debate never needs to be had again.
In my experience, when this process is followed well it leads to a feedback loop that accomplishes the following:
1. Generating a consensus-driven standards document based on the primary values of the team.
2. Creating a code review process that is exclusively aimed at "linting" new code against the standard, and raising exceptions where the standard does not provide a "correct" approach.
3. Ensuring minimal discrepancy between code reviews coming from different devs.
The process doesn't even need to be that formal - just 2 github wiki pages, one for the standards and one for the code review process.
While it's true that not all opinion is fact (uh oh -- 5. sarcasm) there is nothing out of line in asking "Why didn’t you just do ___ here?" How you approach this question has a lot to do with your own perspective. If you are feeling defensive, it can seem like the code reviewer is calling you out. But if you have experienced people reviewing your code, this is a question that you will get a lot.
"Why didn’t you just do ___ here?" often simply means "I don't see why you couldn't have done it this simpler way, can you explain"? It's often for everyone's benefit that things are done in the simplest, most obvious way possible, and it's not productive to take offense at being asked to reconsider how you accomplished something.
It's a more complete statement and it avoids an extra back and forth for someone to say "What do you mean?", which is usually the response to "Why didn't you X?"
And it still accomplishes the same goal of opening a conversation, while better making clear you are seeking understanding.
It's also more useful to people who might refer back to the PR later, which happens surprisingly often in my experience. I try to treat PRs a little bit like historical documentation.
Suppose the code was "list.extend([1])" (Python). And the comment was "why didn't you just list.append(1)"? That would be a completely reasonable thing to say.
I think what I want to caution is that there is a middle ground that this article is ignoring. Yes, sometimes it is uncouth to just say "why didn't you just _". But it can also be important to have a little bit thicker skin, as sometimes you will get a comment that looks just like that, and there will be a good reason.
A better approach is to use the less loaded "have you considered doing X here?".
If you find yourself needing to demand others not take offense at your questions, you might wish to try a different approach.
A "Why[...]" question asks for why.
Assuming that "Why didn't you just do X?" isn't really just a disingenuous way to say, "you're an idiot; do it this way instead", then you should ask for what you want and ask the why question, not the yes-or-no question. In the case that it is a disingenuous question, then that's something that's covered adequately by #5. Suggesting the avoidance of sarcasm is already broadly applicable enough to cover using questions when you really mean to make a statement. (Which is not specific to code reviews—it's as obnoxious in real life.)
After that I usually reply with something like "Could you add a comment and/or test highlighting this reason? (So the next time someone modifies this code they don't make the same mistake I did.)"
Definitely better results all around.
agree with #2 in theory but in practice some authors are crazy sloppy. i usu. write “here and elsewhere” the first 2 times for a given author and after that i break out the copy paste bazooka.
his example for #2 is also terrible. “trailing whitespace” is perfectly sufficient review comment. you don’t need to write a novel and both parties have to understand it isn’t rude, it’s shorthand because life doesn’t need to be spent in code review.
likewise, disagree with unhelpful #7. as reviewer, if a comment wasn’t replied i get it. fine. move on. I’ll track authors and learn which ones ignore because there’s no value in arguing, or those that ignore because of not getting it in general or not getting that a specific comment was incorrectly thought trivial (and ou also have to consider if you played a part). jesus, you can’t take it personally if your comments are ignored. it’s shorthand. i don’t want to go through and read tons of “yes ok” it’s a waste of both parties’ time.
i agree with all the rest!!
although, some of the behaviors he talks about are indicative of toxic culture and it’s just expressed in code review. more important to fix that (or leave) if that’s the case.
For #1 don't hurt their feelings, for #2 teach them instead of expecting them to know.
Going through the points in the article with these lenses:
"This component should be stateless" is reasonable feedback to give to someone familiar with React. Maybe they had a good reason for not making it stateless, maybe they made a mistake, but giving an impromptu lesson and providing references probably isn't appropriate -- it's condescending. Again, fine for someone new to the technology. Audience is important.
The "Avalanche of Comments"... As someone who doesn't really do it, and as someone who has been on the receiving end of it, I always appreciated the thoroughness. It's not judgmental, it's a checklist, and it's helpful.
"Spew emojis" hit this taxonomy too. If my code is ugly enough to warrant one (and it sometimes is), I want to know it has crossed a line, and someone telling me that I am a terrible person who has ruined their day (or given them cancer) is a perfectly reasonable, light-hearted way to do so. Constructive feedback is useful too, but for a senior dev they probably knew it was ugly and just need telling that it was too ugly and that they should try harder. Obviously not appropriate feedback for junior devs, though.
My disagreement with #3 doesn't fit that taxonomy though, mostly I just hate process. Most refactors are super fast, fairly mechanical jobs. Both you and the reviewer have that code in your cache, and it will likely never be a priority again. Take some initiative and rewrite the thing, it'll only take 20-30 minutes. Making a ticket increases debt instead of paying it down.
The topic at hand is harshness of language used providing feedback on another person's work. In addition to being a developer, I've also worked in garages where people swore like sailors and ball-busting was common. Calibrating my voice for the context starts with understanding every critique has: the harshness intended, the harshness communicated, and the harshness received.
This article focuses on the 2nd, leaves out the fact the internet makes it notoriously difficult to communicate voice through text, and assess using the common buzzword "toxic." Which, as far as I can tell based on context clues, is intended to mean "anything harsher than my personal preference threshold."
Just as important are parts 1 and 3. The feedback "pull your head out of your ass" can be run of the mill, or beyond the pale. It all depends on the context and participants. Note: This is very much NOT a point about "having thick skin," it's about understanding how sharp the comment is in the first place.
Each critique also has: the amount of ego hit to the recipient intended, and the amount of ego hit perceived. This one is about "having a thick skin," and where we might ask, "How harsh is too harsh, depending on [bucket of other factors provided by the context]?"
I'm not looking for a dissertation, but I expect people speaking on the topic to show an understanding of the complexity at hand. Otherwise it looks like a pack of schoolmarms trying to enforce I-know-best standards.
A piece of career advice I received can be paraphrased as: "Software development in Silicon Valley is full of people from the participation trophy generation that have only ever held back-office jobs. And it's San Francisco." -- That's all that needs to be said. From that we can derive what the standards for behavior in code reviews are.
And if it sounds like I'm implying that the hegemony in software development demands we treat each other with kids gloves, it's because I am.
Go through your code review thoroughly, and only make one set of comments. Of course, if you happen to find a huge bug then that necessitates more reviews. However, I think that reflects poorly on the reviewer, she should have spent more time reviewer more carefully the first time.
However, if the above process on the developer's side is not quick, then no matter if the comments are batched together, the whole thing will take a long time. This is usually a sign that the wrong person is doing this specific module, or that they need more mentoring/training/firing.
But reviewees should also learn how to take criticism well, and develop the thick skin to shrug off when the reviewer actually is being an asshole. You can't go through life never expecting to encounter an asshole or someone who likes to bully people. Better to know how to shrug off shitty behavior rather than become obsessed with how your feelings were hurt.
So here's a thought: if you see someone being an asshole, and you're thinking about blaming anyone other than the asshole, don't. Instead, work on making it untenable to be an asshole.
But you will also encounter hundreds of assholes in every aspect of your life. If every time you encounter an asshole, you break down and cry and can't go on with your day, the only one that suffers is you, not the asshole.
Also, fearing hurting someone's feelings becomes the lowest common denominator, and leads to everyone walking on eggshells.
Case in point, a fresh grad had her code reviewed by the team, and it was completely professional and non-judgemental. She burst into tears at her desk because she had never gone through this and took the criticism personally.
Were we too harsh, or was she too sensitive? If people burst into tears because of professional code reviews, do we lower the bar as a team to account for her sensitivity, or does she raise her bar and thicken her skin? And who determines what that line is?
Which is the same as "I'm not going to do anything about this asshole, because there'll always be assholes".
Given that approach on your part, one wonders what it really was that caused the "too sensitive" new member of your team to react.
(hint: your new team member wasn't "too sensitive", you've probably just been tolerating assholes on your team -- because "what can you do?" -- for so long you've forgotten what assholes they are)
And hint, the reviews were perfectly fine and professional.
There is a way to have high standards, not be an asshole, and not "coddle" people. As they say, it's not what you say, but how you say it.
This usually comes up with new employees because they haven't learned that one senior dev on your team "has a heart of gold" even if they are a super-asshole in text. "Everyone knows he just cares so much about the code." Already heard it.
Don't forget there's a long history of men talking down to women and using what seems like neutral language, but subtext that implies that she's incompetent, doesn't deserve the job, slept her way to the top, etc, etc.
Clearly though, you've decided that the problem is her. Hopefully she found a more supportive team elsewhere.
The default state of most people is that any criticism is a painful insult. I deliberately try to accept and learn from criticism, and have been told I take criticism unusually well. This pleases me.
That doesn’t mean I accept assholes. I used to accept them, but I learned to rid myself of them at the same time I learned to accept valid criticism.
Unfortunately I don’t know how to reliably give feedback that others find easy to accept. I wish to do better.