3 ms·

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

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.

repsilat 9y ago
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.

repsilat 9y ago

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.