13 ms·

I ruin developers’ lives with my code reviews and I'm sorry

166 points by aracena 5y ago

lhorie 5y ago
Congrats to the author for realizing that confidence doesn't equate skill. This isn't being a pink unicorn, it's simply humility. The real danger of confusing the two is that real skill dulls if you let overconfidence take over, since the very thought of being "better than others" prevents you from accepting that maybe there are things that you don't know or haven't considered. For example, is "destroying" a coworker's commit in a code review really really helping? Did the person perhaps forget to consider that hiring to refill the position of a fired developer actually costs physical money (as opposed to time spent on coaching?) or that perhaps a lot of time is being spent on nitpicks without bottom-line value or that tip-toeing around egomaniacs can stunt autonomy and therefore creativity? This is not merely a matter of whether you're lulling yourself into some false sense of superiority/duty/whatever; thinking in terms of bigger and bigger pictures is an actual skill that one needs to develop when going up the career ladder.
- blahyawnblah 5y ago
  If you have to "destroy" every PR from a developer, you're wasting more time (money) than it would take to replace them. Why should be bad PRs ever be allowed for any reason?
  - fernandotakai 5y ago
    bad PRs shouldn't be allowed, but you don't have to be a smug a-hole while reviewing PRs. hell, that's a GREAT opportunity to get this "bad developer" under your umbrella and start mentoring him to be a better developer.
    - fayten 5y ago
      Yup! I have been very fortunate to work with extremely kind and empathetic senior engineers in the past. They have done wonderful jobs at using the code review process to teach instead of tear anyone down. I am always shocked at how many programmers become so aggressive about development. It might be silly, but I think the xkcd comic Ten Thousand is a great mantra: https://xkcd.com/1053/ https://xkcd.com/1053/ Teaching people cool new ways to code should be fun, not an exercise in showing everyone the size of your ego.
  - nine_zeros 5y ago
    > Why should be bad PRs ever be allowed for any reason? Define bad PR. Are you sure that your definition of "bad" is accurate?
  - n8cpdx 5y ago
    There are many ways to solve any problem. If I reviewed every PR with the standard "is this exactly how I would have done it", I too would be destroying every PR I review. And I'm not even that good at coding - I'm the type of generalist who cares about things like the business beyond the code base. What really matters is whether the code is solid and whether it will actually cause problems. Just because a PR isn't character-for-character how I'd write it doesn't mean it's bad. Based on the author's experience with the fired employee's replacement being just as bad, my guess is he was over defining goodness and raising the bar too high. Bad PR to me means: bad runtime complexity, bad formatting, bad security, bad memory use, bad understandability. If it meets all those bars, which for a crud app is usually easy, then it should probably pass. I'll sometimes leave a "I would have done _x_ differently" comment on matters of taste _if I think it will help the other developer_.
    - marktangotango 5y ago
      > If I reviewed every PR with the standard "is this exactly how I would have done it", This, too many people think their way is the only way. It only takes one cancelled project or one company going bankrupt (and 100,000s loc going "poof") to understand this work we do, really doesn't matter all that much in the end.
      - JamesBarney 5y ago
        I've spent over 6 years working on 5+ legacy projects that had millions of lines of code and maintenance teams 30 times smaller than the army of contractors who developed them. And I've learned all the small nit-pick maintainability issues made no difference. Whether or not there was a style guidelines, and whether or not it was enforced made little difference. Things that really made a big difference were giant bone-headed architectural decisions like "just save persist this data as serialized text", "lets write all the business logic in stored procedures", "building the software on a terrible platform". And having some type of identifiable domain model helped a little too.
      - BoxOfRain 5y ago
        It is a really humbling thought that every single one of our entire life's work as developers is one unfortunate astronomical event away from vanishing as though it never existed. Kind of puts it all into perspective really.
  - rkk3 5y ago
    Of course there are thresholds for competency. But if you find yourself having to destroy every PR from a developer you're probably toxic & insufferable.
  - mjlawson 5y ago
    When you're in these situations, you are faced with a challenge which isn't solved by code reviews no matter how illustrative, nice, or demeaning they are. It's first solved by identifying what the cause is of their poor performance. Are they in over their head? Are they having personal problems which are distracting them? Are they experiencing burnout? I've heard people say that they enjoy code reviews because it gives them latitude to be ruthless. After all, it's for the benefit of the business to not allow suboptimal code through, and it's just code. But frankly, I think that's a lame excuse and the easy way out. There are people behind code, and while the code might suck, the people don't. Obviously, that doesn't mean you should let it through, but it does mean you should be at least professional, and preferably compassionate in how you review it.
  - JamesBarney 5y ago
    A bad PR can mean soo many different things. Ranging from "I wouldn't have implemented it that way" to "this opens up a huge security hole". Security issues shouldn't be allowed in. But a lot of "maintainability" issues have negligible if any gains, and real costs like it takes time to fix could introduce new bugs creates bad feelings between coworkers
- robohoe 5y ago
  Humility goes a long way in life. Anecdotally, it's definitely something a lot of software developers and systems engineers should learn how to practice.
adamnemecek 5y ago
God he sound insufferable.
- watt 5y ago
  The russian version is somewhat self-deprecating in tone, the translation is not doing good job conveying it.
  - DoreenMichele 5y ago
    Thank you for saying that. I read the piece because of your comment and hopefully understood it a bit better because of it.
fwip 5y ago
> And if you tell me that you haven’t had this feeling ever, then you’re lying. Tell me about higher goals, training rookies and all that — I know you’re simply too full of themselves. And if you try to tell me that you learned to defeat that feeling (however it manifests in you), then I must be a pink unicorn. I think that this is called projection.
- fayten 5y ago
  That line also annoyed me. I think this person has a lot more self reflection to do about their insecurities. I am glad he has begun to recognize there are some issues and I hope he is able to continue to improve himself.
papaf 5y ago
I have been on several teams that found the following article "Unlearning toxic behaviours in a code review culture" very helpful: https://medium.com/@sandya.sankarram/unlearning-toxic-behaviors-in-a-code-review-culture-b7c295452a3c https://medium.com/@sandya.sankarram/unlearning-toxic-behavi...
- andreshb 5y ago
  This should be basic training anywhere people learn how to code.
- sbacic 5y ago
  I'd like to point out that every one of the "correct" examples is longer, sometimes much longer, that the "wrong" example preceding it. I've found that I usually start with the "correct" way and end up regressing to the "wrong" way as more and more work piles up, stress builds and deadlines approach. I guess the "correct" way only works if the organization actually prioritizes it and gives reviewers the time to do it the "correct" way no matter what stage the project is in.
backoncemore 5y ago
Nothing makes working at a place more unbearable than having to deal with someone that gives an excessive amount of criticism on working code, at least that's how I feel. I've been in that situation before and it made me never want to submit pull requests. It made me even madder when other developers, whose code was no better than mine but had been at the company longer, received basically no critiques. Some might say, oh it's hazing, well fuck you because I'm quitting. I'm not putting up with any amount of hazing at a job. If I have to spend 8 hours everyday somewhere I'm for sure not going to spend it with some asshole.
- billycorben2 5y ago
  Strange, I’m on a team rn with people who just approve everything. I’m craving some constructive feedback.
  - smoldesu 5y ago
    It's much easier to ask someone to be critical than it is to ask someone to go easy on you.
  - onlyrealcuzzo 5y ago
    Not to be rude - but... have you asked for it?
    - billycorben2 5y ago
      I don’t think this is rude. I haven’t asked them. I should ask for more feedback. I haven’t asked because: I was a latecomer to the team and they had an established process Cultural differences between myself and them Honestly, I’m not sure all of them are experienced enough to give a useful critique
      - vchuravy 5y ago
        Code reviews can also have the opposite effect, I sometimes ask mentees of mine to review PRs of mine. Not necessarily to get their feedback, but to get their questions and to help transfer knowledge. I used to do that when I started working on open-source as well, doing code-reviews to learn the code base (often I would not submit the review to not create noise)
Justsignedup 5y ago
i have been on an ever onward journey to try to find the balance: - is a developer making the same set of mistakes? send them information on a better strategy to try overall - how bad is this mistake? Is it just something that I know how to do better, or legitimately a broken way of thinking? - what is worse, a slightly inefficient or unpretty algorithm, OR someone researching the problem / solution Point is, 90% of my comments were meaningless. I really comment when things look wrong -- Did they misunderstand the intent of the ticket, did they verify everything with product because this looks weird, did they check the edge cases, did they forget about this other behavior, etc. All that I just mentioned is the real meat-and-potatoes. Code-fu is important, but less important than the other stuff. And if you are a calm, trusted, and mentoring team member, those same people will come to you to up their code-fu voluntarily.
deleted 5y ago
[deleted]
- deleted 5y ago
  [deleted]
throwthere 5y ago
I guess if you try to call "feelBetter" right after "throwingShit" you'll never get to feeling better. Maybe that's the point being made?
notamy 5y ago
(2019)
deleted 5y ago
[deleted]
tornato7 5y ago
I had an incident many years ago where I found glaring design flaws in a much more senior developer's code, and, overcome with some feeling of superiority, left a million nitpicky comments and a snide summary, then posted on our team chat "left a review". Finally, I was getting back at all the senior developers who had torn apart my code. I later realized my comments were mean and I apologized. While you could still potentially call it constructive criticism, there is a difference between mean criticism and polite criticism. I do think thorough feedback is important, that's how I've learned the most after all. But typically now if I I'm going to write more than 5 comments, I meet with the other developer in person instead. That way the code problems are solved faster and it avoids any sort of public humiliation.
majormajor 5y ago
> If a guy brings me his code, and it has mistakes, it brings insane pleasure from how smart I feel [...] And if you tell me that you haven’t had this feeling ever, then you’re lying. Tell me about higher goals, training rookies and all that — I know you’re simply too full of themselves. And if you try to tell me that you learned to defeat that feeling (however it manifests in you), then I must be a pink unicorn. I really can't relate at all. I feel the opposite. I often feel angry (I realize this is not a GOOD response either!) and always feel sad. I don't want to have to do more work. I don't want a reminder that my previous feedback didn't click with them. I'm constantly looking for ways to do less tedious work, through automation and such, and code review for bad code is the worst sort of tedious work. I want to learn things, but code reviewing bad code doesn't teach me anything - I want to work with people who can teach me things when they review my code just as often as the reverse. My takeaway from that has been to try to intercept people earlier in the planning and designing and start-of-coding process. If a bunch of bad code gets to code review, someone failed at explaining what this thing needed to do and how it should be done beforehand. The article claims this is always just itself a confrontational form of argument, but... it doesn't have to be. Stop being an asshole, and find non-assholes to work with.
- watwut 5y ago
  My reaction to this was that this guy should not be making code reviews ever. That emotional reaction in combination with conviction everyone has the same one disqualify him.
  - MisterBastahrd 5y ago
    I wouldn't even want that guy on my team as a junior, because he's a headache waiting for a place to happen.
    - smoldesu 5y ago
      I'd expect a neatly-worded blogpost on the company Medium: "I Destroyed Our 55m User Database, But Here's What I Learned:"
  - 5y ago
