I think code review is very important, especially as a learning mechanism. It’s a great way to disseminate knowledge of a code base. But I find the advice here to be dangerous for a sane working environment.
The sponsor is not paying you to make friends in the office.
Strictly speaking, this is true, but it doesn’t mean you shouldn’t be friendly. “Professional”, a word used frequently in this piece, doesn’t mean you are callous in your comments. It does mean being honest, but honestly like “This sucks” isn’t constructive most of the time. I agree with the “No Fear” section in principle, but I’m not keen on the description of how to be fearless.
The worst thing (as in any conflict resolution) is a compromise.
This is just outright terrible advice and demonstrably false. There’s a lot of nuance in most code bases and saying “there is no other way than mine” is a recipe for a toxic work environment. I wouldn’t call that professional. (And the generalization to “any conflict resolution” is an invitation to a real or metaphorical war, which doesn’t help anything.)
Again, you fearlessly said that a method should be designed differently. Your opponent, the author of the code, replies that he doesn’t think so. [emphasis added]
Refering to the author as an opponent, again, doesn’t set the stage for any kind of helpfulness. The OP then goes on to talk about teaching and educating the code author, which is much better. It’s consistent with the oscillations in tone of the piece.
No matter how bad the code is and how stubborn your opponent is, you must remain professional. To be honest, I find this very difficult sometimes.
May I suggest that this difficulty might stem from the mindset of considering code reviews as some sort of adversarial exercise?
I don’t see a difference between those two statements. In both cases you reject the basic concept of inspiration and are claiming that learning from mutliple designs is “less pure” than copying.
I’m absolutely not rejecting the concept of learning from multiple designs, but that’s not what a compromise is.
A compromise is two (or more) parties agreeing to only get part of what they want.
This helps team cohesion (if you are stuck with coworkers who cannot deal with not getting their way at all).
I don’t see a difference between [‘no compromise’ and ‘no way but mine’]
I’m OK with doing things your way (even if I think mine is better). I am not OK with trying to keep us both happy by frankenstein-ing the two approaches together.
IME, most approaches to structuring code comprise several (mutually supportive) sub-strategies.
As a result, taking one part from my approach and one part from yours usually results in a
structure which lacks internal coherence.
I didn’t really mean to write my comment in an accusatory tone. I was using “you” more generically, and your definition of compromise is a very specific one. If that’s the definition the author intended as well, it’s fine, but in general compromises include both your and my definition. The only difference in those is the quality of the outcome.
Precisely this. Code review is impossible to get “right” when you have culture problems like folks wanting to help their friends get ahead (turning a blind eye to messy but working code) and see others fall behind (and nitpick senseless details). In almost every pre-commit review productive team I have been on, folks make little alliances to review quickly and honestly and ship each other’s code.
I was just discussing this with a coworker the other day, and I’ll also come down on pretty much the opposite site of the “no compromise” thing.
When I was a junior, I had a lead who loved to tell us about how he knew what he was doing because he used to work for NASA, and NASA makes the most reliable code on the planet. Come code review time, he would have feedback on everything, but mostly minutiae. And the thing about arguing things like variable names in code review is that usually, no one is definitively right. Which, to him, meant that he was right because he was senior, so the only way to get out of a review session was to agree to whatever changes he wanted.
Once I found myself doing his job, I took a different approach. My theory is that if I think your code is broken, has an important flaw, or violates one of our written guidelines, then yes, this discussion is going to go on until either I make you understand how to fix it, or you convince me that I’m wrong. But if I find something that I think is simply not done the best way, then I’m going to point it out, but I’m also going to be ready to let it go.
Maybe you agree with me that my way is better, and you just didn’t see it before. That’s good, hopefully we get that outcome a lot. Maybe you point out your own reason for doing it that way, and I decide yup, that’s valid. That’s okay too. Maybe your argument doesn’t convince me, and I’ll argue back… but only once. More than two rounds of back-and-forth over something non-critical is a waste of your time and mine. Plus, if I badger you into the ground this time, who’s to say you won’t bother raising a valid argument next time?
This article likes to call the reviewer and the reviewee “opponents”, but in my experience, the only way for code review to work well is to keep in mind that you’re actually the exact opposite of that. At least in the ideal world, you’re coming together because you share goals — write good code, make the customer happy, avoid having to fix bugs and maintain shitty code later on, etc. Shooting down someone’s bad code isn’t attacking them, it’s doing them a service. If you can’t agree on that, then say goodbye to productivity.
When I was a junior, I had a lead who loved to tell us about how he knew what he was doing because he used to work for NASA, and NASA makes the most reliable code on the planet.
The article actually explicitly proclaims that reviews should be substantive, and not based on “qualifications”: «Be ready to show links, articles, books, reports, examples, etc. Just “I know this because I’ve been writing Java for 15 years” is not enough. Moreover, this type of argument only makes you less convincing.»
This article likes to call the reviewer and the reviewee “opponents”
I think it’s merely a figure of speech based on the cultural linguistic background of the author, although I might be wrong. (Funny story: I once identified a fellow countryman at an academic conference in Vegas not by his accent (he’s been American for years), but by his figure of speech, which was very unusual, but at the same time all too familiar.)
He also maintains a list of testimonials on his website, so be careful what you say, at least if you don’t want to be mentioned there.
Maybe he’s some kind of troll, who knows.
Completely unrelated, but I really wish that people would stop using this kind of phrase. It’s simultaneously the most condescending and the most vapid thing that I hear. It sounds like you’re speaking to a stupid child, and it seems like it never comes with a good argument.
Maybe the idea is that given enough time, there are tribal warlords even in “flat” organizations? Sometimes they aren’t easily visible but if you look closely, you’ll find them (or create them?)
It’s odd how I can find myself agreeing with the four high-level, bold subtitles, and yet I completely disagree with the author’s reasoning and examples of each.
It seems like the author may be coming from a very specific development environment that involves a very specific structure in project/client management (“sponsor is not paying you to make friends in the office”), with frequently fluctuating teams (“hire a few new people every week”), and a very rigid corporate/team hierarchy (“In every project, there is a software architect who makes final decisions”). To that end, I just simply can’t imagine this piece of content resonating with many outside of this specific experience.
There are two ways to conduct a code review. one would be to see code review as gatekeeping (i.e. keeping harmful changes out of the codebase), the other one would be to forster discussion about code, code quality and a shared understanding of the code base and conding standards.
The first attitude I see quite often in OSS projects, and it is a useful mindset if two conditions are met: The code base has a high overall quality that one wouldn’t want to jeapordize, and (2) there is a high influx on contributions, i.e. the maintainers don’t have much time to review each contribution, but they also aren’t depending on this one contributor to stick to contributing.
Much more valuable I think is the cooperative mindset however, and especially in a company setting where the number of contributors to a project are < 50 it is the only way to go. Code autoformatters help to reduce the nitpicking, so one can concentrate on the important review aspects. Is it clean code, is the code sufficiently tested, how could tests be improved, are functions, classes and methods named appropriately, is the code clear to a programmer who hasnät developed it themselves, etc.
I think code review is very important, especially as a learning mechanism. It’s a great way to disseminate knowledge of a code base. But I find the advice here to be dangerous for a sane working environment.
Strictly speaking, this is true, but it doesn’t mean you shouldn’t be friendly. “Professional”, a word used frequently in this piece, doesn’t mean you are callous in your comments. It does mean being honest, but honestly like “This sucks” isn’t constructive most of the time. I agree with the “No Fear” section in principle, but I’m not keen on the description of how to be fearless.
This is just outright terrible advice and demonstrably false. There’s a lot of nuance in most code bases and saying “there is no other way than mine” is a recipe for a toxic work environment. I wouldn’t call that professional. (And the generalization to “any conflict resolution” is an invitation to a real or metaphorical war, which doesn’t help anything.)
Refering to the author as an opponent, again, doesn’t set the stage for any kind of helpfulness. The OP then goes on to talk about teaching and educating the code author, which is much better. It’s consistent with the oscillations in tone of the piece.
May I suggest that this difficulty might stem from the mindset of considering code reviews as some sort of adversarial exercise?
The author didn’t say “No way but mine”, they said don’t mash the two ideas together and hope the result is functional. I’d agree with that.
I don’t see a difference between those two statements. In both cases you reject the basic concept of inspiration and are claiming that learning from mutliple designs is “less pure” than copying.
I’m absolutely not rejecting the concept of learning from multiple designs, but that’s not what a compromise is. A compromise is two (or more) parties agreeing to only get part of what they want.
This helps team cohesion (if you are stuck with coworkers who cannot deal with not getting their way at all).
I’m OK with doing things your way (even if I think mine is better). I am not OK with trying to keep us both happy by frankenstein-ing the two approaches together.
IME, most approaches to structuring code comprise several (mutually supportive) sub-strategies.
As a result, taking one part from my approach and one part from yours usually results in a structure which lacks internal coherence.
I didn’t really mean to write my comment in an accusatory tone. I was using “you” more generically, and your definition of compromise is a very specific one. If that’s the definition the author intended as well, it’s fine, but in general compromises include both your and my definition. The only difference in those is the quality of the outcome.
Anyone who refers to their own teammates as their “opponent” must be an absolute pleasure to work with…
Precisely this. Code review is impossible to get “right” when you have culture problems like folks wanting to help their friends get ahead (turning a blind eye to messy but working code) and see others fall behind (and nitpick senseless details). In almost every pre-commit review productive team I have been on, folks make little alliances to review quickly and honestly and ship each other’s code.
I was just discussing this with a coworker the other day, and I’ll also come down on pretty much the opposite site of the “no compromise” thing.
When I was a junior, I had a lead who loved to tell us about how he knew what he was doing because he used to work for NASA, and NASA makes the most reliable code on the planet. Come code review time, he would have feedback on everything, but mostly minutiae. And the thing about arguing things like variable names in code review is that usually, no one is definitively right. Which, to him, meant that he was right because he was senior, so the only way to get out of a review session was to agree to whatever changes he wanted.
Once I found myself doing his job, I took a different approach. My theory is that if I think your code is broken, has an important flaw, or violates one of our written guidelines, then yes, this discussion is going to go on until either I make you understand how to fix it, or you convince me that I’m wrong. But if I find something that I think is simply not done the best way, then I’m going to point it out, but I’m also going to be ready to let it go.
Maybe you agree with me that my way is better, and you just didn’t see it before. That’s good, hopefully we get that outcome a lot. Maybe you point out your own reason for doing it that way, and I decide yup, that’s valid. That’s okay too. Maybe your argument doesn’t convince me, and I’ll argue back… but only once. More than two rounds of back-and-forth over something non-critical is a waste of your time and mine. Plus, if I badger you into the ground this time, who’s to say you won’t bother raising a valid argument next time?
This article likes to call the reviewer and the reviewee “opponents”, but in my experience, the only way for code review to work well is to keep in mind that you’re actually the exact opposite of that. At least in the ideal world, you’re coming together because you share goals — write good code, make the customer happy, avoid having to fix bugs and maintain shitty code later on, etc. Shooting down someone’s bad code isn’t attacking them, it’s doing them a service. If you can’t agree on that, then say goodbye to productivity.
The article actually explicitly proclaims that reviews should be substantive, and not based on “qualifications”: «Be ready to show links, articles, books, reports, examples, etc. Just “I know this because I’ve been writing Java for 15 years” is not enough. Moreover, this type of argument only makes you less convincing.»
I think it’s merely a figure of speech based on the cultural linguistic background of the author, although I might be wrong. (Funny story: I once identified a fellow countryman at an academic conference in Vegas not by his accent (he’s been American for years), but by his figure of speech, which was very unusual, but at the same time all too familiar.)
As for whether or not a compromise should be sought, he has another great article on that subject, which goes further into what he means by suggesting that a compromise is universally a bad idea: http://www.yegor256.com/2017/01/03/how-much-you-love-conflicts.html.
However, nowhere is it suggested (or even implied) that trivial nitpicking has to take any place at any point.
I’d recommend taking anything Yegor Bugayenko says with a grain of salt, as given the responses on lobste.rs to the submissions of his writings.
Personally, I think he’s nuts. This article is a good example.
He also maintains a list of testimonials on his website, so be careful what you say, at least if you don’t want to be mentioned there. Maybe he’s some kind of troll, who knows.
Ha! The AMP redirect for that page 404s!
Oh, honey, no.
Completely unrelated, but I really wish that people would stop using this kind of phrase. It’s simultaneously the most condescending and the most vapid thing that I hear. It sounds like you’re speaking to a stupid child, and it seems like it never comes with a good argument.
Well, it sounds like I’ve hit precisely the rhetorical target I was aiming for. Best of luck finding a good argument.
You were trying to sound pointlessly condescending, with nothing worthwhile to say? If you want, I guess that’s your right.
Maybe the idea is that given enough time, there are tribal warlords even in “flat” organizations? Sometimes they aren’t easily visible but if you look closely, you’ll find them (or create them?)
It’s odd how I can find myself agreeing with the four high-level, bold subtitles, and yet I completely disagree with the author’s reasoning and examples of each.
It seems like the author may be coming from a very specific development environment that involves a very specific structure in project/client management (“sponsor is not paying you to make friends in the office”), with frequently fluctuating teams (“hire a few new people every week”), and a very rigid corporate/team hierarchy (“In every project, there is a software architect who makes final decisions”). To that end, I just simply can’t imagine this piece of content resonating with many outside of this specific experience.
This is maye the worst article on code review I have ever read.
to elaborate my blant rejection.
There are two ways to conduct a code review. one would be to see code review as gatekeeping (i.e. keeping harmful changes out of the codebase), the other one would be to forster discussion about code, code quality and a shared understanding of the code base and conding standards.
The first attitude I see quite often in OSS projects, and it is a useful mindset if two conditions are met: The code base has a high overall quality that one wouldn’t want to jeapordize, and (2) there is a high influx on contributions, i.e. the maintainers don’t have much time to review each contribution, but they also aren’t depending on this one contributor to stick to contributing.
Much more valuable I think is the cooperative mindset however, and especially in a company setting where the number of contributors to a project are < 50 it is the only way to go. Code autoformatters help to reduce the nitpicking, so one can concentrate on the important review aspects. Is it clean code, is the code sufficiently tested, how could tests be improved, are functions, classes and methods named appropriately, is the code clear to a programmer who hasnät developed it themselves, etc.
I wish lobste.rs supported domain based filtering, so I could hide anything written by this clown or by Uncle Bob.