Here's one that always has me stare at the screen, quietly, in disbelief.
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 rather work with you then the other guy. If you add your changes using the new style guide, it will look weird and someone will complain. If you rewrite it all, someone will complain because it was a waste of time. Whatever you do, it will be wrong. The solution is to go ask the most senor and respected dev what to do, then redirect to him/her when others complain. =)
I'd create an initial separate commit with only style changes and then have my new or changed code follow the code standard. The code converges very fast if everyone does that.
Caveat: Try to time it carefully if the code is being actively developed, to reduce risk of merge conflicts and annoyances to the rest of the team. It just leads to a lot of "stop doing that!" and further resistance to that kind of refactoring.
Well, this makes sense somewhat. Files will be edited incrementally to the point that nothing remains from the original. If you follow your strategy, the original style remains there until a large rewrite.
If you make a point to always follow the official style, then at some point you'll automagically have a consistent codebase.
I think it depends on your environment. I've been in situations where code styles have progressed and changed over the years, faster than the file does, so you end up with a hodgepodge of different styles.
I have a potentially controversial opinion on code reviews - that they have a low enough signal-to-noise ratio that they provide minimal value, unless they are conducted using a formal but living checklist/process. I strongly believe that code reviews have to be goal-oriented or they become vectors for excessive opinion airing (read: bikeshedding and holy wars).
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.
Totally agree that they should be goal oriented and checklist driven (as should most processes in a company). Investing in tools like linters and formatters can cut into a lot of the noise too.
Agree with all of this except 4. "asking judgmental questions".
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.
I think that's a perfect example of what the article is about. Instead of "why didn't you do X here?" You can say "I would prefer X here, because I value Y" and then add your reasoning.
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.
I think the suggestions in the article are the victim of a lack of context, in a certain way.
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.
The problem with the question form "why didnt you just do ___ here?" is it's loaded with the assumption that the other party should have known to do this other thing instead.
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 "Have you considered..." question asks for a "yes" or "no".
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.)
I've used "why didn't you just ____" version plenty of times, and in a handful of them, they replied with a paragraph or two outlining the exact reasons - including a very edge-case issue that I'd not noticed.
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.)"
And if you get something along the lines of "of course I have considered X!" as a response, someone took that one as the loaded variant. It's tricky. "What am I missing?" or a variation on it, as the parent mentions as the intended meaning, probably isn't a bad thing to add, since it makes it clear that you aren't accusing.
I often ask questions like that because I genuinely assume there is a good reason something was done a certain way but its not obvious to me. Maybe they made it over complicated by accident or, more likely, since I didn't spend as much time thinking about this code I just missed something during review. Maybe there is a way that can better convey that sort of genuine curiosity.
I've found it helpful to lead with my explicitly labeled expectation I had that the code doesn't express and the reasoning I had for the expectation followed by a request for context. Sometimes better factoring wasn't top priority in the set of many concerns being solved for and sometimes a technique was not known. Oftentimes my expectation wasn't talking interactions in the code into account and the education I receive helps me better review the code or better understand the codebase. I mean, we could deal with the noise of conflict or we can more efficient solve the problem that motivates our effort in the first place.
Code reviews are hard. It takes a lot of time to really review something and understand it. I think it's much better to discuss plans up front and get buy in from other people before and while you write the code. Obviously that requires an open and respectful culture.
Let's just say having some continuous dialog while developing is preferable over review after everything is done. I have that kind of relationship with a few people and it works great.
In a healthy environment, sure. In an unhealthily micromanaged one, you may want to push the review to the end so that you can document that the review process with this person took an extraordinary amount of time and delayed rollout of a feature.
Code review should be informal and conversational, these guidelines are the opposite of that. I'd say the real problem here is people not having the rapport neccesary for a real conversation.
PLEASE READ THE POST. "informal and conversational" are EXACTLY what this poster posits. The problem is that WHAT is considered "conversational" is subjective without exact and precise examples to guide by. The poster provider these examples in extreme detail both for Negative and positive examples. PLEASE READ THE POST.
disagree with unhelpful #1. it is understood that “facts” are opinions. the reviewer doesn’t need to write an essay unless he gets pushback from the author. the author needs to understand that the presentation style is just a shorthand.
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.
Mostly I think this is a mix of (1) "good advice for dealing with junior devs" and (2) "good advice for dealing with people new to a technology."
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.
Do companies usually have guidelines and maybe even training on how to do peer/code review? With some emphasis on the empathy and the way you communicate your thoughts? I've noticed a broad range of peer/code review in my little career and in hindsight it was probably due to the fact that no one really knew how to give a peer/code review.
Great post. I would add a caveat that personal relationships can change the rules a bit. When working with developers that I know very well, and that know me (that's important too) super brief comments, and even sarcasm, can be helpful. A friend and I were working on a nasty piece of code and we used the barf emoji as a way of commiserating :-)
This kind of article dances around it's point in the unproductive way typical of the topic. Here's a condensed version of issue, rather than talking about symptoms:
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.
One thing that I ask people on my team to do is only have one set of review comments. What I hate is when you address a bunch of code reviews, and then the same reviewer comes and finds more things to change. Then it's a never-ending cycle of fix and change, fix and change, which wastes everyone's time.
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.
I found this only to be a problem if the person who wrote the code being reviewed is slow at writing/changing code. It shouldn't be a big deal: Checkout, fix, re-run tests, commit, push, tick off task.
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.
Another I've noticed is not reading the ticket. I've seen a lot of reviewers not take the time to look any deeper than the code at face value so the feedback they provide is low hanging fruit stuff. My favorite feedback is when someone helps me understand the ticket better, or spots potential risks that I hadn't seen.
One other thing the code reviewee needs to do is develop a thick skin. Reviewers never be assholes, because it's unproductive and makes your coworkers not want to work with you.
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.
"Grow a thicker skin" is, usually, identical to "let's just let people be assholes to you and say it's your fault if you complain about it".
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.
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.
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?
False dichotomy: either you have shit code or you coddle the feelings of your junior dev, which you helpfully outed as female for no reason.
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.
you will also encounter hundreds of assholes in every aspect of your life
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)