elwell 5y ago
What would happen if all PRs were submitted/reviewed anonymously, and the feedback was just as impersonal as a stack trace?
- duxup 5y ago
  Anonymity on the internet doesn't seem to provide much in the way of civility.
- jimmyvalmer 5y ago
  As someone for whom face accountability is the only thing preventing me from eviscerating my peers, this sounds like a dream come true.
rootusrootus 5y ago
If you want some empathy with the guy whose code you are reviewing, go pull up some of your own code from a few months or a year ago. Do a mock code review on it. Maybe everyone else only writes good code, but this works for me every time. Even after over 30 years coding, I still have no problem seeing how imperfect the code is that I wrote as recently as last year. And don't ever make it personal, anyway. Adopt a neutral tone, phrase comments abstractly as how they relate to team goals, how the code might be made clearer, etc. Don't accuse, don't belittle, don't try to look smarter than you are. And try to recognize the difference between the stuff that actually matters, the things which will actually be a headache down the road, and the pointless details that are just not how you'd have chosen to code something.
clarle 5y ago
Having an auto-formatter like Prettier or `go fmt` has been a huge boon and productivity gain for the teams that I've worked with. No longer having code nitpicks on formatting has saved so much time going through reviews, and it really increases the signal to noise ratio on pull request comments too. Would definitely recommend those for any teams that have a similar teammate as the article author on their team.
- mrtranscendence 5y ago
  Yeah, we mostly do Python and I've really been trying to push the use of black. I don't actually like some of black's formatting choices (OK, it's grown on me) but it's better to have a standard and stick to it.
ratww 5y ago
Ouch. I’ve had a few cases of developers on the verge of being fired because of bad code causing friction with the team. Code reviews never help in those cases. There is always an underlying cause: the developer is trying to rush to prove something, or maybe over-engineering for the same reason. Maybe he has some issues at home, or isn’t sleeping well. Maybe it’s just lack of experience. Maybe it is a personality issue and the developer is trying to save face by sticking to his guns. Either way, I find that it is much better to have private discussions with the developer to increase the code quality via mentoring, or pairing. It helps removing the defensiveness, doesn’t make them feel publicly humiliated and doesn’t spend the time of the team with the review-fix cycle.
mrtranscendence 5y ago
I'm not a super-coder; give me a JS project of moderate complexity and I'd have no idea what's going on or what I'm supposed to do with it. But as a data scientist I do have the reputation of being good at coding even among some of the engineers at my company. And some of the code produced by both data scientists and engineers ... it's, hm, not great? Just between you and me? So I'm glad I've never had the inclination to tear anyone to shreds. Indeed, part of my job used to be to help people with their coding problems, and I loved doing that. But sometimes when I'm reviewing code ... it's very hard to bite my lip and not call out every little issue, even though it would just be too much and most of the "problems" aren't actually very important. I try to impress upon people that developing good code is useful when I lead training sessions, but it's hard, especially for people who'd rather be doing data science than coding.
2OEH8eoCRo0 5y ago
Funny. I'd say that if somebody can't work with or maintain code written by others then that's the sign of a weak programmer. If all code you work with has to be written exactly as if you wrote it yourself then that's a weakness.
neo2006 5y ago
90% of a code review is not about finding THE BUG in the PR but asking questions and making the author find his own bugs or learning something from him. If you are performing a code review hunting for bugs you already lost your humility and think you are better then the PR author.
- AnimalMuppet 5y ago
  Have you ever written some text, and re-read it, and it sounded just right, and had someone else read it, and they said "what does this mean"? Sometimes the words you write are sufficient to trigger in your own head the thoughts you've already had, but not enough to make someone else think those same thoughts. Code can be like that, too. You write code, and it's enough to convince you that it does what you think it does. But someone else reading it can sometimes see the holes that you don't see. Assuming that person X writes bug-free code is bad, whether X is "you", "me", or "that co-worker". I don't have to be better than the PR author. I just have to be decently good, and be a different person. That's enough. And of course I'm looking for bugs. That's the absolute first thing to look for! Now, true, if I can help the author be able to see their own bugs (asking questions, maybe), then that's probably better than me just lecturing them. Teach them how to think so that they can see it themselves next time.
  - neo2006 5y ago
    I'm not saying that you will never find bugs in someone PRs I'm just saying it shouldn't be your first intention, your first intention should be to understand it and be convinced by it. Finding bugs will be a side effect of that. If you reading the code to execute it in your head and find all the weird combination that could trigger a core dump, you should probably check if the author wrote tests for those use cases instead and comment about missing tests. Genuine interest in others work can go a long way compared to imagining how would you solve it like the article author said.
quxpar 5y ago
Maybe the tone doesn't translate but this guy sounds insanely egotistical. I feel truly bad for his family and anyone he works with.
- Doctor_Fegg 5y ago
  It strongly reminded me of this graph of different cultural norms for phrasing criticism: https://mobile.twitter.com/sebjilke/status/1036406794359173120 https://mobile.twitter.com/sebjilke/status/10364067943591731... (I believe TFA is Russian, so read the Eastern European graph)
  - dwaltrip 5y ago
    Very interesting to see those cultural differences. Hilariously, the first graph is titled "Objective distribution" (of criticism phrasing). That's not how language works...
lhnz 5y ago
In my estimation, most places have the opposite problem: everybody approves the code without looking through it, and the only comments that are left are stylistic problems. Good code reviews spot real bugs and provide solutions and explanations that help the owner to improve their branch. It's completely constructive and feels like going out of your way to way to help them and paying attention to what is meaningful -- the vibe should be a bit like "doorman at a posh hotel providing excellent service", so not disrespectful at all. Also, if you're nitpicking, it's a sign of lack of focus...
deleted 5y ago
[deleted]
jp57 5y ago
I had an experience in my first year of grad school that has informed my code reviews and my general attitude toward reviewing others' work ever since. I was taking a graduate-level introduction to mathematical logic. In the class we learned about a bunch of different propositional axiomatic proof systems, and then we proved properties like soundness and completeness for them. The class was organized around class participation: everyone was required to do at least one proof at the board in front of the class and every session consisted almost entirely of students doing proofs at the board. I'd been out of school, working, for eight years before grad school and I found this all very intense and unfamiliar. I did my required work at the board, choosing an easy problem, but otherwise mostly sat in the back and watched. There was, however, a cohort of very intense aspiring logicians who sat in the front row and took deep interest in every proof. One day, a student went up with a particularly challenging problem. From the start it was clear that he was taking an unorthodox approach to the proof. The front row went nuts, interjecting and kibitzing on practically every line, telling the guy different things that he should do instead of whatever it was that he was doing. The guy was a good sport and tried to justify everything and answer all the criticism, but it seemed like he might not get through the proof before the class ended. The prof who had been watching quietly from the side of the class, let it go on for a while, then stepped out and quieted everyone. "I need you to remember," he said to the front row, "that we are the audience. It is not our job to prove this theorem; it is this gentleman's job. Our job as the audience is to listen to him, and at the end, decide if we are convinced. He may do things differently that you would have done, but that doesn't matter, as long as in the end we are convinced." That was twenty-three years ago, but it stuck with me. In programming, there are often many different ways that a problem could be solved, but there is almost never One True Way™, even though there are often many voices that would like to convince you that their way is the right one. My job as a reviewer of code, or really anything, is not to try to get the other person to do what I would have done, but to decide if I'm convinced that the way that they've chosen is good enough, and, if not, to ask for changes that would convince me. Sure, sometimes a PR is so bad that it needs to be completely rewritten, but in my experience that's rare, and in that case the feedback should probably be given in person and in private or in a friendly, sympathetic group.
- jimmyvalmer 5y ago
plutonorm 5y ago
This except there is no right way and there are 'n' equally good ways of solving any real world problem. It's only worth pointing out flaws if something is genuinely bad and I mean bad. Otherwise you are using a gut opinion to trash someone else's emotional health. Yay! you found an edge case that breaks a piece of code that will never get hit while the other million and 1 edge cases that haven't been found are still out there. Yet you drag it out from the dark and show it off for all to see like some damn trophy. Yay! look I am great I found one more thing wrong than the other guy. No matter that we are all swimming in a sea of wrong and every one is completely clueless. Only slightly less clueless than those who don't even realise they don't have a clue. It follows from a simple argument - every programmer and every company has a different view of 'good'. I've seen it, I've worked in many. Just as every religion cannot be right, every weird view on correct programming style cannot be correct.
- vdqtp3 5y ago
  > there are 'n' equally good ways of solving any real world problem. This is objectively, mathematically, incorrect. There are optimal solutions, and for some/many problems, there is one optimal solution.
  - plutonorm 5y ago
    well done for illustrating my point
duxup 5y ago
I really haven't coded for long compared to some. I always look at code reviews or just reading / talking about each other's code as a way for us to just get on the same page about how we do things. Make everyone's life easier. "Hey man when we're looping through that thing here is how we usually do it." "Ok cool, I'll do that." Or maybe we have a discussion on why I did it differently this time and how we might address that. Much of the time I don't "care" or feel strongly what the change or answer is. If we're all doing the same thing / spot a bug sooner, it's just easier for everyone. But it seems code reviews to some are this sort of battle / proving ground / have a lot of elements of status. I don't get that...
yupper32 5y ago
There are a few types of comments on code review that I see: 1. Critical mistakes 2. Style guide mistakes 3. Non-optimal design (in your opinion) #1 Always receives a comment, obviously. #2 Always receives a comment, but we have good automation here so it's not too much of a worry. #3 Is where it gets interesting. A lot of the time, commenting about this category is prematurely optimizing and it's a waste of time for both the reviewer and the owner of the change. At most I try to make one overarching comment about the design. You really have to ask yourself "Does it work? Will it cause issues down the line?" If the answers are Yes and No respectively, you should likely let it go. The feeling of ownership over the design, especially for a junior engineer, is incredibly important for moral and the ability to get better over time. IMO it's also important to let them make small mistakes now and again so they can learn from it.
- deleted 5y ago
  [deleted]
