It bothers me that the conclusion doesn't escape the framework of code reviews as places of conflict, and that the author feels guilty for his claimed competence.
> No big deal if the code’s not good, I can fix it myself it I need to.
A mentor can't be passive like that. Don't leave 200 comments like you used to, but distill them into something smaller, more cogent and digestible. More importantly, drop the idea of "good" and focus on what the implementor was thinking. I think the author understands the need for more effective communication in code reviews, but unfairly reduces the exchange to an "argument."
It's also dangerous to correlate work-life balance with incompetence. It's not a contradiction to be good at your job, current with the industry, and present for your family.
The guy appears to come to the realization that he causes more conflict than necessary.
The purpose of code reviews is to get quality code. Quality code makes the project work. But why do we work on projects to begin with? To benefit people.
This guy is realizing the conflict between the two goals of getting quality code but also serving people.
People need criticism to improve, but it needs to be constructive, otherwise the fragile human psyche inside of every person may not recover.
Reminds me of a lot of code reviews from my Google days. There was even a formal rite of passage called "readability" where you went from being rightfully humiliated to rightfully humiliating others.
My feeling is that criticism, no matter what the domain, should be relatively dispassionate (neutral in tone), and come from a desire to help and with a dose of humility. Of course, I sometimes fall far short of that ideal.
I actually feel that criticism should be empathetic rather than dispassionate... it should be worded to show that you understand the person is a competent professional who was working in good faith, but might have missed something or made a mistake.
This is why I try to word reviews as questions, not statements of facts:
"Is this supposed to be reversed? I think it might not do what you want it to"
"Having a bit of trouble following this logic.. is there a way to make it more clear to future readers?"
"I might be missing something, but I don't see how this code can be reached?"
Don't give the review in such a way that implies you know everything and the other person was wrong. It should be a dialog, and be based on the assumption that we don't yet know the right answer and are working together to figure it out.
This is incredibly honest. Maybe it's just me, but it 100% resonates with me. I find myself doing the exact same thing: as soon as I feel myself mounting up on my high-horse I have to talk myself down from being an asshat. I've gotten better at it because I'm now responsible for people's careers, and my pettiness is no reason to make someone's life suck because they are still learning -OR- because they think differently than me and have a different solution that I might not even UNDERSTAND. Being open-minded is really fucking hard for me some times.
These comments stuck out:
"Because I do code review for self-identification."
and
"It turned out that, instead of becoming a good coder, you simply have to convince others you’re a good coder. This behaviour begets a vicious cycle that produces not professionals, but toxic asshats."
One way to nip this culture is stop hiring people who only KNOW THIS culture.
If the author is reading this, there is one solution I have encountered: diversity. If you only hire white dudes in hoodies with stickers on their macbooks you're only going to get the toxic gamergate crowd, which has not gone away at all.
Believe or not there are other kinds of programmers in the world, and the most enjoyable products I've worked on were ones that had more women and more POC on their teams. Toxic asshattery can be minimized by not giving them complete control over the culture of a workplace.
I don't think diversity of skin color and gender will solve this problem in its entirety too. People of every race and every gender have the capacity to be this kind of self-centered egotist. People of every race and every gender have the capacity to be excellent developers who are focused outwards, on developing their teammates and building things well. Diversity is part of the picture, yes, but it's no instant win, either.
Why did you feel the need to state the obvious? This is the kind of toxic nonsense that comes up every time someone mentions diversity. Its the status-quo "notallmen" argument again and again and again.
Please, assume just for a minute that we aren't all idiots. Of -course- everyone has the capacity to be an idiot, but your unnecessary and highly defensive response is very telling.
As much as I am tempted to, I don't think it's right to equate diversity of appearance and diversity of experience, either. One is just a proxy for the other.
To be honest, I found myself slightly offended at your original comment, but I decided not to say anything since I agreed with the overall point you were trying to make. This response makes me feel like I have to say something though. Sometimes the person calling everything else toxic is actually a source of toxicity. Frankly, as a white guy who has been known to wear hoodies, I don't like being grouped in with toxic assholes just because of my appearance, and I think you could convey your point better by learning to phrase it in a more respectful way. Instead of trying to call out an entire group of people based on gender and skin color, just point out that it's hard for toxicity to survive when surrounded by a large variety of viewpoints and backgrounds.
As always, political extremism is a horseshoe. <Radical> assert their <object of worship> is the end-all-be-all, someone politely points out exceptions to <object of worship>, <Radical> begins screeching.
Mindlessness mixed with insecurity is a toxic brew.
Where did you get that the GP was highly defensive? I didn't read that at all. TBH I also thought it was strange that the proposed solution to certain toxic personalities was to hire different genders and races, which seems like a non sequitur to me.
Personality does correlate with gender, albeit very weakly. And differences in 'race' correlate with different social/cultural contexts to an extent that might have some effect as well - indeed, 'race' is itself a social construct that largely reflects differences in culture.
You are really on the offensive here. The concerns you've raised here remind me of a time when I let news and social media pull me into that disgusting culture war on oppression, equality, collectivism, and individuality.
I still have my stance but I've found that people that get sucked in, on both sides, will get hypersensitive about detecting their opposition, and then they project all of the ideas they dislike the most onto the person in real life who exhibits a hint of it.
You might find that you are trying to find things to be angry about. You might feel very strongly that masculinity is toxic but you don't really have any examples in your life except for the two and a half times you got cat called. The "gamers" that hate minorities and women might be on the forefront of your mind, but only because you spend too much time consuming narratives online.
Genuinely how many people would be concerned about toxic masculinity if the internet didn't exist. How many people would be concerned about the culture war at all if outrage couldn't be shared.
As much as you might like to blame it on a particular skin color or gender, your happiness and peace of mind is your responsibility. If you are frustrated, angry, or sad about gamers, the gamers aren't the problem, you are losing control of your mind.
The interesting part of this pushback isn't that there's between-individual variability with different populations, but that "white guy with stickers on his laptop", and the realm of predictive strategies that implies, is a very poor indicator of intellectual broadness or diversity.
But do you really care, seeing as to how you've already furthered a theory of someone else being either emotionally hostile or defensive?
And do you really not see the obviousness of how a certain situation plays out, of someone using white guy with stickers on laptop as a symbol for toxicity? Or do you see the obviousness, and that's why you're playing the situation out this way?
> People of every race and every gender have the capacity to be this kind of self-centered egotist
He didn't imply that women and non-whites can't be self-centered egotists, only that they're less so than white men. Despite the author of the article being Russian, a very different culture than the US.
In addition to a shared aversion to voluntarily turning my possessions into billboards, I'd like to add there's real value in keeping computing devices generic looking.
By plastering your laptop with a completely unique combination and placement of stickers one can discover in videos or photographs online from conferences or talks for example, you make it a whole lot easier to pick out your unattended machine from a set, a hotel room, or luggage.
This can easily be leveraged by assisting targeted theft, destruction, or sophisticated physical access attacks.
> In addition to a shared aversion to voluntarily turning my possessions into billboards
I know that feel, but you can flair up with whimsy rather than corporate tribalism. For example, I have a strawberry from the game Celeste over my MBP's apple logo. It's even less billboardy than before!
> you make it a whole lot easier to pick out your unattended machine from a set, a hotel room, or luggage.
Laptops get mixed up all of the time in security when you fly. A sticker both reduces the probability of this happening accidentally and also makes it harder for a thief to claim innocence if you keep your eye on your laptop and notice someone nab it.
I agree with you that one part of the solution is to stop hiring people who only know that culture. The ability of individual agents to sow dysfunction far outstrips any individual contributions, in my experience.
However, I'm not sure that diversity is really a solution so much as a symptom of a solution, which is a healthy professional culture. Adding women and POC may work tactically but it's not a great long term strategy for solving that particularly problem -- it's entirely possible for women and POC to behave in this manner (although it's quite a bit rarer in my experience). Not saying you're suggesting this, but I've seen quite a few orgs where women and POC are hired as tokens and don't get to real positions of power where they have the power and responsibility to truly run things and reform/refactor systems. Reforming your organization so that it's not toxic to women and POC is an indicator that you're not going in the wrong direction, but it's hardly the end goal. In fact, it should be positively mundane, boring, average and the norm (and hopefully, will become that way soon).
Will these problems cease at that point? I doubt it. Callous, abusive leaders exist everywhere, and short of a massive societal shift where nonviolent communication becomes required reading for managers and leaders (which is verbatim what Satya Nadella did at Microsoft to great effect), the high leverage move would be to start there and not paper over the root.
These problems come from the top -- corporate and engineering leadership. If you have a culture that accepts and allows for brilliant jerks, there are a couple root causes:
1) Your leadership aids and abets it
2) Your leadership is apathetic about it
3) Your leadership dislikes it but feels powerless to stop it
If you've got issue class 1 or 2, it's likely more pragmatic to leave than try to wage a cultural coup (of course, more power to you if you can pull that off). If you've got issue class 3, well, maybe you've got some options. You can convince leadership that they've got a certain kind of problem -- easy enough. Then you've got to convince leadership that a given approach could ameliorate it -- doable, but tricky and not at all a guaranteed success. Finally, you've got the hardest part: actually implementing it. The resulting shift in power could result in folks trying to sabotage things, and success will be doomed unless it is unilaterally supported and individually guided through by leadership. This is rare, but possible (again the Nadella reference). But again, the goal should be to create a cohesive, respectful, supportive teamwork oriented environment, and the means should support that.
If you can pull that off, achieving not just diversity but a safe, sustainable place for folks of diverse backgrounds to thrive becomes a real possibility. I think that's why it's worth setting our sights there in the first place.
A practice I've become interested in but don't have an opportunity to engage with(doing the solo ISV thing) is "mob programming", since it confronts a few axes of the toxic-developer problem:
* There is some built-in diversity of skillsets, if not demographics, by pushing team communication into a continuous meeting format where non-developers are given space.
* It forces some vulnerability into the mix, which gets you into a less inhibited state: "I don't know, but" is way more common if you literally can't run off and prepare some slick answer to every question or hide behind your ownership of the solution space.
* The I/O bottleneck of having a whole crowd at one screen moves the emphasis away from the lines of code, and towards the broader parts of problem solving and getting feedback. Everyone that's experienced always says that it's not how fast you type that matters.
* Feedback becomes less punishing. Everyone gets a chance to drive, make some minor errors, and immediately correct them, which keeps everyone on the same level and encourages a healthy attitude to learning, vs the anxious/punishing "all-seeing-eye judgment" nature of batch code reviews.
But mostly, I like the idea of a hypothetical mind-melded "superdeveloper" emerging from a mob - a coder that pumps out extremely high quality code solving exactly the right problems in a single iteration, without breaking a sweat. I do think I've seen it in bursts in the past, just not in a systematic fashion. My experiences with pair programming definitely suggest that it adds intensity to problem solving that isn't there alone, and it makes me suspect that we may just straight-up be "doing software wrong" by focusing on quantities of code edits and not the overall communication flows.
> If you only hire white dudes in hoodies with stickers on their macbooks you're only going to get the toxic gamergate crowd, which has not gone away at all.
Um, excuse me? I'm a white dude who wears a hoodie and has a sticker on my macbook, and I am not in "the toxic gamergate crowd." If anything you're the one being toxic.
I'm tired of dealing with people like this. It does not have to be this hard. Tech companies should fire more people just for being jerks.
> I was mad that, while I spent my nights learning F#, my daughter started calling everyone around “fathers”.
Huh, you mean missing out on the good parts of life while martyring yourself for a company that does not give a shit about you can lead to resentment and toxic work culture? YOU. DON'T. SAY.
Honestly, a lot of the time management will stoke the "hero coders" ego so they will do the free work.
Management: Oh, Hero Protagonist! We need you! How will we ever earn enough to give our CEO the 15 million bonus if you do not martyr yourself for this feature?
Hero Protagonist: They're right. I am the best and smartest. I better dump all of my emotional labor onto my coworkers and significant others and bury myself into doing free work for my company.
> When you work as a developer, you always have to argue. You, as a team, arrive on a solution after a lot of argument, even though we call them “discussions”.
I do not feel this way at all. There are productive ways to have discussions that do not involve getting defensive and arguing.
The fact that you have to drag all of your discussions into toxic arguments in order to save face is... sad.
> And yet it’s somehow important than your arguments “win” more often than not, just to feel good and confident in your power.
No, it is not. That is just how you feel to justify your constant toxicity.
I see very little "power" in bullying all of the other developers on my team into following my opinions.
It sounds more to me like the author is trying to illustrate those toxic thought processes (based on his personal experience) using that second-person perspective, rather than actually claiming that those things are true. It's just the language barrier that makes it not work as well. Check out the last few paragraphs:
> This review I kicked off the article with? I didn’t send it. Instead I gave the guy a couple of comments and politely asked to fix a couple of things. No big deal if the code’s not good, I can fix it myself it I need to. But I can’t fix the psyche of a guy broken by dozens of harsh reviews.
> My personality today isn’t my disease. It’s a disease of the whole industry, at least in Russia. Our mentality is predicated on the cult of power and superiority. And that’s what we need to fix.
I have met quite a large number of software developers in my day, and if I didn't recall meeting them face-to-face, I would describe the behavior of around 15-20% of them as "obvious satire".
If the author is coming around by realizing the error in their ways; a way that seems painfully obvious to you, I don't think it does anyone any favors to chastise them for not knowing sooner. Its clear to say not everyone grows in environments that reinforce positive thinking.
Further, unless I am misreading the tone of the article, the author is listing out all of the defects you quoted not to defend or justify, but precisely to highlight their ridiculousness and falseness.
> I see very little "power" in bullying all of the other developers on my team into following my opinions.
The author actually addresses that. It's just the style of the article takes you on a journey in his mind before getting to the conclusion. It's a different style of writing. From the article:
If we were being laughed at while young, it doesn’t mean you have to return the favor later on. The vicious cycle can easily be broken. Life becomes easier if you learn to lose arguments, if you can admit that another developer is more talented than you.
The author is talking about a learning experience he had regarding a deficiency he identified (being a jerk) in his own work (reviewing other coders' code).
Meanwhile, your comment levies personal criticism toward him using pretty inflammatory language ("toxic," "sad," "asshole," "jerk," etc.), because of the way he responds to people who are learning.
I also felt weird reading this. I really want to see some of this person's "code reviews." Two hundred comments on a pull request? How many of the comments are useful and actionable?
I love having my code thoroughly reviewed. It does not ruin my life to see someone say "I don't think this is a good idea." I think you have to be operating on a different level to be ruining lives through pull request comments.
I've seen a few cases where otherwise talented developers would kind of miss the point of code reviews and focus on code style much more than the code itself, nitpicky stuff like sorting of imports, etc., leaving hundreds of comments while at the same time overlooking quite serious bugs.
Presumably, codestyle comes easy for them due to their neurotype, but they have a hard time reining themselves in and just end up wasting thousands of other developers' hours for zero business value.
I've never understood why these people don't just spend a few minutes adding some rules to ESLint or whatever.
However, while this person really feels like an asshole, it's incredibly mature of him to recognize his past behavior and try to improve.
For many developers, it's incredibly satisfying to "be right", and due to the fact that's it's easy to leave some comments, the humanity of their coworkers can often take a backseat.
Step number 1 for any dev joining my org; learn the difference between "how I would have done it" and genuine optimisation/bugs.
Its probably too carte blanche; but ive reached the point where I will refuse to rule on codestyle issues (general answer; whoever did it first sets the style)
I sometimes fall into a similar trap. Usually on the larger changelists where I was struggling to keep the focus necessary to find the serious bugs, but not being comfortable only looking at half the files in the code review - feeling like I'd not be doing my part of the job I didn't at least look at them. I have to remind myself that there's a limit to how useful nitpicking can be, and that it can become counterproductive as well.
Sometimes the quite serious bugs are just hard to notice, as well. I recently reviewed a large changelist dealing with lots of multithreading and locks, and cases where locks aren't taken to avoid deadlocks. I have 0 confidence I caught all edge cases, which terrifies me for multi-threaded code.
I don't have your context, but if the changelist author is writing multithreaded code where there's no way that a reader can convince themselves of the correctness, then my heuristic is that the burden is on the author to improve the code to be easier to reason about and add enough tests to exercise all of the interesting code paths. There are always techniques to simplify code or make it easier to grasp.
I have similar heuristics. I'm drawn to Rust because I really like the idea of having to prove it's at least data race free to the compiler (not that this will solve the problem of deadlocks.) In most situations I'd push back on code that got half as tricky with it's multithreading.
Simplifying is easy. Early versions of this code, years ago, were simple. They weren't even multithreaded! Just a simple implementation to unblock other devs.
But now it's a highly used bottleneck that must be high throughput, low latency, support asynchronous cancellation of requests, interacts with the main thread for third party APIs that aren't thread safe (such as d3d9), interacts with the main thread for our own APIs which aren't thread safe by design (our debug replay system replays the events of the main thread), but must avoid synchronizing with the main thread for performance elsewhere...
Simplifying this without performance or feature regressions is significantly more difficult. Possible, but difficult. And benefits from slow, testable, incremental changes. But that means reviewing incremental diffs on the large and complicated existing system in the interim. At least it has some of our best test coverage, including lots of tests to help try and tease out threading bugs. They don't always succeed at that, but they do help.
Consistency of code style does matter, up to a point, to people reading and understanding the code. As you suggest, setting up ground rules and tooling helps
Code authors who don't see the value in consistency of code are potentially a problem - if authors are submitting code reviews with hundreds of actual style issues, that's either a failure of process or the author to write readable code (not sure if that's what happened in your examples, to be clear).
It depends on how important you think it is to optimize for future readers or maintainers of the code. I weigh that pretty heavily and a do think relatively minor style issues impose a tax once they're pervasive in a codebase.
Tooling is great, but I think there are a lot of style issues that aren't readily enforceable with linters - commenting, naming, control flow, abstraction, etc.
When I was a student I had this same experience peer-editing prose. In reading a classmate's paper, I found endless nits to pick about syntax and minute points of diction or style, but much less to say about the actual argument. The main reason for this is just that it was easier. It doesn't take much effort to call out bits that strike one as unaesthetic, but evaluating the substance of a paper requires that I really think about what it's saying.
Great developers see the opportunity to grow someone.
The former shows ownership of the code, but is generally bad for a business.
The latter understands that a successful business is more than simply his competence.
Reaching this level of realisation is critical for being a good lead; I interview loads of clearly solid programmers who havent figured this out, and sadly price themselves out of the market (I blame their previous managers for missing their growth opportunity)
"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."
I can tell you I haven't had this feeling ever, but not for reasons I'm proud of. I hate doing code reviews. I have hated doing every one. I have disliked having to type every comment I have made on a code review. Each time I hope that everything is good enough and I can just approve it. And I won't comment unless I am convinced it is important.
I don't hate the coder, or the process that requires code reviews. I know code reviews are very important for many different reasons, so I don't shirk them, but I have become afraid of being an expert on a large team as I will have to do more of them. Not sure what is wrong, I love coding. Reviewing good code isn't so bad, reviewing bad code takes all the joy out of coding for me. If it's bad enough, I know that I need to make a lot of comments, and I'll need to make some more when it comes back with changes. I think we are just wired differently.
EDIT After further thought, I think a fundamental difference is I really want to see my coworkers succeed, and don't enjoy their failure. Also I dislike all forms of toil. But I think maybe part of the authors problem is not able to comprehend that some people can be nice like that, genuinely. In fact, I assume my coworkers want me to succeed as well, and I really think most of the time they do. But I am glad he decided to start acting that way, which must be especially difficult if you don't believe others will treat you the same way.
Turn it around - make sure each person has the chance to review one anothers' code, and make sure the senior members of the team lead the way in using those reviews as opportunities to both highlight great things in the code and to propose changes /in a way that teaches/ how you're analyzing it and what made you propose that change. If they challenge it, great! They're thinking about your reasoning on their own, and you can use that to come to a new consensus on the team's approach to development.
There's great health to be gained from code reviews. But they really do need to practiced, reconfigured, and practiced again so your team gets what they need from it.
> After further thought, I think a fundamental difference is I really want to see my coworkers succeed, and don't enjoy their failure. Also I dislike all forms of toil.
I'm in both of these camps too. I think I'd dislike code reviews if I didn't remind myself:
* Code my coworkers write is code I don't have to write, so it's saving me toil.
* Issues I catch in review are issues that won't lead to late night debugging sessions catching heisenbugs on the eve of shipping our product, so it's saving me some of the worst kinds of toil.
A little work now to save me a lot of work later. And I've been burned by suddenly getting stuck maintaining & bugfixing code that I got lax about reviewing. Each issue I catch, I think "thank goodness I caught that now - that'd be a pain in the ass to debug".
It also makes me genuinely appreciate review feedback catching all the dumb shit I might do, which can really help when your quick change turns into a slog of missed edge cases, "can you fix X while you're in there", etc.
I find using automated tools as part of the pipeline, like codefactor, are a decent first step to sort out some of the glaring issues. As they're automated and act as quality gates before an actual human comes to do a review then they're not as bad as they could be. Sure there scope can be limited, depending on what language/framework you're using, but common ones catch quite a lot of obvious issues.
I can't say I enjoyed code reviewing but I never felt like I was being mean to my teammates. I was really happy when a teammate found a bug in code review of my code rather than that bug getting checked in. I was also really happy when they shared knowledge and I learned something. Maybe they shared a pattern I was unfamiliar with or maybe they pointed out an existing function I could use I didn't know was in the code.
I assumed the same was true for them. I learned from another team member to try to phrase comments as questions. "Did you mean to do X here? It seems like it might have issue Y" etc...
I hated code reviews when I first had to do them because it was a toxic place. I hate them less upon focusing on what you describe. I'll add to it that I've taken a more conversational tone. Sometimes I'll just add comments to talk about how this code fits into the whole system and weird legacy stuff that is affecting what they are writing.
Bugs are easy.... you just point out the bug and move on... design flaws are hard... it's hard to bring up the fact that while yes, your module solves this one immediate problem, it was written so rigidly that two sprints from now the whole thing will need to be re-written scratch. Or maybe you took a particular approach that seemed easy to implement, but it won't do async stuff properly and so it needs to be re-worked from the ground up. And I get you solved your immediate problem for your immediate task, but whoever works on this next is going to have to completely re-think the approach and that's not theoretical, that next week when we add more behavior.
You ever worked on a team a where someone would go write code in their own crazy way that wouldn't follow any sort of existing pattern or take advantage of existing tooling? So they spend like a week on a simple task because they re-wrote the strings.c because they didn't want to include it? Yeah, they get fired eventually, but they are the worst to write code reviews for, because you go into it just thinking WTH, why are they doing it this way, this entire approach is convoluted and error-prone and rigid and fragile and complicated.
If a super engineer has advice for me on how to deal with this stuff, I'm all ears. I usually just ignore it unless it directly impacts me and then go back and re-write it when we have to add on to it/make it interop with some more code/release it as an available API.
I had a coworker who had a 2,000 line PR that took me 3 days to review, it was like code breaking. Custom abstractions upon custom abstractions, abstractions in custom imported utilities for a one-liner that was only used once. Basically, instead of writing the code, he made code that generated the code that was needed, once the program was running. Dear Lord. Like somehow this monstrosity was good because instead of creating the feature, he built tooling to make many identical features whenever we want! Except nothing can be different, if it's different we need to rewrite the whole thing.
Every 4 hours or so I would ask my boss if I really had to do it. He told me to just keep going, I think he was building a case for letting him go. I still only made like 10 comments, needless to say they weren't taken well. What a nightmare.
Tangential to this whole thread, but guys like that, every day he worked took two days for other devs to fix/undo his work. Took my company a few years to catch on, he was a senior dev and also good at talking himself up. I would have paid him the same salary to go sit on a beach somewhere and enjoy himself instead of touching the code, we'd all have been happier. In fact the world would be a better place, because now he is presumably working somewhere else doing the same things.
> "If a guy brings me his code, and it has mistakes, it brings insane pleasure from how smart I feel"
This quote made me laugh from the exaggerated "insane pleasure" in other's mistakes. But I feel this way sometimes. I'm completely sure it comes from my own insecurities, so that when I see another guy missed something I wouldn't, I get a feeling of relief - actually I'm not the worst programmer in the world.
So for me this feeling comes from impostor syndrome - I thought I was bad, I saw another guy who was worse, I felt good. Insecurity is really a self-fulfilling prophecy. I'm trying to work on it by forcing myself to ask questions I think are dumb, and not fearing judgment from others. This in turn will also influence my peers to feel less afraid of their doubts, turning the spiral around.
A study from Google [1] agrees with this: they found that the best predictor of team quality was the willingness to ask questions and express opinions without fear. Quoting Google:
"In a team with high psychological safety, teammates feel safe to take risks around their team members. They feel confident that no one on the team will embarrass or punish anyone else for admitting a mistake, asking a question, or offering a new idea."
While I have similar feelings, I enjoy code reviews on high quality code. I like reading high quality code. But yes, the enjoyment of a code review is inversely proportional to the amount of feedback required. Some people enjoy mentorship more than others, and that's ok.
I do tons of code review and enjoy it. I approach it with both learner/teacher attitude (not critic) and treat it as project learning and communication exercise.
Best way of doing code review is by asking questions.
Recently coworker recommended this talk which suggest good attitude IMO - you can find it under "strong code review" from rails conf
> And I won't comment unless I am convinced it is important.
I tend to lean this way as well. Im of the mindset that most things that are "caught" in a code review don't add any real value. Or maybe I just have bad coding standards.
>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.
Are you kidding? Call me a liar if you want, but I don't feel that way when doing code reviews, and never have.
Yes, when I was a teenager I wielded my knowledge like a titan. The meager amount of knowledge I had.
But as a professional, I have never once been mean-spirited in a code review. Everything I send back is to keep the code clean and maintainable and to help the developer improve.
Not everyone feels the need to be "right" all the time. And by that, I mean being perceived as correct, even if they aren't. For me, that was a function of actual skill.
The better I got at programming, the less I needed everyone else to acknowledge how good I was.
Even better, that applied to the rest of my life, too. I haven't felt the need to show off my intelligence or skills in a long, long time. I simply do my thing and if/when someone notices, it's a great feeling. If they don't, I still enjoyed doing it well.
So you haven't -ever- been overly critical or tried to show off? Because your comment made it sound like you have improved over time and feel less of a need to improve yourself now.
I couldn't really read it fully kind of in a run. But it seems like that the author isn't just talking about the language but that commenting on every bad issue of the code is bad? I don't agree with that.
I have a done a lot of review not as much lately but my principle is pretty simple - Can this be made better to the best of my understanding and knowledge. I will give it the same attention as I will my code. When my first version of a code or the POC is done I will think of what can be improved upon with the goal of the final version to be cleaner, faster and more robust if possible. My job is to help the author and the company to make sure together we get the best version possible merged of course with an understanding of cost-benefit.
The language is of course important but not catching issues on every line if that's what I find. I will appreciate if someone does the same thing for me. At one point I have reviewed more than 50% of the codes in my company and I know how hard it can be to do so with full care so I take it as a favor when somebody does a good review on my code finds mistakes or potential improvement. I have had way junior developers giving me feedback ranging from better names to serious bugs. I have also always explained why a certain idea should be explored or might be better with the author regardless of how junior they might be. I have always liked the developers best who leaves the ego out of it both a reviewer and author of a certain piece of code. Pride and Ego I find are unrelated. I take pride in giving my best both when writing a code or reviewing one all the while knowing not just that there are other developers much better than me but even someone who is on average worse than me can still find potential improvements in my code. I like to think that I can take any comment on its merit and not my perception of the person.
Code reviews aren't supposed to be humiliating. This article does a lot to illustrate how a negative attitude in review comments can be counter-productive, but the conclusion feels like code reviews that point out dozens of flaws in the change are bad.
I disagree that pointing out every flaw in a code review is a bad thing to do. I believe that these could be positive experiences with the right attitude from both the reviewer, the submitter, and the team's management. A few things that really bugged me while reading this:
- The reviewer feels like pointing out a flaw is an adversarial zero-sum sort of action that affirms his superiority over the submitter.
- The submitter will feel bad about each feedback given (at least in the POV of the author). If the feedback is mean-spirited, then sure, but if it's constructive, then this shouldn't be the case.
- His team fired the developer who received "too much" feedback. I'm sure there's missing context here, but if that's the main reason, then this is pretty messed up.
Finally, his conclusion to not submit the review really bugged me. I do agree that no review is better than a mean-spirited destructuve review, but IMO this is still a failure on the part of the reviewer. He should write the review in a constructive manner and work with the submitter to hash out the issues. If it's an argument, then fine, work through it... but teammates should help each other grow and be better.
In this case, by not submitting honest and comprehensive reviews, I believe his team and his product suffers in the following ways:
- The reviewer has to waste his time "cleaning up" the code later on.
- The submitter loses out on the knowledge transfer that takes place during a good code review.
- The product potentially has unfixed bugs.
- A fear of conflict is further instilled in the team, and criticism now becomes off-limits because it is considered destructive due to the negative attitudes of everyone involved. This further hurts product quality, and the result is that everyone codes in isolation out of their ivory towers instead of collaborating on a product together, among many other detrimental effects.
I think the solution at least begins with:
- A better team commitment to personal growth, and a sense of responsibility on the part of everyone to help out their teammates in this respect.
- Clear team guidelines on lint/style standards to eliminate the need to argue about it.
- An attitude that feedback is about the code and NOT about the person submitting it. Everyone should feel like an owner of the code, and all discussion should be about the code and how to improve it.
- Comprehensive code reviews. Note everything you find. Maybe call an in-person review if you feel like there's too much to comment on.
The secret to good code review is addressing the code, not the person. Ask questions and make suggestions, rather than being critical of the other person.
Instead of "Why would you ever do it this way?", try "I think I would probably do this like (example)"
Also, be liberal with complementary reviews, as well. I see a lot of pull requests where the solid, workday, well written code is just passed over, or given a ":+1:", even though it's definitely worthy of praise.
You wrote a new class extracted from an old confusing method, it has good tests, and the feature is working as requested? A triumph.
It's one of the things I'm most pleased about at my current gig that reviews are in general collegial rather than adversarial.
I wish I had a magic blog-post-length formula for how that happened. I don't. I know I personally talk about it in interviews, and we hire for developers who think about development as a team sport rather than a solo FPS rampage, and we talk about code review sort of a lot in onboarding.
The way I explain it to noobs is that a collegial review culture is a question of trust. When we all believe that we are all on the same team with the same goals, we feel safe, and we can trust that (1) critical feedback we receive from others is intended just to improve the code, not to score points on us personally, and (2) if we give feedback that is critical, it will be received in that spirit as well. And it's our responsibility to give critical feedback where the code under review doesn't meet our standards of efficiency, readability, and maintainability.
Grandstanding or mean-spirited reviews break down the team-wide spirit of trust and the feeling of safety; they are a far greater danger to the integrity of the code base than a badly-coded method or spotty test suite.
The author sounds like somebody I probably wouldn't want to work with. They seem to have an inflated view of their own ability. Did he consider pairing with the person or trying to mentor them? Russian culture is no excuse for being a dick, be the person you want to be and treat others how you would like to be treated yourself.
Being tough on people seems to work if the knife isn't aimed at yourself. Eventually, it will be.
People that are really good at things are graceful and helpful usually.
PS-Question: Author mentions Russian culture being an influence. I feel like he's saying there is an emphasis/high value on ultra-competence ultra-stoic unshakability sort of state of being? The meme is that Russians are intense, which I fully respect.
This guy ties his ego to his code, and assumes everyone does the same. The proposed solution is to not devalue the person by devaluing the code.
This seems exactly backwards to me.
Disconnecting ego from the work was the first big lesson I had when I started working in software. I hear parallel ideas from friends across industries, in fact an electrical contractor explained to me how he expects it of his apprentices just a few days ago.
I once had a joint software team with a client and put my foot in my mouth assuming they separated code and ego the way we did. The first time I reviewed some of their work, there was a design decision that I sorta assumed was the best of a few bad options. So I asked the developer why they did it that way.
I just wanted to hear some reason, any at all would have done. Something special about that approach, or that they considered the others and found it was, in fact, the least worst. Then we'd just move on, happy enough with our implementation. I'm used to doing that (and having it done to me) dozens of times a month. But what I got was not a defense of the design decision, and more a defense that he made a design decision. I touched a nerve. I now know to hedge, apologize before giving feedback and ask questions by asking around them. I don't really think it's an overall plus. Especially when the business domain puts high standards on us.
> Disconnecting ego from the work was the first big lesson I had when I started working in software.
100% agree. Any code you write for your employer is not your code - you should be comfortable leaving it forever tomorrow if a better opportunity presents itself elsewhere.
I wish I had 100 up votes for this quote: "Disconnecting ego from the work was the first big lesson I had when I started working in software."
Until you can do that, you'll always be a lesser software developer. You'll concentrate on maintaining your ego instead of developing the best software for the problem at hand. In a way that's the gist of the article.
It's interesting, but at the company I've worked at, where we've had dozens of senior developers come and go, we've never had this problem. Our approach to code reviews is purely educational. "Here's how we do it so we have the least amount of trouble understanding each others code." Everyone seems to be good with that, and egos get left at the front stoop. I've learned lots, I've taught lots, and I've never felt better than anyone or worse than anyone. It's all about the learning.
And let me tell you, when you hire some junior dev, and he gets these code reviews, and 3 years later he's a senior dev and anchoring a project, it's a pretty good feeling. I think better than any feeling you can get from lording knowledge over others.
> Until you can do that, you'll always be a lesser software developer.
Reminds me of a joke-ish comment I saw on here some time ago: The difference between a junior and senior developer is the willingness to say "that bug was totally my fault".
Yeah I had to learn this lesson, and I think I will continue to find my ego cropping up in strange places, and again practice letting go.
My ego drove me to work extra hard to try and impress others. I would become petulant if I didn't get my way. I look back on the first few years of my career with so much cringe!
Couldn't agree more -- one of my first real important lessons in the industry on the soft skills side of things. What went along with that was that it really took a while to sink in and figure out not just how to personally avoid it, but in a team setting how to help build a culture of that.
It's a theme I hear a lot when I see interviews of powerful business people - you don't do anyone a favor by pretending something that is bad is good.
Agreed that the OP took the wrong lesson - the fix is not to lower standards, the fix is to learn to deliver honest and complete feedback with empathy.
I've had my fair share of controversial code reviews, both on the giving and receiving end.
I no longer ask 'why?' inside a code review, because it immediately puts the author in the position of justifying what they did, while feeling like they have to defend a decision.
Instead, I'll take a coaching (or a more socratic) approach that doesn't risk making the author feel dumb, or like they never considered the alternatives: "what was your reason for this?", "did you try x,y,z?", "I wondered if this would work?", "what is this for?", "what does this do?"
In addition to that, if I have a suggestion (or a suqqestion [0]), I will always follow it up with a refactored, copy/pastable code snippet to make it clear what I'm talking about and also to help push the review forward. This is especially useful for learning by example.
This means that code review takes more time and effort, but that investment of time has value. Code reviews become an implicit, shared mentoring space, and people like it when they see your review pop up.
I find it fascinating that people don't immediately translate "what was your reason for this?", "did you try x,y,z?" and "I wondered if this would work?" to "why?" "why?" "why"?
BTW does anyone have a good guide, book, tutorial on doing code reviews? Everywhere I've been its kinda adhoc with no rules or guidelines.
Often code reviews come in after someone has put a few weeks of work into something and its too late to change how it was done. Or a review is passed in by someone in a hurry without actually critiquing it.
my one suggestion is to make your code reviews have more questions than statements. so instead of: ‘forgot to check for null here’, do something like: ’do you think we can have a null issue here?’. in my mind, this word change makes the reviewee feel like they’re not just being told what to change, but actually have a sense of code ownership and control over their work
We had a senior engineer (who sadly moved on to a different company) periodically give a talk to new hires about good code review practices to set expectations and ground rules. Some of the key points:
* Authors are expected optimize for readability of code, even if it takes longer to get the code in.
* Reviewers should generally only start reviewing code if they are ready and willing to continue the review to completion (i.e. no drive-by reviews). Once they start reviewing they should try to minimize response time.
* Focus the reviews on testing, correctness, readability
* Consistent code style is important (see optimizing for readability)
* Break code reviews into the smallest reviewable units. Often these are somewhat large because parts of the change don't make sense in isolation
* The code author is responsible for writing code in a way that it convinces the reviewer that it is correct (see optimizing for readability)
* You should approach it collaboratively - you're working together to get the work done and make the codebase as maintainable as possible
* You should aim to leave each bit of the codebase that you touch in at least a good a state as it was previously
Articles complaining about the reality of code reviews seem to be commonplace. But if they're not working out, why not just abandon them, maybe try something different?
I'd argue that if you have a toxic team culture then no matter what you do things will escalate. Code reviews are a focal point but if they didn't exist then the focal point would move somewhere else. For example, engineer A commits some code and then engineer B rewrites it and then they get into an argument about it during a meeting. Toxicity in code reviews is at least auditable so a pro-active manager can nip it versus someone going off in a meeting they weren't part of (in which case it becomes a she said/he said situation).
I don't think "toxic team culture" is the only way code reviews get frustrating. Other issues include frustrating waiting for reviews to be completed or the flip-side of people being knocked out of flow to do a review because "reviews are top priority". Relatedly, arguments about what the appropriate size is for a reviewable change.
The one alternative that (some) code-review advocates seem willing to accept is XP-style full-time pair programming. But that leaves even less room for individuality and solo accomplishment.
The issue is that code reviews are hard on people and good on code bases.
Dollar for dollar, code reviews are more effective at finding and fixing bugs than any other activity we can do, including QA tests. However code reviews are very hard on people, and can easily create conflict.
Which one matters more, and how careful people are, varies widely by organization.
My recollection is that the studies showing that code reviews have high defect yield are for extremely strict methods. Everyone in a room, prescribed roles for each person, line by line, checklists, forms, the whole nine yards.
Comments on pull requests are not on the same level and those findings shouldn't be hastily generalised.
And this is why we hire for empathy, and pair-program instead of doing code reviews. Code reviews and features branches don't cause this type of behaviour, but they are a fertile field for this to flourish.
I'd rather have ten mediocre developers who can be part of a team than one gifted diva.
I had a friend in elementary school that was a lot like this. As we stayed friends all throughout our school days, he would constantly argue with me about the stupidest things. Even when I was right and could demonstrate he was wrong, he'd still try to win.
Ironically, we're still good friends because I learned to look past this BS and not care.
163 comments
[ 3.0 ms ] story [ 96.6 ms ] threadA lot of elitism comes from going through a high level of discipline for no reward.
Just do what is fulfilling
Rich people collect money; find themselves empty inside; seek more money;
Find no reward in shredding through meaningless suffering; demand payback; be forced to extract blood from your fellow turnips.
basically starting a rant about being a tightass about code review with a bad piece of code
> No big deal if the code’s not good, I can fix it myself it I need to.
A mentor can't be passive like that. Don't leave 200 comments like you used to, but distill them into something smaller, more cogent and digestible. More importantly, drop the idea of "good" and focus on what the implementor was thinking. I think the author understands the need for more effective communication in code reviews, but unfairly reduces the exchange to an "argument."
It's also dangerous to correlate work-life balance with incompetence. It's not a contradiction to be good at your job, current with the industry, and present for your family.
The purpose of code reviews is to get quality code. Quality code makes the project work. But why do we work on projects to begin with? To benefit people.
This guy is realizing the conflict between the two goals of getting quality code but also serving people.
People need criticism to improve, but it needs to be constructive, otherwise the fragile human psyche inside of every person may not recover.
My feeling is that criticism, no matter what the domain, should be relatively dispassionate (neutral in tone), and come from a desire to help and with a dose of humility. Of course, I sometimes fall far short of that ideal.
This is why I try to word reviews as questions, not statements of facts:
"Is this supposed to be reversed? I think it might not do what you want it to"
"Having a bit of trouble following this logic.. is there a way to make it more clear to future readers?"
"I might be missing something, but I don't see how this code can be reached?"
Don't give the review in such a way that implies you know everything and the other person was wrong. It should be a dialog, and be based on the assumption that we don't yet know the right answer and are working together to figure it out.
These comments stuck out:
"Because I do code review for self-identification."
and
"It turned out that, instead of becoming a good coder, you simply have to convince others you’re a good coder. This behaviour begets a vicious cycle that produces not professionals, but toxic asshats."
One way to nip this culture is stop hiring people who only KNOW THIS culture.
If the author is reading this, there is one solution I have encountered: diversity. If you only hire white dudes in hoodies with stickers on their macbooks you're only going to get the toxic gamergate crowd, which has not gone away at all.
Believe or not there are other kinds of programmers in the world, and the most enjoyable products I've worked on were ones that had more women and more POC on their teams. Toxic asshattery can be minimized by not giving them complete control over the culture of a workplace.
Please, assume just for a minute that we aren't all idiots. Of -course- everyone has the capacity to be an idiot, but your unnecessary and highly defensive response is very telling.
The implicit -- wait, no -- the explicit argument of your grandparent post is that all white men are the same.
Of course people are going to push back on that.
Mindlessness mixed with insecurity is a toxic brew.
I still have my stance but I've found that people that get sucked in, on both sides, will get hypersensitive about detecting their opposition, and then they project all of the ideas they dislike the most onto the person in real life who exhibits a hint of it.
You might find that you are trying to find things to be angry about. You might feel very strongly that masculinity is toxic but you don't really have any examples in your life except for the two and a half times you got cat called. The "gamers" that hate minorities and women might be on the forefront of your mind, but only because you spend too much time consuming narratives online.
Genuinely how many people would be concerned about toxic masculinity if the internet didn't exist. How many people would be concerned about the culture war at all if outrage couldn't be shared.
As much as you might like to blame it on a particular skin color or gender, your happiness and peace of mind is your responsibility. If you are frustrated, angry, or sad about gamers, the gamers aren't the problem, you are losing control of your mind.
But do you really care, seeing as to how you've already furthered a theory of someone else being either emotionally hostile or defensive?
And do you really not see the obviousness of how a certain situation plays out, of someone using white guy with stickers on laptop as a symbol for toxicity? Or do you see the obviousness, and that's why you're playing the situation out this way?
You just crossed the line from making this about the issue to making it personal. That's not okay.
He didn't imply that women and non-whites can't be self-centered egotists, only that they're less so than white men. Despite the author of the article being Russian, a very different culture than the US.
(Sarcasm, Office Space reference, I'm with you on the stickers.)
By plastering your laptop with a completely unique combination and placement of stickers one can discover in videos or photographs online from conferences or talks for example, you make it a whole lot easier to pick out your unattended machine from a set, a hotel room, or luggage.
This can easily be leveraged by assisting targeted theft, destruction, or sophisticated physical access attacks.
I know that feel, but you can flair up with whimsy rather than corporate tribalism. For example, I have a strawberry from the game Celeste over my MBP's apple logo. It's even less billboardy than before!
> you make it a whole lot easier to pick out your unattended machine from a set, a hotel room, or luggage.
Laptops get mixed up all of the time in security when you fly. A sticker both reduces the probability of this happening accidentally and also makes it harder for a thief to claim innocence if you keep your eye on your laptop and notice someone nab it.
However, I'm not sure that diversity is really a solution so much as a symptom of a solution, which is a healthy professional culture. Adding women and POC may work tactically but it's not a great long term strategy for solving that particularly problem -- it's entirely possible for women and POC to behave in this manner (although it's quite a bit rarer in my experience). Not saying you're suggesting this, but I've seen quite a few orgs where women and POC are hired as tokens and don't get to real positions of power where they have the power and responsibility to truly run things and reform/refactor systems. Reforming your organization so that it's not toxic to women and POC is an indicator that you're not going in the wrong direction, but it's hardly the end goal. In fact, it should be positively mundane, boring, average and the norm (and hopefully, will become that way soon).
Will these problems cease at that point? I doubt it. Callous, abusive leaders exist everywhere, and short of a massive societal shift where nonviolent communication becomes required reading for managers and leaders (which is verbatim what Satya Nadella did at Microsoft to great effect), the high leverage move would be to start there and not paper over the root.
These problems come from the top -- corporate and engineering leadership. If you have a culture that accepts and allows for brilliant jerks, there are a couple root causes:
1) Your leadership aids and abets it
2) Your leadership is apathetic about it
3) Your leadership dislikes it but feels powerless to stop it
If you've got issue class 1 or 2, it's likely more pragmatic to leave than try to wage a cultural coup (of course, more power to you if you can pull that off). If you've got issue class 3, well, maybe you've got some options. You can convince leadership that they've got a certain kind of problem -- easy enough. Then you've got to convince leadership that a given approach could ameliorate it -- doable, but tricky and not at all a guaranteed success. Finally, you've got the hardest part: actually implementing it. The resulting shift in power could result in folks trying to sabotage things, and success will be doomed unless it is unilaterally supported and individually guided through by leadership. This is rare, but possible (again the Nadella reference). But again, the goal should be to create a cohesive, respectful, supportive teamwork oriented environment, and the means should support that.
If you can pull that off, achieving not just diversity but a safe, sustainable place for folks of diverse backgrounds to thrive becomes a real possibility. I think that's why it's worth setting our sights there in the first place.
* There is some built-in diversity of skillsets, if not demographics, by pushing team communication into a continuous meeting format where non-developers are given space.
* It forces some vulnerability into the mix, which gets you into a less inhibited state: "I don't know, but" is way more common if you literally can't run off and prepare some slick answer to every question or hide behind your ownership of the solution space.
* The I/O bottleneck of having a whole crowd at one screen moves the emphasis away from the lines of code, and towards the broader parts of problem solving and getting feedback. Everyone that's experienced always says that it's not how fast you type that matters.
* Feedback becomes less punishing. Everyone gets a chance to drive, make some minor errors, and immediately correct them, which keeps everyone on the same level and encourages a healthy attitude to learning, vs the anxious/punishing "all-seeing-eye judgment" nature of batch code reviews.
But mostly, I like the idea of a hypothetical mind-melded "superdeveloper" emerging from a mob - a coder that pumps out extremely high quality code solving exactly the right problems in a single iteration, without breaking a sweat. I do think I've seen it in bursts in the past, just not in a systematic fashion. My experiences with pair programming definitely suggest that it adds intensity to problem solving that isn't there alone, and it makes me suspect that we may just straight-up be "doing software wrong" by focusing on quantities of code edits and not the overall communication flows.
Um, excuse me? I'm a white dude who wears a hoodie and has a sticker on my macbook, and I am not in "the toxic gamergate crowd." If anything you're the one being toxic.
I'm tired of dealing with people like this. It does not have to be this hard. Tech companies should fire more people just for being jerks.
> I was mad that, while I spent my nights learning F#, my daughter started calling everyone around “fathers”.
Huh, you mean missing out on the good parts of life while martyring yourself for a company that does not give a shit about you can lead to resentment and toxic work culture? YOU. DON'T. SAY.
Honestly, a lot of the time management will stoke the "hero coders" ego so they will do the free work.
Management: Oh, Hero Protagonist! We need you! How will we ever earn enough to give our CEO the 15 million bonus if you do not martyr yourself for this feature?
Hero Protagonist: They're right. I am the best and smartest. I better dump all of my emotional labor onto my coworkers and significant others and bury myself into doing free work for my company.
> When you work as a developer, you always have to argue. You, as a team, arrive on a solution after a lot of argument, even though we call them “discussions”.
I do not feel this way at all. There are productive ways to have discussions that do not involve getting defensive and arguing.
The fact that you have to drag all of your discussions into toxic arguments in order to save face is... sad.
> And yet it’s somehow important than your arguments “win” more often than not, just to feel good and confident in your power.
No, it is not. That is just how you feel to justify your constant toxicity.
I see very little "power" in bullying all of the other developers on my team into following my opinions.
> This review I kicked off the article with? I didn’t send it. Instead I gave the guy a couple of comments and politely asked to fix a couple of things. No big deal if the code’s not good, I can fix it myself it I need to. But I can’t fix the psyche of a guy broken by dozens of harsh reviews.
> My personality today isn’t my disease. It’s a disease of the whole industry, at least in Russia. Our mentality is predicated on the cult of power and superiority. And that’s what we need to fix.
The article is thought provoking. It makes good points, even if you don't like the author. I can relate to a lot of his examples.
Further, unless I am misreading the tone of the article, the author is listing out all of the defects you quoted not to defend or justify, but precisely to highlight their ridiculousness and falseness.
The author actually addresses that. It's just the style of the article takes you on a journey in his mind before getting to the conclusion. It's a different style of writing. From the article:
If we were being laughed at while young, it doesn’t mean you have to return the favor later on. The vicious cycle can easily be broken. Life becomes easier if you learn to lose arguments, if you can admit that another developer is more talented than you.
Meanwhile, your comment levies personal criticism toward him using pretty inflammatory language ("toxic," "sad," "asshole," "jerk," etc.), because of the way he responds to people who are learning.
Interesting juxtaposition, that.
I love having my code thoroughly reviewed. It does not ruin my life to see someone say "I don't think this is a good idea." I think you have to be operating on a different level to be ruining lives through pull request comments.
The problem is that the traits that make someone a 'jerk' are often the same traits that make a '10x developer'.
Presumably, codestyle comes easy for them due to their neurotype, but they have a hard time reining themselves in and just end up wasting thousands of other developers' hours for zero business value.
I've never understood why these people don't just spend a few minutes adding some rules to ESLint or whatever.
However, while this person really feels like an asshole, it's incredibly mature of him to recognize his past behavior and try to improve.
For many developers, it's incredibly satisfying to "be right", and due to the fact that's it's easy to leave some comments, the humanity of their coworkers can often take a backseat.
Its probably too carte blanche; but ive reached the point where I will refuse to rule on codestyle issues (general answer; whoever did it first sets the style)
Sometimes the quite serious bugs are just hard to notice, as well. I recently reviewed a large changelist dealing with lots of multithreading and locks, and cases where locks aren't taken to avoid deadlocks. I have 0 confidence I caught all edge cases, which terrifies me for multi-threaded code.
Simplifying is easy. Early versions of this code, years ago, were simple. They weren't even multithreaded! Just a simple implementation to unblock other devs.
But now it's a highly used bottleneck that must be high throughput, low latency, support asynchronous cancellation of requests, interacts with the main thread for third party APIs that aren't thread safe (such as d3d9), interacts with the main thread for our own APIs which aren't thread safe by design (our debug replay system replays the events of the main thread), but must avoid synchronizing with the main thread for performance elsewhere...
Simplifying this without performance or feature regressions is significantly more difficult. Possible, but difficult. And benefits from slow, testable, incremental changes. But that means reviewing incremental diffs on the large and complicated existing system in the interim. At least it has some of our best test coverage, including lots of tests to help try and tease out threading bugs. They don't always succeed at that, but they do help.
Code authors who don't see the value in consistency of code are potentially a problem - if authors are submitting code reviews with hundreds of actual style issues, that's either a failure of process or the author to write readable code (not sure if that's what happened in your examples, to be clear).
Style is not consistent across authors, codebases, projects, even companies. I had to learn to read code in many different styles.
Enforcing consistency can be done with formatters/linters. It doesn't need to come up in review.
Tooling is great, but I think there are a lot of style issues that aren't readily enforceable with linters - commenting, naming, control flow, abstraction, etc.
Great developers see the opportunity to grow someone.
The former shows ownership of the code, but is generally bad for a business.
The latter understands that a successful business is more than simply his competence.
Reaching this level of realisation is critical for being a good lead; I interview loads of clearly solid programmers who havent figured this out, and sadly price themselves out of the market (I blame their previous managers for missing their growth opportunity)
I can tell you I haven't had this feeling ever, but not for reasons I'm proud of. I hate doing code reviews. I have hated doing every one. I have disliked having to type every comment I have made on a code review. Each time I hope that everything is good enough and I can just approve it. And I won't comment unless I am convinced it is important.
I don't hate the coder, or the process that requires code reviews. I know code reviews are very important for many different reasons, so I don't shirk them, but I have become afraid of being an expert on a large team as I will have to do more of them. Not sure what is wrong, I love coding. Reviewing good code isn't so bad, reviewing bad code takes all the joy out of coding for me. If it's bad enough, I know that I need to make a lot of comments, and I'll need to make some more when it comes back with changes. I think we are just wired differently.
EDIT After further thought, I think a fundamental difference is I really want to see my coworkers succeed, and don't enjoy their failure. Also I dislike all forms of toil. But I think maybe part of the authors problem is not able to comprehend that some people can be nice like that, genuinely. In fact, I assume my coworkers want me to succeed as well, and I really think most of the time they do. But I am glad he decided to start acting that way, which must be especially difficult if you don't believe others will treat you the same way.
There's great health to be gained from code reviews. But they really do need to practiced, reconfigured, and practiced again so your team gets what they need from it.
I'm in both of these camps too. I think I'd dislike code reviews if I didn't remind myself:
* Code my coworkers write is code I don't have to write, so it's saving me toil.
* Issues I catch in review are issues that won't lead to late night debugging sessions catching heisenbugs on the eve of shipping our product, so it's saving me some of the worst kinds of toil.
A little work now to save me a lot of work later. And I've been burned by suddenly getting stuck maintaining & bugfixing code that I got lax about reviewing. Each issue I catch, I think "thank goodness I caught that now - that'd be a pain in the ass to debug".
It also makes me genuinely appreciate review feedback catching all the dumb shit I might do, which can really help when your quick change turns into a slog of missed edge cases, "can you fix X while you're in there", etc.
I assumed the same was true for them. I learned from another team member to try to phrase comments as questions. "Did you mean to do X here? It seems like it might have issue Y" etc...
You ever worked on a team a where someone would go write code in their own crazy way that wouldn't follow any sort of existing pattern or take advantage of existing tooling? So they spend like a week on a simple task because they re-wrote the strings.c because they didn't want to include it? Yeah, they get fired eventually, but they are the worst to write code reviews for, because you go into it just thinking WTH, why are they doing it this way, this entire approach is convoluted and error-prone and rigid and fragile and complicated.
If a super engineer has advice for me on how to deal with this stuff, I'm all ears. I usually just ignore it unless it directly impacts me and then go back and re-write it when we have to add on to it/make it interop with some more code/release it as an available API.
Every 4 hours or so I would ask my boss if I really had to do it. He told me to just keep going, I think he was building a case for letting him go. I still only made like 10 comments, needless to say they weren't taken well. What a nightmare.
Tangential to this whole thread, but guys like that, every day he worked took two days for other devs to fix/undo his work. Took my company a few years to catch on, he was a senior dev and also good at talking himself up. I would have paid him the same salary to go sit on a beach somewhere and enjoy himself instead of touching the code, we'd all have been happier. In fact the world would be a better place, because now he is presumably working somewhere else doing the same things.
This quote made me laugh from the exaggerated "insane pleasure" in other's mistakes. But I feel this way sometimes. I'm completely sure it comes from my own insecurities, so that when I see another guy missed something I wouldn't, I get a feeling of relief - actually I'm not the worst programmer in the world.
So for me this feeling comes from impostor syndrome - I thought I was bad, I saw another guy who was worse, I felt good. Insecurity is really a self-fulfilling prophecy. I'm trying to work on it by forcing myself to ask questions I think are dumb, and not fearing judgment from others. This in turn will also influence my peers to feel less afraid of their doubts, turning the spiral around.
A study from Google [1] agrees with this: they found that the best predictor of team quality was the willingness to ask questions and express opinions without fear. Quoting Google:
"In a team with high psychological safety, teammates feel safe to take risks around their team members. They feel confident that no one on the team will embarrass or punish anyone else for admitting a mistake, asking a question, or offering a new idea."
[1] https://www.inc.com/justin-bariso/google-spent-years-studyin...
Best way of doing code review is by asking questions.
Recently coworker recommended this talk which suggest good attitude IMO - you can find it under "strong code review" from rails conf
I tend to lean this way as well. Im of the mindset that most things that are "caught" in a code review don't add any real value. Or maybe I just have bad coding standards.
Are you kidding? Call me a liar if you want, but I don't feel that way when doing code reviews, and never have.
Yes, when I was a teenager I wielded my knowledge like a titan. The meager amount of knowledge I had.
But as a professional, I have never once been mean-spirited in a code review. Everything I send back is to keep the code clean and maintainable and to help the developer improve.
Not everyone feels the need to be "right" all the time. And by that, I mean being perceived as correct, even if they aren't. For me, that was a function of actual skill.
The better I got at programming, the less I needed everyone else to acknowledge how good I was.
Even better, that applied to the rest of my life, too. I haven't felt the need to show off my intelligence or skills in a long, long time. I simply do my thing and if/when someone notices, it's a great feeling. If they don't, I still enjoyed doing it well.
As a kid? I'm sure I did. I probably even did it about code on the internet. But I wasn't a professional then and wasn't employed as a developer.
I have a done a lot of review not as much lately but my principle is pretty simple - Can this be made better to the best of my understanding and knowledge. I will give it the same attention as I will my code. When my first version of a code or the POC is done I will think of what can be improved upon with the goal of the final version to be cleaner, faster and more robust if possible. My job is to help the author and the company to make sure together we get the best version possible merged of course with an understanding of cost-benefit.
The language is of course important but not catching issues on every line if that's what I find. I will appreciate if someone does the same thing for me. At one point I have reviewed more than 50% of the codes in my company and I know how hard it can be to do so with full care so I take it as a favor when somebody does a good review on my code finds mistakes or potential improvement. I have had way junior developers giving me feedback ranging from better names to serious bugs. I have also always explained why a certain idea should be explored or might be better with the author regardless of how junior they might be. I have always liked the developers best who leaves the ego out of it both a reviewer and author of a certain piece of code. Pride and Ego I find are unrelated. I take pride in giving my best both when writing a code or reviewing one all the while knowing not just that there are other developers much better than me but even someone who is on average worse than me can still find potential improvements in my code. I like to think that I can take any comment on its merit and not my perception of the person.
I disagree that pointing out every flaw in a code review is a bad thing to do. I believe that these could be positive experiences with the right attitude from both the reviewer, the submitter, and the team's management. A few things that really bugged me while reading this:
- The reviewer feels like pointing out a flaw is an adversarial zero-sum sort of action that affirms his superiority over the submitter.
- The submitter will feel bad about each feedback given (at least in the POV of the author). If the feedback is mean-spirited, then sure, but if it's constructive, then this shouldn't be the case.
- His team fired the developer who received "too much" feedback. I'm sure there's missing context here, but if that's the main reason, then this is pretty messed up.
Finally, his conclusion to not submit the review really bugged me. I do agree that no review is better than a mean-spirited destructuve review, but IMO this is still a failure on the part of the reviewer. He should write the review in a constructive manner and work with the submitter to hash out the issues. If it's an argument, then fine, work through it... but teammates should help each other grow and be better.
In this case, by not submitting honest and comprehensive reviews, I believe his team and his product suffers in the following ways:
- The reviewer has to waste his time "cleaning up" the code later on.
- The submitter loses out on the knowledge transfer that takes place during a good code review.
- The product potentially has unfixed bugs.
- A fear of conflict is further instilled in the team, and criticism now becomes off-limits because it is considered destructive due to the negative attitudes of everyone involved. This further hurts product quality, and the result is that everyone codes in isolation out of their ivory towers instead of collaborating on a product together, among many other detrimental effects.
I think the solution at least begins with:
- A better team commitment to personal growth, and a sense of responsibility on the part of everyone to help out their teammates in this respect.
- Clear team guidelines on lint/style standards to eliminate the need to argue about it.
- An attitude that feedback is about the code and NOT about the person submitting it. Everyone should feel like an owner of the code, and all discussion should be about the code and how to improve it.
- Comprehensive code reviews. Note everything you find. Maybe call an in-person review if you feel like there's too much to comment on.
edit: typos
Instead of "Why would you ever do it this way?", try "I think I would probably do this like (example)"
Also, be liberal with complementary reviews, as well. I see a lot of pull requests where the solid, workday, well written code is just passed over, or given a ":+1:", even though it's definitely worthy of praise.
You wrote a new class extracted from an old confusing method, it has good tests, and the feature is working as requested? A triumph.
I wish I had a magic blog-post-length formula for how that happened. I don't. I know I personally talk about it in interviews, and we hire for developers who think about development as a team sport rather than a solo FPS rampage, and we talk about code review sort of a lot in onboarding.
The way I explain it to noobs is that a collegial review culture is a question of trust. When we all believe that we are all on the same team with the same goals, we feel safe, and we can trust that (1) critical feedback we receive from others is intended just to improve the code, not to score points on us personally, and (2) if we give feedback that is critical, it will be received in that spirit as well. And it's our responsibility to give critical feedback where the code under review doesn't meet our standards of efficiency, readability, and maintainability.
Grandstanding or mean-spirited reviews break down the team-wide spirit of trust and the feeling of safety; they are a far greater danger to the integrity of the code base than a badly-coded method or spotty test suite.
People that are really good at things are graceful and helpful usually.
PS-Question: Author mentions Russian culture being an influence. I feel like he's saying there is an emphasis/high value on ultra-competence ultra-stoic unshakability sort of state of being? The meme is that Russians are intense, which I fully respect.
This seems exactly backwards to me.
Disconnecting ego from the work was the first big lesson I had when I started working in software. I hear parallel ideas from friends across industries, in fact an electrical contractor explained to me how he expects it of his apprentices just a few days ago.
I once had a joint software team with a client and put my foot in my mouth assuming they separated code and ego the way we did. The first time I reviewed some of their work, there was a design decision that I sorta assumed was the best of a few bad options. So I asked the developer why they did it that way.
I just wanted to hear some reason, any at all would have done. Something special about that approach, or that they considered the others and found it was, in fact, the least worst. Then we'd just move on, happy enough with our implementation. I'm used to doing that (and having it done to me) dozens of times a month. But what I got was not a defense of the design decision, and more a defense that he made a design decision. I touched a nerve. I now know to hedge, apologize before giving feedback and ask questions by asking around them. I don't really think it's an overall plus. Especially when the business domain puts high standards on us.
100% agree. Any code you write for your employer is not your code - you should be comfortable leaving it forever tomorrow if a better opportunity presents itself elsewhere.
Until you can do that, you'll always be a lesser software developer. You'll concentrate on maintaining your ego instead of developing the best software for the problem at hand. In a way that's the gist of the article.
It's interesting, but at the company I've worked at, where we've had dozens of senior developers come and go, we've never had this problem. Our approach to code reviews is purely educational. "Here's how we do it so we have the least amount of trouble understanding each others code." Everyone seems to be good with that, and egos get left at the front stoop. I've learned lots, I've taught lots, and I've never felt better than anyone or worse than anyone. It's all about the learning.
And let me tell you, when you hire some junior dev, and he gets these code reviews, and 3 years later he's a senior dev and anchoring a project, it's a pretty good feeling. I think better than any feeling you can get from lording knowledge over others.
Reminds me of a joke-ish comment I saw on here some time ago: The difference between a junior and senior developer is the willingness to say "that bug was totally my fault".
My ego drove me to work extra hard to try and impress others. I would become petulant if I didn't get my way. I look back on the first few years of my career with so much cringe!
Agreed that the OP took the wrong lesson - the fix is not to lower standards, the fix is to learn to deliver honest and complete feedback with empathy.
I wish it was my first big lesson, it took me way to long to learn and my mental health suffered greatly until I did.
I no longer ask 'why?' inside a code review, because it immediately puts the author in the position of justifying what they did, while feeling like they have to defend a decision.
Instead, I'll take a coaching (or a more socratic) approach that doesn't risk making the author feel dumb, or like they never considered the alternatives: "what was your reason for this?", "did you try x,y,z?", "I wondered if this would work?", "what is this for?", "what does this do?"
In addition to that, if I have a suggestion (or a suqqestion [0]), I will always follow it up with a refactored, copy/pastable code snippet to make it clear what I'm talking about and also to help push the review forward. This is especially useful for learning by example.
This means that code review takes more time and effort, but that investment of time has value. Code reviews become an implicit, shared mentoring space, and people like it when they see your review pop up.
[0] https://www.urbandictionary.com/define.php?term=Suquestion
Often code reviews come in after someone has put a few weeks of work into something and its too late to change how it was done. Or a review is passed in by someone in a hurry without actually critiquing it.
* Authors are expected optimize for readability of code, even if it takes longer to get the code in.
* Reviewers should generally only start reviewing code if they are ready and willing to continue the review to completion (i.e. no drive-by reviews). Once they start reviewing they should try to minimize response time.
* Focus the reviews on testing, correctness, readability
* Consistent code style is important (see optimizing for readability)
* Break code reviews into the smallest reviewable units. Often these are somewhat large because parts of the change don't make sense in isolation
* The code author is responsible for writing code in a way that it convinces the reviewer that it is correct (see optimizing for readability)
* You should approach it collaboratively - you're working together to get the work done and make the codebase as maintainable as possible
* You should aim to leave each bit of the codebase that you touch in at least a good a state as it was previously
Articles complaining about the reality of code reviews seem to be commonplace. But if they're not working out, why not just abandon them, maybe try something different?
The one alternative that (some) code-review advocates seem willing to accept is XP-style full-time pair programming. But that leaves even less room for individuality and solo accomplishment.
Dollar for dollar, code reviews are more effective at finding and fixing bugs than any other activity we can do, including QA tests. However code reviews are very hard on people, and can easily create conflict.
Which one matters more, and how careful people are, varies widely by organization.
Comments on pull requests are not on the same level and those findings shouldn't be hastily generalised.
I'd rather have ten mediocre developers who can be part of a team than one gifted diva.