- RandallBrown 5y ago
  With number 3 I usually leave a comment but it's something along the lines of "Why did you decide to do x instead of y. I think y would be better because..." I don't necessarily want or expect them to change their code but I want to make sure they're considering multiple options and intentionally choosing that path instead of just copy and pasting something from stack overflow (as an example.)
leepowers 5y ago
> I was, again, technically stronger. A thousand-string pull request was littered with 200 comments, not leaving the person even a faint hope in his competency. Great. That's way too much feedback. It makes it very hard determine which problems are critical, and which come down to aesthetics or personal preference. On the flip side, if I was churning out buggy code at that rate, I'd want to know about those bugs in detail. There's also an opportunity cost of evaluating code so tightly. Not only does it take up more of the reviewer's time, it also eats up developer time that could be spent on other features. > The reason for these angry code reviews is obvious. As part of the team, I bear full responsibility for the project’s code base. I have to work with it later, after all. It’s a source of many issues for the business. The code doesn’t scale, can’t be tested properly, is littered with bugs. Support gets more and more expensive. It can’t be open-sourced or used to lure new developers. Premature optimization? The mistake here is trying to optimize the legacy of your code base, possibly thinking too far ahead.
marstall 5y ago
PRs definitely bring out the worst in some people ... would head for the exit with a vibe anywhere near like this on the team. sad to me that young devs have to go through this.
gchamonlive 5y ago
> The universe blessed me with this trait so I can awaken the anger in other young and inexperienced coders An important skill to develop is the ability to "read the audience". Almost prescient in nature, predicting how people will react to you is of utmost importance so that you can tailor your responses accordingly. This is important when mentoring people because, even though tough love might work on some, it may very well destroy self esteem for others and make them shift career, ending something before it even starts. If you are in a position of mentoring, it is your responsibility to adapt. It is painful, frustrating and a source of contempt when you try and try but you just can't seem to find the sweet spot to inspire newcomers, but as long as you persevere there is hope in teaching others. I had a recent experience in this. I am in no way a long experienced infrastructure engineer, but I built my share of cloud applications to have an idea what can go wrong. I recently tried mentoring someone new and it was just plain frustrating. I couldn't explain too much without overwhelming him with details, and I couldn't just direct to sources without him feeling abandoned. That sweet spot of attention I couldn't reach before he requested to "learn different things" and effectively abandon infrastructure haunts me sometimes.
myk9001 5y ago
I speak Russian and used to read habr.com a lot, which is a Russian-speaking platform where the community members can post on topics related to software engineering, computers, and tech in general. (As you can notice, habr.com have also been trying to gain an English-speaking readers for some time now.) So I read or saw multiple articles in Russian from this specific author. Actually, the one linked in this post is a translation. To put it bluntly, while they clearly have a background in software engineering, they are a troll. All their articles seem to have one thing in common: they look to be as controversial as possible on purpose, I don't believe them to be sincere. And I'll give it to the author, the tactic worked great: their articles usually generated a huge discussion in comments and -- I would guess -- a lot of traffic for the site. Though people seem to got immune to it over time. And there's probably nothing wrong with writing in a provocative style, but you might want to keep it in mind when reading the post and not take it too literally.
- kbenson 5y ago
  > saw multiple articles in Russian from this specific author. Actually, the one linked in this post is a translation. To put it bluntly, while they clearly have a background in software engineering, they are a troll. All their articles seem to have one thing in common: they look to be as controversial as possible on purpose, I don't believe them to be sincere. Is "don't be an asshole when it's actually not helping the other person like you told yourself if you look closely" actually controversial enough on that site to be considered a troll? Honestly, the article comes across to me as someone publicly acknowledging their shortcomings in an attempt to combat them. If that's trolling Russian developers that frequent that site... maybe they deserve to be trolled in that way?
  - golergka 5y ago
    > Is "don't be an asshole when it's actually not helping the other person like you told yourself if you look closely" actually controversial enough on that site to be considered a troll? In Russia, yes. Like the OP comment author, I'm familiar with both IT cultures, and there's a dramatic differences in the amount of politeness. I have actually seen people get into a physical fight because of work-related dispute, more than once.
nanis 5y ago
Everyone is capable of writing crap code ... Code that might give the impression of working but is unmaintainable. PRs are conversations. Every team member should be able to request changes to start a conversation. A conversation about the code should focus on what tradeoffs are necessary to get stuff done now along with a consideration of how comfortable every team members feels they can maintain the code. Teams where PR approvals are considered endorsements, where PRs are rubber stamped, where developers throw fits when others dare to ask questions etc are detrimental to both getting things done and maintaining a sane working environment. Don't confuse the code with your worth. The greatest asset you will ever have is the ability to be openly critical about code you have written while understanding the right level of imperfection for a given circumstance. This only comes with experience. The more you nurture your ability to view your own code critically, the faster it comes. Since we all have blind spots, take others' questions seriously.
glitchc 5y ago
Buddy, don’t blame the yhe industry for your argumentative nature. Blame your dad. And unless the company pays you for every line of code you comment, you’re better off spending more time with your kid.
jhawk28 5y ago
The answer is strong contracts based development. This way, they have the responsibility to make it work behind the public API. If it is garbage, that is their problem. If it needs to be replaced, you have someplace to start with.
frazbin 5y ago
flagged. do the same if you don't need this nonsense.
- declanhaigh 5y ago
  What's the issue? I like this article. It's an honest reflection and a clear demonstration of humility. We need more of that in this industry.
djangofan 5y ago
In my opinion, when doing a code review over a weaker developer, as the stronger developer you should show your strength by making commits to help them improve their pull request, instead of passing back-and-forth a bunch of passive-aggressive comments. If you are reviewing someones work and you go back and forth more than a couple times, you should step in and pro-actively help resolve. Together as a team you draw the conclusion. NOT working as a team is a aspect of a Narcissist.
- t-writescode 5y ago
  Definitely not; but, I would recommend, in the comment, to share alternative code and sample usage that they can take advantage of and apply themselves to their own code. If you overwrite all their code, what have they learned?
mtnGoat 5y ago
sounds like the folks that quit didn't have their lives ruined by his attitude and outlook, rather they had their lives improved by being allowed to move away from such toxicity. ego is one heck of a thing. this guy sounds terribly painful to work with. i also agree with another commenter, if tearing people down gives you pleasure, you might be wise to seek counseling.
jimmyvalmer 5y ago
tldr; In 2019 guy feels guilty about flexing code reviews. Editorial: code reviews are inherently combative; if criticism doesn't come off petty, it comes off paternalistic which is worse, imo.
cowanon22 5y ago
This is why I now use tools like SonarQube (commercial static code analyzer) to drive most code quality reviews. SonarQube also gives justifications for all of it's recommendations, so it serves as a training tool as well. The only I things I manually look for in reviews are overall readability, sensible naming, high level adherence to the architecture/design, and data system/database/API queries. After 20 years of writing code and leading teams, I've found that most of the stuff developer blogs and the OOP community fret over simply doesn't matter in most cases. The high level interfaces are often more important than the low levels details, and static code analyzers do a decent job of pointing to problems. I've almost never seen bad code that didn't also have many issues in the static analyzer. It also matters what kind of code you are writing - foundation code or something that will be used by thousands/millions of developers should have a higher bar than some internal one off. However, I've seen enough very profitable companies running on total crap code to believe that code quality is the most important thing.
kaminar 5y ago
You guys obviously have no leadership skills...sad.
seiferteric 5y ago
Even better is when you get a review like this and get the pleasure of telling the reviewer why they are wrong!
ChrisMarshallNY 5y ago
There's something to be said for "Marine Drill Instructor" responses. When the DI is screaming profanities at recruits, [s]he is preemptively saving their lives. The manner in which it is done, is actually a component of the training. A significant part of military training, is to deprecate the individual, in favor of the team. The DI is not an individual. They are the sharp end of the elite organization that the recruit hopes to join. The variable is the recruit. The organization is a hardcoded constant. But most of the world is not in the military. I understand Linus' expletive-laden rants. I don't like them, and I think he could probably find other ways to enforce Quality, but he does get results. The same for Steve Jobs, and his infamous diatribes. When I worked for a Japanese company, I watched managers literally bring employees to tears, with simple, quiet comments. I worked for a British company, and got experience with the sarcasm-fueled way that British managers work. As a manager, I worked with firm kindness. I made it clear what was expected as an end result, and did my best to support my employees. It seemed to work for my team, but I also had a particular type of team. My style probably would not have worked in other contexts. I feel that I projected a gestalt, as opposed to a set of individual orders. We acted as a unit; which wasn't easy, as I also worked to preserve the individuality of the members. As a coder, I am fairly obsessive about Quality. Code quality, documentation quality, design quality, process quality, schedule quality, and end-result quality. I'm very hard on myself. I set a high bar, and usually meet it. I've learned not to project it onto others; if at all possible. I understand that my level of Quality is probably not something most companies would consider "cost-effective." Here's an example: I am writing an app with a screen that displays a big map, and has overlaid views and controls. I spent at least 45 minutes, today, ensuring that one of the controls aligns pixel-perfect with its "alter ego" in the "unfurled" HUD display. It was a pain. I had to set a measurement point in the simulator, run the animation, then review the open display, and make sure they matched. Then, I had to rotate the device, and do the same. Many, many developers would say "5 or six pixels off; good enough. It's all under a fat finger, anyway." I am not right in the head, though. That kind of thing actually makes me physically uncomfortable. For me, that needs to be 0 pixels; even if it is under a fingertip. So I will do it for my work. If I find that I can't use the work of others, I won't work with them. I'm very, very picky about dependencies. Fancy Web sites and eye-candy aren't particularly indicative of decent work. Sadly, large user communities no longer seem to be a mark of quality, either. I can't take anyone at their word. I have to test for myself. I have been fortunate to find teams of like-minded folks, but they are rare. But I won't tear people apart, and I won't lob public insults. I feel that is not helpful. If the only way that I can feel good about myself, is to tear down others, I need to do some work on myself; maybe with a psychotherapist.
athenot 5y ago
From 2019. Previous discussion: https://news.ycombinator.com/item?id=19190472 https://news.ycombinator.com/item?id=19190472 - 155 comments
908B64B197 5y ago
> If a guy brings me his code, and it has mistakes, it brings insane pleasure from how smart I feel. [...] And if you tell me that you haven't had this feeling ever, then you're lying. Tell me about higher goals, training rookies and all that - I know you're simply too full of themselves. And if you try to tell me that you learned to defeat that feeling (however it manifests in you), then I must be a pink unicorn. This has to either be a cultural thing or satire.
draklor40 5y ago
You can be a good Developer AND a nice human being. The choice is not either or. That is the goal one should strive for, not to decide between shooting down a PR or checking-in bad code into the repo.
AtlasBarfed 5y ago
Tis far easier to criticize than create. Devs often have insufficient time for the standard of perfection code reviews assume: perfect code, formatting, testing, experience, foresight, etc. Code reviews also have a strong element of bullying, hazing, and people reenacting semi-trauma (that is the cycle of abuse so common in life: trauma makes you lonely, enacting that trauma on others in kind makes you feel less lonely) I used to joke that in lots of enterprises, it isn't faster, cheaper better pick two, it's actually faster, cheaper, better, follow process, document. Pick about 1 1/2.
mdip 5y ago
Sometimes when I read these things, I wonder if I've just gotten really lucky and selected positions at places with well-adjusted, mature, talented software developers? This is in no way representative of any of my experiences both giving and receiving code reviews. I've experienced them in a variety of "ways" -- senior's reviewing mid/juniors "by policy", dedicated teams reviewing certain types of code for certain purposes[0], "randomly distributed" reviews among people of similar skill levels, two-person I review you, you review me always and on and on. I have been doing this work for far longer than I care to admit. My rejections/non-approvals (both received and provided) land in one of the following categories: (a) Did you notice that? (Oops, D'Oh!), (b) Did you know about that? (Educational), (c) Did you think about that? (Edge Cases), and (d) Why? I work with these men and women day-to-day, so we don't often need to butter up negative responses, we have established that "I trust you can do your job" and my comments do not imply otherwise. The best way I've heard it said is "We do code reviews because errors in software development are common, regardless of proficiency. I don't write perfect code and having a second set of eyes on my work will only make it better/me look better. Reading a growing code-base by reviewing each other's work benefits everyone." Of the ways those land, the (b) category is the one that requires a little care (if your goal isn't just to bully/shit on someone's gap in knowledge). I find the way to take the sting out of it is to start off with "Hey, I came across an approach to solving this using (whatever); have you done anything with that? It would be an improvement ... (comment goes on)". I didn't say I came across that approach 15 years ago, that it's what I have decided is "Kindergarten in C# School". Guess what? Someone's read my code and thought the exact same thing. I know this because I've thought them about code I've written as early as 6 months prior. Of course, it's a matter of perspective. I thought that over the one thing I saw that made my toes curl up, never mind that the rest of it was perfectly fine, just like the code I'm currently reviewing. My favorite, though, is the occasional class/implementation that lands in "Why?". Usually those look like the previous -- a WTF-worthy implementation that the only comment I can send with is "Why was this particular implementation used?" -- and often it requires refactoring ... but every once in a while there's that gem. Some edge case that was cleverly discovered, covered and the developer was left with "This is, literally, the only way that this can be done (given the constraints)." It's the code that's both frighteningly ugly/really cool once you realize why it had to be that way. [0] I was one of two (but really, one of one) doing the entirety of secure code review for a large multi-national telecom for one brief, terrifying, point.
gautamcgoel 5y ago
I had a completely different response to this article then all of the other commenters. Yes, he is a smug, insufferable asshole who takes pleasure in putting his colleagues down. On the other hand, he is publicly acknowledging his poor behavior and apologizing for it. I thought this was a raw, vulnerable thing to write - he says that his poor behavior arises from a need to feel superior. We've all met people like that - but how many would publicly acknowledge and try to do better? A lot of posters are tearing into this guy for his confession. I suggest that we instead thank him for sharing and encourage him to do better in the future. His post reminds me a bit of this religious group, Filhos da Mente de Cristo, that appears in Orson Scott Card's novel Speaker for the Dead. All members of this group take names that reflect their resolve to overcome their greatest flaw, for example: "I will put others before myself" or "I will listen carefully before speaking", etc.
WalterBright 5y ago
> And if you tell me that you haven’t had this feeling ever, then you’re lying. I haven't. I love it when I see a great PR that I can just stamp "Approved" on. The last thing I want to do is close something as so bad it's unsalvageable.
chaps 5y ago
A team I was on years back had developers like this. My role there was as an infra dev where the main project was to install security daemons on a bunch of hosts. The devs who worked on the project for two years basically made an unusable piece of junk, with infrastructure written in four different languages. In my role I was expected to use what they built, but it never worked, though the opinionated devs didn't believe in storing logs, calling it an "ephemeral" system and we could never know why the damn thing didn't work. So we had to ssh into hosts and install the daemons by hand. At one point we had a major issue with a client, because our daemons weren't installed on their hosts right before a major event of theirs, and none of the tooling could install on this clients' hosts. So I threw something together over an afternoon that could do it all by frankensteining the other devs' code. It was run-once code to fix the emergency. Lots of selenium garbage. Since it was one-off, hacky code, I didn't feel it was necessary to put it into git, so I didn't.. until others complained to management and forced me to. From there, I had about four devs dog pile my code, commenting on everything wrong with it - maybe 20-30 comments. I explained that it was one-off code meant to solve a specific problem and of course it was going to be messy, but I got responses like "if your code was made public, you'd want your code to be clean so you can be proud of it!". It was infuriating, considering their system worked maybe 5% of the time. I'd like to say it was in good-faith, but I really doubt it. At one point while I was there, one of the devs rejected a PR because I had bash in an Ansible role, saying "you don't know if the remote host will have bash installed." That whole ordeal was enough to leave the company.
nsonha 5y ago
Is ths really the norm though? If this really happened to you, by all mean voice your story, but it's not fair to make up story just to get a point across, as they could unrealistically pain the software developer community a bunch of a-holes. In reality, most people I worked with are sympathetic as code review is one of the first skills we learn working professionally.
bbertelsen 5y ago
If you're giving soul-crushing feedback and not enjoying it like the OP, one problem could be that perhaps you're not providing the developer with enough detail on the task at hand. Communication isn't easy. I have been mentoring junior developers for about 10 years, and some more senior ones for the last few years. My ego is more attuned to getting things done than tearing up people's work. The following works really well for me: 1. Never insult people's work, it's counter-productive. (Unless see number 9) 2. Nitpick on convention/style, not implementation, unless the implementation is infeasible. Convention and style will quickly become rote. This forces new developers to pay more attention. 3. If it works as intended and has meaningful tests, MERGE IT. Congratulate them :rocket:! 4. Point out logical pitfalls/caveats by asking "what would happen if". Asking questions starting with "why" triggers people and puts them on the defensive. That's your EQ lesson for today. 5. Through code-review, you can identify a lot of little problems and suggest articles to read that would help the person grow. Examples: misunderstanding async/await vs. thenable in JS. Parallel programming in R/Py. These are distinct, language specific (R has sessions/multicore, Py has the GIL) pieces of knowledge that people can learn easily with a tiny bit of guidance. 5. If you're a better programmer - write the first 1000 lines so that they can fill in the next 10,000 (credit: don't remember, someone tell me?) 6. Even junior developers can get a surprisingly challenging task done if you give them hints (point form) about how you might approach it. 7. Do code reviews in-person (1-1) or if you are tactful with your feedback and have encouraged an environment where making mistakes is acceptable - in a group, that way everyone learns! This is the management version of the DRY principle. 8. One of the biggest problems I see with new developers is "how do I break this problem into smaller pieces". Discuss potential approaches BEFORE the PR. Not after. 9. Fire people that are consistently lazy. When you manage people for a long time it easy to see the difference between someone that needs training and someone that is lazy. You can help a someone that wants to learn, but you can't motivate someone to be interested in something that they aren't already. 10. Cultural differences can make people less likely to speak up when they are stuck - check in with your team!
aazaa 5y ago
> And finally, I became the exact thing I hated: a toxic asshat throwing his skills around like fists. I don’t do code review for the business, I just like showing the rookies their place. My skills have finally started to pay off. Or he's just lazy. It's way harder to build up the people you work with than tear them down. The key ingredients tp the former are patience and empathy. And those qualities are lacking in many managers. Also, aggressive, opinionated people are often perceived as more intelligent than their more reserved peers. You'd think that the snowmen can't get snowed, but the type of person the author describes seems to be just as vulnerable to the misdirection as others.
gn170xdw07d5 5y ago
tried to use bitrix like 6 years ago or so and it was hell, they said absent documentation should be looked for on their forum, and yes it was there, but so much hate even from topic starters, got out 1 month later and it was never too early. im russian myself, i will never work with russian dev products ever.
mixmastamyk 5y ago
Be a coach, not an abusive parent. There are also good comments here about prioritizing the categories of issues into whether important or not.
bigpeopleareold 5y ago
I had this once with a co-worker, who was also a technical lead. I don't mind critical code reviews, I do want them. I can spare my own feelings if I didn't do something right. But, if that person comes off as someone who cannot relate to anyone, the code review goes into a different direction. Many times, I felt like I had to write code to get through the co-worker's code review, not for the requirement. The requirements to get pass that code review felt inconsistent and vague, so it made work very stressful. Having a conversation with him was a chore as well, as he came off to be very critical even when prepared with questions. When he left the company, I was not the only one who felt a weight lifted off all of us. No one wanted to put up with that. Code reviews and other analyses should be topics of open conversation, if anything. They should work both ways and help each other about not only needs but skill development. When it is used as a tool to boost someone's ego instead, it becomes counter-productive and many years later, I sometimes have that in the back of the head, even with those who I know can be attentive towards criticism in code reviews but are otherwise good people.
ethanfinni 5y ago
People like this are so toxic that companies must eject them. They have no real added value, they merely harass those that should be helped to grow and learn as they evolve in their positions. Software engineering teams need collaborating peers and mentors not bullies.

166 points by aracena 5y ago

Congrats to the author for realizing that confidence doesn't equate skill. This isn't being a pink unicorn, it's simply humility. The real danger of confusing the two is that real skill dulls if you let overconfidence take over, since the very thought of being "better than others" prevents you from accepting that maybe there are things that you don't know or haven't considered. For example, is "destroying" a coworker's commit in a code review really really helping? Did the person perhaps forget to consider that hiring to refill the position of a fired developer actually costs physical money (as opposed to time spent on coaching?) or that perhaps a lot of time is being spent on nitpicks without bottom-line value or that tip-toeing around egomaniacs can stunt autonomy and therefore creativity? This is not merely a matter of whether you're lulling yourself into some false sense of superiority/duty/whatever; thinking in terms of bigger and bigger pictures is an actual skill that one needs to develop when going up the career ladder.

blahyawnblah 5y ago
If you have to "destroy" every PR from a developer, you're wasting more time (money) than it would take to replace them. Why should be bad PRs ever be allowed for any reason?
- fernandotakai 5y ago
  bad PRs shouldn't be allowed, but you don't have to be a smug a-hole while reviewing PRs. hell, that's a GREAT opportunity to get this "bad developer" under your umbrella and start mentoring him to be a better developer.
  - fayten 5y ago
    Yup! I have been very fortunate to work with extremely kind and empathetic senior engineers in the past. They have done wonderful jobs at using the code review process to teach instead of tear anyone down. I am always shocked at how many programmers become so aggressive about development. It might be silly, but I think the xkcd comic Ten Thousand is a great mantra: https://xkcd.com/1053/ https://xkcd.com/1053/ Teaching people cool new ways to code should be fun, not an exercise in showing everyone the size of your ego.
- nine_zeros 5y ago
  > Why should be bad PRs ever be allowed for any reason? Define bad PR. Are you sure that your definition of "bad" is accurate?
- n8cpdx 5y ago
  There are many ways to solve any problem. If I reviewed every PR with the standard "is this exactly how I would have done it", I too would be destroying every PR I review. And I'm not even that good at coding - I'm the type of generalist who cares about things like the business beyond the code base. What really matters is whether the code is solid and whether it will actually cause problems. Just because a PR isn't character-for-character how I'd write it doesn't mean it's bad. Based on the author's experience with the fired employee's replacement being just as bad, my guess is he was over defining goodness and raising the bar too high. Bad PR to me means: bad runtime complexity, bad formatting, bad security, bad memory use, bad understandability. If it meets all those bars, which for a crud app is usually easy, then it should probably pass. I'll sometimes leave a "I would have done _x_ differently" comment on matters of taste _if I think it will help the other developer_.
  - marktangotango 5y ago
    > If I reviewed every PR with the standard "is this exactly how I would have done it", This, too many people think their way is the only way. It only takes one cancelled project or one company going bankrupt (and 100,000s loc going "poof") to understand this work we do, really doesn't matter all that much in the end.
    - JamesBarney 5y ago
      I've spent over 6 years working on 5+ legacy projects that had millions of lines of code and maintenance teams 30 times smaller than the army of contractors who developed them. And I've learned all the small nit-pick maintainability issues made no difference. Whether or not there was a style guidelines, and whether or not it was enforced made little difference. Things that really made a big difference were giant bone-headed architectural decisions like "just save persist this data as serialized text", "lets write all the business logic in stored procedures", "building the software on a terrible platform". And having some type of identifiable domain model helped a little too.
    - BoxOfRain 5y ago
      It is a really humbling thought that every single one of our entire life's work as developers is one unfortunate astronomical event away from vanishing as though it never existed. Kind of puts it all into perspective really.
- rkk3 5y ago
  Of course there are thresholds for competency. But if you find yourself having to destroy every PR from a developer you're probably toxic & insufferable.
- mjlawson 5y ago
  When you're in these situations, you are faced with a challenge which isn't solved by code reviews no matter how illustrative, nice, or demeaning they are. It's first solved by identifying what the cause is of their poor performance. Are they in over their head? Are they having personal problems which are distracting them? Are they experiencing burnout? I've heard people say that they enjoy code reviews because it gives them latitude to be ruthless. After all, it's for the benefit of the business to not allow suboptimal code through, and it's just code. But frankly, I think that's a lame excuse and the easy way out. There are people behind code, and while the code might suck, the people don't. Obviously, that doesn't mean you should let it through, but it does mean you should be at least professional, and preferably compassionate in how you review it.
- JamesBarney 5y ago
  A bad PR can mean soo many different things. Ranging from "I wouldn't have implemented it that way" to "this opens up a huge security hole". Security issues shouldn't be allowed in. But a lot of "maintainability" issues have negligible if any gains, and real costs like it takes time to fix could introduce new bugs creates bad feelings between coworkers
robohoe 5y ago
Humility goes a long way in life. Anecdotally, it's definitely something a lot of software developers and systems engineers should learn how to practice.

adamnemecek 5y ago

God he sound insufferable.

watt 5y ago
The russian version is somewhat self-deprecating in tone, the translation is not doing good job conveying it.
- DoreenMichele 5y ago
  Thank you for saying that. I read the piece because of your comment and hopefully understood it a bit better because of it.

fwip 5y ago

> And if you tell me that you haven’t had this feeling ever, then you’re lying. Tell me about higher goals, training rookies and all that — I know you’re simply too full of themselves. And if you try to tell me that you learned to defeat that feeling (however it manifests in you), then I must be a pink unicorn. I think that this is called projection.

fayten 5y ago
That line also annoyed me. I think this person has a lot more self reflection to do about their insecurities. I am glad he has begun to recognize there are some issues and I hope he is able to continue to improve himself.

papaf 5y ago

I have been on several teams that found the following article "Unlearning toxic behaviours in a code review culture" very helpful: https://medium.com/@sandya.sankarram/unlearning-toxic-behaviors-in-a-code-review-culture-b7c295452a3c https://medium.com/@sandya.sankarram/unlearning-toxic-behavi...

andreshb 5y ago
This should be basic training anywhere people learn how to code.
sbacic 5y ago
I'd like to point out that every one of the "correct" examples is longer, sometimes much longer, that the "wrong" example preceding it. I've found that I usually start with the "correct" way and end up regressing to the "wrong" way as more and more work piles up, stress builds and deadlines approach. I guess the "correct" way only works if the organization actually prioritizes it and gives reviewers the time to do it the "correct" way no matter what stage the project is in.

backoncemore 5y ago

Nothing makes working at a place more unbearable than having to deal with someone that gives an excessive amount of criticism on working code, at least that's how I feel. I've been in that situation before and it made me never want to submit pull requests. It made me even madder when other developers, whose code was no better than mine but had been at the company longer, received basically no critiques. Some might say, oh it's hazing, well fuck you because I'm quitting. I'm not putting up with any amount of hazing at a job. If I have to spend 8 hours everyday somewhere I'm for sure not going to spend it with some asshole.

billycorben2 5y ago
Strange, I’m on a team rn with people who just approve everything. I’m craving some constructive feedback.
- smoldesu 5y ago
  It's much easier to ask someone to be critical than it is to ask someone to go easy on you.
- onlyrealcuzzo 5y ago
  Not to be rude - but... have you asked for it?
  - billycorben2 5y ago
    I don’t think this is rude. I haven’t asked them. I should ask for more feedback. I haven’t asked because: I was a latecomer to the team and they had an established process Cultural differences between myself and them Honestly, I’m not sure all of them are experienced enough to give a useful critique
    - vchuravy 5y ago
      Code reviews can also have the opposite effect, I sometimes ask mentees of mine to review PRs of mine. Not necessarily to get their feedback, but to get their questions and to help transfer knowledge. I used to do that when I started working on open-source as well, doing code-reviews to learn the code base (often I would not submit the review to not create noise)

Justsignedup 5y ago

i have been on an ever onward journey to try to find the balance: - is a developer making the same set of mistakes? send them information on a better strategy to try overall - how bad is this mistake? Is it just something that I know how to do better, or legitimately a broken way of thinking? - what is worse, a slightly inefficient or unpretty algorithm, OR someone researching the problem / solution Point is, 90% of my comments were meaningless. I really comment when things look wrong -- Did they misunderstand the intent of the ticket, did they verify everything with product because this looks weird, did they check the edge cases, did they forget about this other behavior, etc. All that I just mentioned is the real meat-and-potatoes. Code-fu is important, but less important than the other stuff. And if you are a calm, trusted, and mentoring team member, those same people will come to you to up their code-fu voluntarily.

deleted 5y ago

[deleted]

deleted 5y ago
[deleted]

throwthere 5y ago

I guess if you try to call "feelBetter" right after "throwingShit" you'll never get to feeling better. Maybe that's the point being made?

notamy 5y ago

(2019)

deleted 5y ago

[deleted]

tornato7 5y ago

I had an incident many years ago where I found glaring design flaws in a much more senior developer's code, and, overcome with some feeling of superiority, left a million nitpicky comments and a snide summary, then posted on our team chat "left a review". Finally, I was getting back at all the senior developers who had torn apart my code. I later realized my comments were mean and I apologized. While you could still potentially call it constructive criticism, there is a difference between mean criticism and polite criticism. I do think thorough feedback is important, that's how I've learned the most after all. But typically now if I I'm going to write more than 5 comments, I meet with the other developer in person instead. That way the code problems are solved faster and it avoids any sort of public humiliation.

majormajor 5y ago

> If a guy brings me his code, and it has mistakes, it brings insane pleasure from how smart I feel [...] And if you tell me that you haven’t had this feeling ever, then you’re lying. Tell me about higher goals, training rookies and all that — I know you’re simply too full of themselves. And if you try to tell me that you learned to defeat that feeling (however it manifests in you), then I must be a pink unicorn. I really can't relate at all. I feel the opposite. I often feel angry (I realize this is not a GOOD response either!) and always feel sad. I don't want to have to do more work. I don't want a reminder that my previous feedback didn't click with them. I'm constantly looking for ways to do less tedious work, through automation and such, and code review for bad code is the worst sort of tedious work. I want to learn things, but code reviewing bad code doesn't teach me anything - I want to work with people who can teach me things when they review my code just as often as the reverse. My takeaway from that has been to try to intercept people earlier in the planning and designing and start-of-coding process. If a bunch of bad code gets to code review, someone failed at explaining what this thing needed to do and how it should be done beforehand. The article claims this is always just itself a confrontational form of argument, but... it doesn't have to be. Stop being an asshole, and find non-assholes to work with.

watwut 5y ago
My reaction to this was that this guy should not be making code reviews ever. That emotional reaction in combination with conviction everyone has the same one disqualify him.
- MisterBastahrd 5y ago
  I wouldn't even want that guy on my team as a junior, because he's a headache waiting for a place to happen.
  - smoldesu 5y ago
    I'd expect a neatly-worded blogpost on the company Medium: "I Destroyed Our 55m User Database, But Here's What I Learned:"
- 5y ago

elwell 5y ago

What would happen if all PRs were submitted/reviewed anonymously, and the feedback was just as impersonal as a stack trace?

duxup 5y ago
Anonymity on the internet doesn't seem to provide much in the way of civility.
jimmyvalmer 5y ago
As someone for whom face accountability is the only thing preventing me from eviscerating my peers, this sounds like a dream come true.

rootusrootus 5y ago

If you want some empathy with the guy whose code you are reviewing, go pull up some of your own code from a few months or a year ago. Do a mock code review on it. Maybe everyone else only writes good code, but this works for me every time. Even after over 30 years coding, I still have no problem seeing how imperfect the code is that I wrote as recently as last year. And don't ever make it personal, anyway. Adopt a neutral tone, phrase comments abstractly as how they relate to team goals, how the code might be made clearer, etc. Don't accuse, don't belittle, don't try to look smarter than you are. And try to recognize the difference between the stuff that actually matters, the things which will actually be a headache down the road, and the pointless details that are just not how you'd have chosen to code something.

clarle 5y ago

Having an auto-formatter like Prettier or `go fmt` has been a huge boon and productivity gain for the teams that I've worked with. No longer having code nitpicks on formatting has saved so much time going through reviews, and it really increases the signal to noise ratio on pull request comments too. Would definitely recommend those for any teams that have a similar teammate as the article author on their team.

mrtranscendence 5y ago
Yeah, we mostly do Python and I've really been trying to push the use of black. I don't actually like some of black's formatting choices (OK, it's grown on me) but it's better to have a standard and stick to it.

ratww 5y ago

Ouch. I’ve had a few cases of developers on the verge of being fired because of bad code causing friction with the team. Code reviews never help in those cases. There is always an underlying cause: the developer is trying to rush to prove something, or maybe over-engineering for the same reason. Maybe he has some issues at home, or isn’t sleeping well. Maybe it’s just lack of experience. Maybe it is a personality issue and the developer is trying to save face by sticking to his guns. Either way, I find that it is much better to have private discussions with the developer to increase the code quality via mentoring, or pairing. It helps removing the defensiveness, doesn’t make them feel publicly humiliated and doesn’t spend the time of the team with the review-fix cycle.

mrtranscendence 5y ago

I'm not a super-coder; give me a JS project of moderate complexity and I'd have no idea what's going on or what I'm supposed to do with it. But as a data scientist I do have the reputation of being good at coding even among some of the engineers at my company. And some of the code produced by both data scientists and engineers ... it's, hm, not great? Just between you and me? So I'm glad I've never had the inclination to tear anyone to shreds. Indeed, part of my job used to be to help people with their coding problems, and I loved doing that. But sometimes when I'm reviewing code ... it's very hard to bite my lip and not call out every little issue, even though it would just be too much and most of the "problems" aren't actually very important. I try to impress upon people that developing good code is useful when I lead training sessions, but it's hard, especially for people who'd rather be doing data science than coding.

2OEH8eoCRo0 5y ago

Funny. I'd say that if somebody can't work with or maintain code written by others then that's the sign of a weak programmer. If all code you work with has to be written exactly as if you wrote it yourself then that's a weakness.

neo2006 5y ago

90% of a code review is not about finding THE BUG in the PR but asking questions and making the author find his own bugs or learning something from him. If you are performing a code review hunting for bugs you already lost your humility and think you are better then the PR author.

AnimalMuppet 5y ago
Have you ever written some text, and re-read it, and it sounded just right, and had someone else read it, and they said "what does this mean"? Sometimes the words you write are sufficient to trigger in your own head the thoughts you've already had, but not enough to make someone else think those same thoughts. Code can be like that, too. You write code, and it's enough to convince you that it does what you think it does. But someone else reading it can sometimes see the holes that you don't see. Assuming that person X writes bug-free code is bad, whether X is "you", "me", or "that co-worker". I don't have to be better than the PR author. I just have to be decently good, and be a different person. That's enough. And of course I'm looking for bugs. That's the absolute first thing to look for! Now, true, if I can help the author be able to see their own bugs (asking questions, maybe), then that's probably better than me just lecturing them. Teach them how to think so that they can see it themselves next time.
- neo2006 5y ago
  I'm not saying that you will never find bugs in someone PRs I'm just saying it shouldn't be your first intention, your first intention should be to understand it and be convinced by it. Finding bugs will be a side effect of that. If you reading the code to execute it in your head and find all the weird combination that could trigger a core dump, you should probably check if the author wrote tests for those use cases instead and comment about missing tests. Genuine interest in others work can go a long way compared to imagining how would you solve it like the article author said.

quxpar 5y ago

Maybe the tone doesn't translate but this guy sounds insanely egotistical. I feel truly bad for his family and anyone he works with.

Doctor_Fegg 5y ago
It strongly reminded me of this graph of different cultural norms for phrasing criticism: https://mobile.twitter.com/sebjilke/status/1036406794359173120 https://mobile.twitter.com/sebjilke/status/10364067943591731... (I believe TFA is Russian, so read the Eastern European graph)
- dwaltrip 5y ago
  Very interesting to see those cultural differences. Hilariously, the first graph is titled "Objective distribution" (of criticism phrasing). That's not how language works...

lhnz 5y ago

In my estimation, most places have the opposite problem: everybody approves the code without looking through it, and the only comments that are left are stylistic problems. Good code reviews spot real bugs and provide solutions and explanations that help the owner to improve their branch. It's completely constructive and feels like going out of your way to way to help them and paying attention to what is meaningful -- the vibe should be a bit like "doorman at a posh hotel providing excellent service", so not disrespectful at all. Also, if you're nitpicking, it's a sign of lack of focus...

deleted 5y ago

[deleted]

jp57 5y ago

I had an experience in my first year of grad school that has informed my code reviews and my general attitude toward reviewing others' work ever since. I was taking a graduate-level introduction to mathematical logic. In the class we learned about a bunch of different propositional axiomatic proof systems, and then we proved properties like soundness and completeness for them. The class was organized around class participation: everyone was required to do at least one proof at the board in front of the class and every session consisted almost entirely of students doing proofs at the board. I'd been out of school, working, for eight years before grad school and I found this all very intense and unfamiliar. I did my required work at the board, choosing an easy problem, but otherwise mostly sat in the back and watched. There was, however, a cohort of very intense aspiring logicians who sat in the front row and took deep interest in every proof. One day, a student went up with a particularly challenging problem. From the start it was clear that he was taking an unorthodox approach to the proof. The front row went nuts, interjecting and kibitzing on practically every line, telling the guy different things that he should do instead of whatever it was that he was doing. The guy was a good sport and tried to justify everything and answer all the criticism, but it seemed like he might not get through the proof before the class ended. The prof who had been watching quietly from the side of the class, let it go on for a while, then stepped out and quieted everyone. "I need you to remember," he said to the front row, "that we are the audience. It is not our job to prove this theorem; it is this gentleman's job. Our job as the audience is to listen to him, and at the end, decide if we are convinced. He may do things differently that you would have done, but that doesn't matter, as long as in the end we are convinced." That was twenty-three years ago, but it stuck with me. In programming, there are often many different ways that a problem could be solved, but there is almost never One True Way™, even though there are often many voices that would like to convince you that their way is the right one. My job as a reviewer of code, or really anything, is not to try to get the other person to do what I would have done, but to decide if I'm convinced that the way that they've chosen is good enough, and, if not, to ask for changes that would convince me. Sure, sometimes a PR is so bad that it needs to be completely rewritten, but in my experience that's rare, and in that case the feedback should probably be given in person and in private or in a friendly, sympathetic group.

jimmyvalmer 5y ago

plutonorm 5y ago

This except there is no right way and there are 'n' equally good ways of solving any real world problem. It's only worth pointing out flaws if something is genuinely bad and I mean bad. Otherwise you are using a gut opinion to trash someone else's emotional health. Yay! you found an edge case that breaks a piece of code that will never get hit while the other million and 1 edge cases that haven't been found are still out there. Yet you drag it out from the dark and show it off for all to see like some damn trophy. Yay! look I am great I found one more thing wrong than the other guy. No matter that we are all swimming in a sea of wrong and every one is completely clueless. Only slightly less clueless than those who don't even realise they don't have a clue. It follows from a simple argument - every programmer and every company has a different view of 'good'. I've seen it, I've worked in many. Just as every religion cannot be right, every weird view on correct programming style cannot be correct.

vdqtp3 5y ago
> there are 'n' equally good ways of solving any real world problem. This is objectively, mathematically, incorrect. There are optimal solutions, and for some/many problems, there is one optimal solution.
- plutonorm 5y ago
  well done for illustrating my point

duxup 5y ago

I really haven't coded for long compared to some. I always look at code reviews or just reading / talking about each other's code as a way for us to just get on the same page about how we do things. Make everyone's life easier. "Hey man when we're looping through that thing here is how we usually do it." "Ok cool, I'll do that." Or maybe we have a discussion on why I did it differently this time and how we might address that. Much of the time I don't "care" or feel strongly what the change or answer is. If we're all doing the same thing / spot a bug sooner, it's just easier for everyone. But it seems code reviews to some are this sort of battle / proving ground / have a lot of elements of status. I don't get that...

yupper32 5y ago

There are a few types of comments on code review that I see: 1. Critical mistakes 2. Style guide mistakes 3. Non-optimal design (in your opinion) #1 Always receives a comment, obviously. #2 Always receives a comment, but we have good automation here so it's not too much of a worry. #3 Is where it gets interesting. A lot of the time, commenting about this category is prematurely optimizing and it's a waste of time for both the reviewer and the owner of the change. At most I try to make one overarching comment about the design. You really have to ask yourself "Does it work? Will it cause issues down the line?" If the answers are Yes and No respectively, you should likely let it go. The feeling of ownership over the design, especially for a junior engineer, is incredibly important for moral and the ability to get better over time. IMO it's also important to let them make small mistakes now and again so they can learn from it.

deleted 5y ago
[deleted]
RandallBrown 5y ago
With number 3 I usually leave a comment but it's something along the lines of "Why did you decide to do x instead of y. I think y would be better because..." I don't necessarily want or expect them to change their code but I want to make sure they're considering multiple options and intentionally choosing that path instead of just copy and pasting something from stack overflow (as an example.)

leepowers 5y ago

> I was, again, technically stronger. A thousand-string pull request was littered with 200 comments, not leaving the person even a faint hope in his competency. Great. That's way too much feedback. It makes it very hard determine which problems are critical, and which come down to aesthetics or personal preference. On the flip side, if I was churning out buggy code at that rate, I'd want to know about those bugs in detail. There's also an opportunity cost of evaluating code so tightly. Not only does it take up more of the reviewer's time, it also eats up developer time that could be spent on other features. > The reason for these angry code reviews is obvious. As part of the team, I bear full responsibility for the project’s code base. I have to work with it later, after all. It’s a source of many issues for the business. The code doesn’t scale, can’t be tested properly, is littered with bugs. Support gets more and more expensive. It can’t be open-sourced or used to lure new developers. Premature optimization? The mistake here is trying to optimize the legacy of your code base, possibly thinking too far ahead.

marstall 5y ago

PRs definitely bring out the worst in some people ... would head for the exit with a vibe anywhere near like this on the team. sad to me that young devs have to go through this.

gchamonlive 5y ago

> The universe blessed me with this trait so I can awaken the anger in other young and inexperienced coders An important skill to develop is the ability to "read the audience". Almost prescient in nature, predicting how people will react to you is of utmost importance so that you can tailor your responses accordingly. This is important when mentoring people because, even though tough love might work on some, it may very well destroy self esteem for others and make them shift career, ending something before it even starts. If you are in a position of mentoring, it is your responsibility to adapt. It is painful, frustrating and a source of contempt when you try and try but you just can't seem to find the sweet spot to inspire newcomers, but as long as you persevere there is hope in teaching others. I had a recent experience in this. I am in no way a long experienced infrastructure engineer, but I built my share of cloud applications to have an idea what can go wrong. I recently tried mentoring someone new and it was just plain frustrating. I couldn't explain too much without overwhelming him with details, and I couldn't just direct to sources without him feeling abandoned. That sweet spot of attention I couldn't reach before he requested to "learn different things" and effectively abandon infrastructure haunts me sometimes.

myk9001 5y ago

I speak Russian and used to read habr.com a lot, which is a Russian-speaking platform where the community members can post on topics related to software engineering, computers, and tech in general. (As you can notice, habr.com have also been trying to gain an English-speaking readers for some time now.) So I read or saw multiple articles in Russian from this specific author. Actually, the one linked in this post is a translation. To put it bluntly, while they clearly have a background in software engineering, they are a troll. All their articles seem to have one thing in common: they look to be as controversial as possible on purpose, I don't believe them to be sincere. And I'll give it to the author, the tactic worked great: their articles usually generated a huge discussion in comments and -- I would guess -- a lot of traffic for the site. Though people seem to got immune to it over time. And there's probably nothing wrong with writing in a provocative style, but you might want to keep it in mind when reading the post and not take it too literally.

kbenson 5y ago
> saw multiple articles in Russian from this specific author. Actually, the one linked in this post is a translation. To put it bluntly, while they clearly have a background in software engineering, they are a troll. All their articles seem to have one thing in common: they look to be as controversial as possible on purpose, I don't believe them to be sincere. Is "don't be an asshole when it's actually not helping the other person like you told yourself if you look closely" actually controversial enough on that site to be considered a troll? Honestly, the article comes across to me as someone publicly acknowledging their shortcomings in an attempt to combat them. If that's trolling Russian developers that frequent that site... maybe they deserve to be trolled in that way?
- golergka 5y ago
  > Is "don't be an asshole when it's actually not helping the other person like you told yourself if you look closely" actually controversial enough on that site to be considered a troll? In Russia, yes. Like the OP comment author, I'm familiar with both IT cultures, and there's a dramatic differences in the amount of politeness. I have actually seen people get into a physical fight because of work-related dispute, more than once.

nanis 5y ago

Everyone is capable of writing crap code ... Code that might give the impression of working but is unmaintainable. PRs are conversations. Every team member should be able to request changes to start a conversation. A conversation about the code should focus on what tradeoffs are necessary to get stuff done now along with a consideration of how comfortable every team members feels they can maintain the code. Teams where PR approvals are considered endorsements, where PRs are rubber stamped, where developers throw fits when others dare to ask questions etc are detrimental to both getting things done and maintaining a sane working environment. Don't confuse the code with your worth. The greatest asset you will ever have is the ability to be openly critical about code you have written while understanding the right level of imperfection for a given circumstance. This only comes with experience. The more you nurture your ability to view your own code critically, the faster it comes. Since we all have blind spots, take others' questions seriously.

glitchc 5y ago

Buddy, don’t blame the yhe industry for your argumentative nature. Blame your dad. And unless the company pays you for every line of code you comment, you’re better off spending more time with your kid.

jhawk28 5y ago

The answer is strong contracts based development. This way, they have the responsibility to make it work behind the public API. If it is garbage, that is their problem. If it needs to be replaced, you have someplace to start with.

frazbin 5y ago

flagged. do the same if you don't need this nonsense.

declanhaigh 5y ago
What's the issue? I like this article. It's an honest reflection and a clear demonstration of humility. We need more of that in this industry.

djangofan 5y ago

In my opinion, when doing a code review over a weaker developer, as the stronger developer you should show your strength by making commits to help them improve their pull request, instead of passing back-and-forth a bunch of passive-aggressive comments. If you are reviewing someones work and you go back and forth more than a couple times, you should step in and pro-actively help resolve. Together as a team you draw the conclusion. NOT working as a team is a aspect of a Narcissist.

t-writescode 5y ago
Definitely not; but, I would recommend, in the comment, to share alternative code and sample usage that they can take advantage of and apply themselves to their own code. If you overwrite all their code, what have they learned?

mtnGoat 5y ago

sounds like the folks that quit didn't have their lives ruined by his attitude and outlook, rather they had their lives improved by being allowed to move away from such toxicity. ego is one heck of a thing. this guy sounds terribly painful to work with. i also agree with another commenter, if tearing people down gives you pleasure, you might be wise to seek counseling.

jimmyvalmer 5y ago

tldr; In 2019 guy feels guilty about flexing code reviews. Editorial: code reviews are inherently combative; if criticism doesn't come off petty, it comes off paternalistic which is worse, imo.

cowanon22 5y ago

This is why I now use tools like SonarQube (commercial static code analyzer) to drive most code quality reviews. SonarQube also gives justifications for all of it's recommendations, so it serves as a training tool as well. The only I things I manually look for in reviews are overall readability, sensible naming, high level adherence to the architecture/design, and data system/database/API queries. After 20 years of writing code and leading teams, I've found that most of the stuff developer blogs and the OOP community fret over simply doesn't matter in most cases. The high level interfaces are often more important than the low levels details, and static code analyzers do a decent job of pointing to problems. I've almost never seen bad code that didn't also have many issues in the static analyzer. It also matters what kind of code you are writing - foundation code or something that will be used by thousands/millions of developers should have a higher bar than some internal one off. However, I've seen enough very profitable companies running on total crap code to believe that code quality is the most important thing.

kaminar 5y ago

You guys obviously have no leadership skills...sad.

seiferteric 5y ago

Even better is when you get a review like this and get the pleasure of telling the reviewer why they are wrong!

ChrisMarshallNY 5y ago

There's something to be said for "Marine Drill Instructor" responses. When the DI is screaming profanities at recruits, [s]he is preemptively saving their lives. The manner in which it is done, is actually a component of the training. A significant part of military training, is to deprecate the individual, in favor of the team. The DI is not an individual. They are the sharp end of the elite organization that the recruit hopes to join. The variable is the recruit. The organization is a hardcoded constant. But most of the world is not in the military. I understand Linus' expletive-laden rants. I don't like them, and I think he could probably find other ways to enforce Quality, but he does get results. The same for Steve Jobs, and his infamous diatribes. When I worked for a Japanese company, I watched managers literally bring employees to tears, with simple, quiet comments. I worked for a British company, and got experience with the sarcasm-fueled way that British managers work. As a manager, I worked with firm kindness. I made it clear what was expected as an end result, and did my best to support my employees. It seemed to work for my team, but I also had a particular type of team. My style probably would not have worked in other contexts. I feel that I projected a gestalt, as opposed to a set of individual orders. We acted as a unit; which wasn't easy, as I also worked to preserve the individuality of the members. As a coder, I am fairly obsessive about Quality. Code quality, documentation quality, design quality, process quality, schedule quality, and end-result quality. I'm very hard on myself. I set a high bar, and usually meet it. I've learned not to project it onto others; if at all possible. I understand that my level of Quality is probably not something most companies would consider "cost-effective." Here's an example: I am writing an app with a screen that displays a big map, and has overlaid views and controls. I spent at least 45 minutes, today, ensuring that one of the controls aligns pixel-perfect with its "alter ego" in the "unfurled" HUD display. It was a pain. I had to set a measurement point in the simulator, run the animation, then review the open display, and make sure they matched. Then, I had to rotate the device, and do the same. Many, many developers would say "5 or six pixels off; good enough. It's all under a fat finger, anyway." I am not right in the head, though. That kind of thing actually makes me physically uncomfortable. For me, that needs to be 0 pixels; even if it is under a fingertip. So I will do it for my work. If I find that I can't use the work of others, I won't work with them. I'm very, very picky about dependencies. Fancy Web sites and eye-candy aren't particularly indicative of decent work. Sadly, large user communities no longer seem to be a mark of quality, either. I can't take anyone at their word. I have to test for myself. I have been fortunate to find teams of like-minded folks, but they are rare. But I won't tear people apart, and I won't lob public insults. I feel that is not helpful. If the only way that I can feel good about myself, is to tear down others, I need to do some work on myself; maybe with a psychotherapist.

athenot 5y ago

From 2019. Previous discussion: https://news.ycombinator.com/item?id=19190472 https://news.ycombinator.com/item?id=19190472 - 155 comments

908B64B197 5y ago

> If a guy brings me his code, and it has mistakes, it brings insane pleasure from how smart I feel. [...] And if you tell me that you haven't had this feeling ever, then you're lying. Tell me about higher goals, training rookies and all that - I know you're simply too full of themselves. And if you try to tell me that you learned to defeat that feeling (however it manifests in you), then I must be a pink unicorn. This has to either be a cultural thing or satire.

draklor40 5y ago

You can be a good Developer AND a nice human being. The choice is not either or. That is the goal one should strive for, not to decide between shooting down a PR or checking-in bad code into the repo.

AtlasBarfed 5y ago

Tis far easier to criticize than create. Devs often have insufficient time for the standard of perfection code reviews assume: perfect code, formatting, testing, experience, foresight, etc. Code reviews also have a strong element of bullying, hazing, and people reenacting semi-trauma (that is the cycle of abuse so common in life: trauma makes you lonely, enacting that trauma on others in kind makes you feel less lonely) I used to joke that in lots of enterprises, it isn't faster, cheaper better pick two, it's actually faster, cheaper, better, follow process, document. Pick about 1 1/2.

mdip 5y ago

Sometimes when I read these things, I wonder if I've just gotten really lucky and selected positions at places with well-adjusted, mature, talented software developers? This is in no way representative of any of my experiences both giving and receiving code reviews. I've experienced them in a variety of "ways" -- senior's reviewing mid/juniors "by policy", dedicated teams reviewing certain types of code for certain purposes[0], "randomly distributed" reviews among people of similar skill levels, two-person I review you, you review me always and on and on. I have been doing this work for far longer than I care to admit. My rejections/non-approvals (both received and provided) land in one of the following categories: (a) Did you notice that? (Oops, D'Oh!), (b) Did you know about that? (Educational), (c) Did you think about that? (Edge Cases), and (d) Why? I work with these men and women day-to-day, so we don't often need to butter up negative responses, we have established that "I trust you can do your job" and my comments do not imply otherwise. The best way I've heard it said is "We do code reviews because errors in software development are common, regardless of proficiency. I don't write perfect code and having a second set of eyes on my work will only make it better/me look better. Reading a growing code-base by reviewing each other's work benefits everyone." Of the ways those land, the (b) category is the one that requires a little care (if your goal isn't just to bully/shit on someone's gap in knowledge). I find the way to take the sting out of it is to start off with "Hey, I came across an approach to solving this using (whatever); have you done anything with that? It would be an improvement ... (comment goes on)". I didn't say I came across that approach 15 years ago, that it's what I have decided is "Kindergarten in C# School". Guess what? Someone's read my code and thought the exact same thing. I know this because I've thought them about code I've written as early as 6 months prior. Of course, it's a matter of perspective. I thought that over the one thing I saw that made my toes curl up, never mind that the rest of it was perfectly fine, just like the code I'm currently reviewing. My favorite, though, is the occasional class/implementation that lands in "Why?". Usually those look like the previous -- a WTF-worthy implementation that the only comment I can send with is "Why was this particular implementation used?" -- and often it requires refactoring ... but every once in a while there's that gem. Some edge case that was cleverly discovered, covered and the developer was left with "This is, literally, the only way that this can be done (given the constraints)." It's the code that's both frighteningly ugly/really cool once you realize why it had to be that way. [0] I was one of two (but really, one of one) doing the entirety of secure code review for a large multi-national telecom for one brief, terrifying, point.

gautamcgoel 5y ago

I had a completely different response to this article then all of the other commenters. Yes, he is a smug, insufferable asshole who takes pleasure in putting his colleagues down. On the other hand, he is publicly acknowledging his poor behavior and apologizing for it. I thought this was a raw, vulnerable thing to write - he says that his poor behavior arises from a need to feel superior. We've all met people like that - but how many would publicly acknowledge and try to do better? A lot of posters are tearing into this guy for his confession. I suggest that we instead thank him for sharing and encourage him to do better in the future. His post reminds me a bit of this religious group, Filhos da Mente de Cristo, that appears in Orson Scott Card's novel Speaker for the Dead. All members of this group take names that reflect their resolve to overcome their greatest flaw, for example: "I will put others before myself" or "I will listen carefully before speaking", etc.

WalterBright 5y ago

> And if you tell me that you haven’t had this feeling ever, then you’re lying. I haven't. I love it when I see a great PR that I can just stamp "Approved" on. The last thing I want to do is close something as so bad it's unsalvageable.

chaps 5y ago

A team I was on years back had developers like this. My role there was as an infra dev where the main project was to install security daemons on a bunch of hosts. The devs who worked on the project for two years basically made an unusable piece of junk, with infrastructure written in four different languages. In my role I was expected to use what they built, but it never worked, though the opinionated devs didn't believe in storing logs, calling it an "ephemeral" system and we could never know why the damn thing didn't work. So we had to ssh into hosts and install the daemons by hand. At one point we had a major issue with a client, because our daemons weren't installed on their hosts right before a major event of theirs, and none of the tooling could install on this clients' hosts. So I threw something together over an afternoon that could do it all by frankensteining the other devs' code. It was run-once code to fix the emergency. Lots of selenium garbage. Since it was one-off, hacky code, I didn't feel it was necessary to put it into git, so I didn't.. until others complained to management and forced me to. From there, I had about four devs dog pile my code, commenting on everything wrong with it - maybe 20-30 comments. I explained that it was one-off code meant to solve a specific problem and of course it was going to be messy, but I got responses like "if your code was made public, you'd want your code to be clean so you can be proud of it!". It was infuriating, considering their system worked maybe 5% of the time. I'd like to say it was in good-faith, but I really doubt it. At one point while I was there, one of the devs rejected a PR because I had bash in an Ansible role, saying "you don't know if the remote host will have bash installed." That whole ordeal was enough to leave the company.

nsonha 5y ago

Is ths really the norm though? If this really happened to you, by all mean voice your story, but it's not fair to make up story just to get a point across, as they could unrealistically pain the software developer community a bunch of a-holes. In reality, most people I worked with are sympathetic as code review is one of the first skills we learn working professionally.

bbertelsen 5y ago

If you're giving soul-crushing feedback and not enjoying it like the OP, one problem could be that perhaps you're not providing the developer with enough detail on the task at hand. Communication isn't easy. I have been mentoring junior developers for about 10 years, and some more senior ones for the last few years. My ego is more attuned to getting things done than tearing up people's work. The following works really well for me: 1. Never insult people's work, it's counter-productive. (Unless see number 9) 2. Nitpick on convention/style, not implementation, unless the implementation is infeasible. Convention and style will quickly become rote. This forces new developers to pay more attention. 3. If it works as intended and has meaningful tests, MERGE IT. Congratulate them :rocket:! 4. Point out logical pitfalls/caveats by asking "what would happen if". Asking questions starting with "why" triggers people and puts them on the defensive. That's your EQ lesson for today. 5. Through code-review, you can identify a lot of little problems and suggest articles to read that would help the person grow. Examples: misunderstanding async/await vs. thenable in JS. Parallel programming in R/Py. These are distinct, language specific (R has sessions/multicore, Py has the GIL) pieces of knowledge that people can learn easily with a tiny bit of guidance. 5. If you're a better programmer - write the first 1000 lines so that they can fill in the next 10,000 (credit: don't remember, someone tell me?) 6. Even junior developers can get a surprisingly challenging task done if you give them hints (point form) about how you might approach it. 7. Do code reviews in-person (1-1) or if you are tactful with your feedback and have encouraged an environment where making mistakes is acceptable - in a group, that way everyone learns! This is the management version of the DRY principle. 8. One of the biggest problems I see with new developers is "how do I break this problem into smaller pieces". Discuss potential approaches BEFORE the PR. Not after. 9. Fire people that are consistently lazy. When you manage people for a long time it easy to see the difference between someone that needs training and someone that is lazy. You can help a someone that wants to learn, but you can't motivate someone to be interested in something that they aren't already. 10. Cultural differences can make people less likely to speak up when they are stuck - check in with your team!

aazaa 5y ago

> And finally, I became the exact thing I hated: a toxic asshat throwing his skills around like fists. I don’t do code review for the business, I just like showing the rookies their place. My skills have finally started to pay off. Or he's just lazy. It's way harder to build up the people you work with than tear them down. The key ingredients tp the former are patience and empathy. And those qualities are lacking in many managers. Also, aggressive, opinionated people are often perceived as more intelligent than their more reserved peers. You'd think that the snowmen can't get snowed, but the type of person the author describes seems to be just as vulnerable to the misdirection as others.

gn170xdw07d5 5y ago

tried to use bitrix like 6 years ago or so and it was hell, they said absent documentation should be looked for on their forum, and yes it was there, but so much hate even from topic starters, got out 1 month later and it was never too early. im russian myself, i will never work with russian dev products ever.

mixmastamyk 5y ago

Be a coach, not an abusive parent. There are also good comments here about prioritizing the categories of issues into whether important or not.

bigpeopleareold 5y ago

I had this once with a co-worker, who was also a technical lead. I don't mind critical code reviews, I do want them. I can spare my own feelings if I didn't do something right. But, if that person comes off as someone who cannot relate to anyone, the code review goes into a different direction. Many times, I felt like I had to write code to get through the co-worker's code review, not for the requirement. The requirements to get pass that code review felt inconsistent and vague, so it made work very stressful. Having a conversation with him was a chore as well, as he came off to be very critical even when prepared with questions. When he left the company, I was not the only one who felt a weight lifted off all of us. No one wanted to put up with that. Code reviews and other analyses should be topics of open conversation, if anything. They should work both ways and help each other about not only needs but skill development. When it is used as a tool to boost someone's ego instead, it becomes counter-productive and many years later, I sometimes have that in the back of the head, even with those who I know can be attentive towards criticism in code reviews but are otherwise good people.

ethanfinni 5y ago

People like this are so toxic that companies must eject them. They have no real added value, they merely harass those that should be helped to grow and learn as they evolve in their positions. Software engineering teams need collaborating peers and mentors not bullies.