7 ms·

Code duplication sometimes is a good thing but it doesn't prove that clean code is a bad thing. I think you overcommitted a bit on the refactor, if you had rep

Code duplication sometimes is a good thing but it doesn't prove that clean code is a bad thing. I think you overcommitted a bit on the refactor, if you had replaced those "10 repetitive lines of math" with a function it would have been cleaner. Let's be crystal clear about it, your teammate did a terrible job not creating a function for those 10 repetitive lines. I would be rejecting this PR but never rewriting it. Rewriting a PR is a quick way to insult someone because you're not open to debate and many times you don't have the full picture of why the code was written this way. PRs have to be reviewed and rejected when there are no coding standards, the last thing I want is people committing code and saying "Clean code is not important", this mindset only leads to a codebase that no one wants to work within a matter of months. Communicating better and having some tact is the lesson you should be taking from your experience, clean code has nothing to do with it.

blowski 3y ago
Absolutely. The lesson is "goodbye to over-applying a rule without considering context-specific trade-offs", which is a lesson that takes time to learn.
- rewmie 3y ago
  > The lesson is "goodbye to over-applying a rule without considering context-specific trade-offs", I think the problem is the rule itself. A rule of thumb is fundamentally broken if it does more harm than good. The heuristics to drive a decision need to lead the developer to a good outcome if they apply it without overthinking things or have meetings, because that's what rules of thumb are for. Time and again developers tie codebases into contrived knots by mindlessly repeating the mantra "duplicate code is bad code" because they fail to realize that code that looks the same is not the same code at all, and sometimes should not be the same code at all for a number of reasons. Developers need to think hard about whether a refactorization helps simplify a project, and shoving code sections into multiple unrelated code paths is surely a way to do the exact opposite. Moreso when a mastermind developer opts to add conditionals to force similar code blocks to fit other code paths.
59nadir 3y ago
> Let's be crystal clear about it, your teammate did a terrible job not creating a function for those 10 repetitive lines. I've found over a long time (20+ years) that this is usually a sentiment held by people who are focusing on the wrong things in code bases (and quite often aren't actually solving real problems but spend their time solving non-problems with the additional side effect of creating more for the future). The first implementation of this should most definitely not abstract away anything like those 10 lines (which is minuscule). It's trivial to take something that does exactly (and only) the thing and modify it later and it's pointless to abstract away something as small as 10 lines for a gain you haven't yet proven or tested. "Clean Code" is absolutely not important and most rules/"principles" of the same character as those that make up Clean Code are absolutely not important either, but are things that people with nothing better to do hold on to in order to validate the hornets nests they accumulate in code bases over time. It leads to over-abstracted, hard-to-change code that runs badly and is much harder to understand, generally speaking. The only thing you should use as a guiding principle, if anything, is to express your data transformations in a clear and easily changed way, since that's the only real thing a program actually does. If something doesn't have to do with improving the expression of data transformation it's very likely it's bullshit that's been stacked on top of what you're doing for no real reason. Most of the SOLID principles have nothing to do with or in fact make it harder to see what data transformations take place and how, which is why they are largely useless for understanding what is actually going on in a program.
- all_factz 3y ago
  So much this. It’s hard not to lose the forest for the trees — we are craftspeople after all — but at the end of the day the overall structure of a program is so much important than whether there’s a bit of duplication here or there. Better to let the structure emerge and then reduce duplication instead of trying to guess the right structure up front. And yeah I’m still not convinced SOLID is real (but I’m also not convinced classes are useful much of the time, for that matter).
  - zelphirkalt 3y ago
    Letting the structure emerge requires people thinking in depth about the underlying principles of what the code does or should do. As for classes: They are merely a construct in many languages, that people have come up with for organizing code and in my opinion a very debatable one. Some newer languages don't even deal in classes at all (Rust for example) and with good reason. If one says we need classes for having objects—No we don't. And objects are a concept to manage state over the lifetime of what the object represents, so that might be a worthy concept, but a class? I mostly find classes being used as a kind of modules, not actually doing anything but grouping functionality, that could simply be expressed by writing ... functions ... in a module, a construct for grouping that functionality. I think what many people actually want is modularity, which in contrast to classes is a concept, that truly seems to be well accepted and almost every language tries to offer it in some way or another. It is just that many people don't realize, that this is what they are chasing after, when they write class after class in some mainstream language, that possibly does not even provide a good module system.
aidos 3y ago
To be fair, he does say that he took that away as a lesson. He listed it first, so I’m charitably going to call it the “primary lesson”
roenxi 3y ago
> Rewriting a PR is a quick way to insult someone ... Although I agree with this there is also another, more subtle thing going on. Rewriting the code that works and someone else is maintaining is a waste of the rewriter's time and is unprofessional. It also denies the original person an opportunity to learn because they don't see how lower quality code wastes time in practice (the time wasted in the refactor doesn't count because Abramov did it by initiative). The clean code approach is better, but the issue here isn't code. It is that he was wasting his time while not providing useful feedback to the unnamed coder. He was making terrible choices. He isn't maximising the business or team outcomes. The best outcome is the original coder raising their standards and him going and working on something that needs work. He should have angled to that. Ie, the correct option was to do a code review.
- zelphirkalt 3y ago
  > Although I agree with this there is also another, more subtle thing going on. Rewriting the code that works and someone else is maintaining is a waste of the rewriter's time and is unprofessional. Rewriting existing functioning code is not a waste of time in general. We don't know the full picture, so one cannot make such a generalized statement. I can write perfectly functioning code in the most terrible way that makes maintaining the code a herculean task. Very easily so even. Making things worse is almost always easy. Rewriting that code might make maintenance much easier, even if the code was previously working. It might make onboarding easier, since others can better understand what is going on. It is not in general unprofessional. To judge that, you need a way more complete picture. It can even be unprofessional to leave horrible code as is, instead of fixing it, leaving future people to plaster over it with more leaky abstraction.
  - FrustratedMonky 3y ago
    But, without talking to the other person is the problem. This made it sound like it was checked in, and this guy jumped in and re-wrote it the very same week without talking to the other person. That is probably the biggest problem here. So, there was a project happening, one person did a section, someone else jumped in and re-did it (double work), without talking about it (risk another future triple re-write).
j-bos 3y ago
> I think you overcommitted a bit on the refactor, Source domain checks out
indymike 3y ago
> Rewriting a PR is a quick way to insult someone Wow. Just review the review and if it's good, merge it, resubmit to a different reviewer, whatever your process is. The reviewer/re-writer is helping get work done and being offended is counter-productive.
- cjaybo 3y ago
  What the GP explained is commonly true when it comes to interacting with other humans, with only a few exceptions. You can complain about human nature being counter productive all you want, but refusing to adapt to this reality and foregoing fundamental soft skills is, ironically, even more counter productive.
  - indymike 3y ago
    > What the GP explained is commonly true when it comes to interacting with other humans, Not really. It's company culture. It does not have to be that way at all, and is honestly a symptom of poor leadership and a misunderstanding of roles on a team. > foregoing fundamental soft skills is, ironically, even more counter productive. I disagree with having a culture that makes people afraid to submit a pull request because someone might help out and fix a problem with it, or make it better. Being angry comes from misunderstanding and fear that a developer's standing will be diminished because of the rewrite. A developer being angered by a correct or better rewrite is a symptom of bad leadership and a toxic culture.
toss1 3y ago
>>many times you don't have the full picture of why the code was written this way. Chesterton's Fence [0] [0] https://fs.blog/chestertons-fence/ https://fs.blog/chestertons-fence/
simiones 3y ago
I think the original blog post is badly worded. We can see by the proposed refactor that in fact those 10 lines were not identical - they were only similar. The formulas for resizing the top-left corner of a rectangle are different from the formulas for modifying the bottom-right of an oval. They look quite similar, but one will have a + instead of a minus here and another one there etc. The complexity is built into geometry itself in this case, and "abstracting" it away is only moving the mess around, it fundamentally can't (perhaps there is some clever mathematical abstraction that could using some special number group, but unless you have that built-in, it'll probably be much harder to implement the special arithmetic using built-in ints than you gain).
angarg12 3y ago
Assuming this story is real, there are several smells * Colleague writes quite a bit of code, and merges it without anyone reviewing it or providing feedback. * OP rewrites the code from a colleague without communicating, and again merges it without a review. * The manager calls a meeting with OP and ask them to revert their changes. The initial code might have been messy or not, and the refactor might have been a bad or good idea. Nevertheless I think OP is taking away the wrong lesson.

blowski 3y ago

Absolutely. The lesson is "goodbye to over-applying a rule without considering context-specific trade-offs", which is a lesson that takes time to learn.

rewmie 3y ago
> The lesson is "goodbye to over-applying a rule without considering context-specific trade-offs", I think the problem is the rule itself. A rule of thumb is fundamentally broken if it does more harm than good. The heuristics to drive a decision need to lead the developer to a good outcome if they apply it without overthinking things or have meetings, because that's what rules of thumb are for. Time and again developers tie codebases into contrived knots by mindlessly repeating the mantra "duplicate code is bad code" because they fail to realize that code that looks the same is not the same code at all, and sometimes should not be the same code at all for a number of reasons. Developers need to think hard about whether a refactorization helps simplify a project, and shoving code sections into multiple unrelated code paths is surely a way to do the exact opposite. Moreso when a mastermind developer opts to add conditionals to force similar code blocks to fit other code paths.

59nadir 3y ago

> Let's be crystal clear about it, your teammate did a terrible job not creating a function for those 10 repetitive lines. I've found over a long time (20+ years) that this is usually a sentiment held by people who are focusing on the wrong things in code bases (and quite often aren't actually solving real problems but spend their time solving non-problems with the additional side effect of creating more for the future). The first implementation of this should most definitely not abstract away anything like those 10 lines (which is minuscule). It's trivial to take something that does exactly (and only) the thing and modify it later and it's pointless to abstract away something as small as 10 lines for a gain you haven't yet proven or tested. "Clean Code" is absolutely not important and most rules/"principles" of the same character as those that make up Clean Code are absolutely not important either, but are things that people with nothing better to do hold on to in order to validate the hornets nests they accumulate in code bases over time. It leads to over-abstracted, hard-to-change code that runs badly and is much harder to understand, generally speaking. The only thing you should use as a guiding principle, if anything, is to express your data transformations in a clear and easily changed way, since that's the only real thing a program actually does. If something doesn't have to do with improving the expression of data transformation it's very likely it's bullshit that's been stacked on top of what you're doing for no real reason. Most of the SOLID principles have nothing to do with or in fact make it harder to see what data transformations take place and how, which is why they are largely useless for understanding what is actually going on in a program.

all_factz 3y ago
So much this. It’s hard not to lose the forest for the trees — we are craftspeople after all — but at the end of the day the overall structure of a program is so much important than whether there’s a bit of duplication here or there. Better to let the structure emerge and then reduce duplication instead of trying to guess the right structure up front. And yeah I’m still not convinced SOLID is real (but I’m also not convinced classes are useful much of the time, for that matter).
- zelphirkalt 3y ago
  Letting the structure emerge requires people thinking in depth about the underlying principles of what the code does or should do. As for classes: They are merely a construct in many languages, that people have come up with for organizing code and in my opinion a very debatable one. Some newer languages don't even deal in classes at all (Rust for example) and with good reason. If one says we need classes for having objects—No we don't. And objects are a concept to manage state over the lifetime of what the object represents, so that might be a worthy concept, but a class? I mostly find classes being used as a kind of modules, not actually doing anything but grouping functionality, that could simply be expressed by writing ... functions ... in a module, a construct for grouping that functionality. I think what many people actually want is modularity, which in contrast to classes is a concept, that truly seems to be well accepted and almost every language tries to offer it in some way or another. It is just that many people don't realize, that this is what they are chasing after, when they write class after class in some mainstream language, that possibly does not even provide a good module system.

aidos 3y ago

To be fair, he does say that he took that away as a lesson. He listed it first, so I’m charitably going to call it the “primary lesson”

roenxi 3y ago

> Rewriting a PR is a quick way to insult someone ... Although I agree with this there is also another, more subtle thing going on. Rewriting the code that works and someone else is maintaining is a waste of the rewriter's time and is unprofessional. It also denies the original person an opportunity to learn because they don't see how lower quality code wastes time in practice (the time wasted in the refactor doesn't count because Abramov did it by initiative). The clean code approach is better, but the issue here isn't code. It is that he was wasting his time while not providing useful feedback to the unnamed coder. He was making terrible choices. He isn't maximising the business or team outcomes. The best outcome is the original coder raising their standards and him going and working on something that needs work. He should have angled to that. Ie, the correct option was to do a code review.

zelphirkalt 3y ago
> Although I agree with this there is also another, more subtle thing going on. Rewriting the code that works and someone else is maintaining is a waste of the rewriter's time and is unprofessional. Rewriting existing functioning code is not a waste of time in general. We don't know the full picture, so one cannot make such a generalized statement. I can write perfectly functioning code in the most terrible way that makes maintaining the code a herculean task. Very easily so even. Making things worse is almost always easy. Rewriting that code might make maintenance much easier, even if the code was previously working. It might make onboarding easier, since others can better understand what is going on. It is not in general unprofessional. To judge that, you need a way more complete picture. It can even be unprofessional to leave horrible code as is, instead of fixing it, leaving future people to plaster over it with more leaky abstraction.
- FrustratedMonky 3y ago
  But, without talking to the other person is the problem. This made it sound like it was checked in, and this guy jumped in and re-wrote it the very same week without talking to the other person. That is probably the biggest problem here. So, there was a project happening, one person did a section, someone else jumped in and re-did it (double work), without talking about it (risk another future triple re-write).

j-bos 3y ago

> I think you overcommitted a bit on the refactor, Source domain checks out

indymike 3y ago

> Rewriting a PR is a quick way to insult someone Wow. Just review the review and if it's good, merge it, resubmit to a different reviewer, whatever your process is. The reviewer/re-writer is helping get work done and being offended is counter-productive.

cjaybo 3y ago
What the GP explained is commonly true when it comes to interacting with other humans, with only a few exceptions. You can complain about human nature being counter productive all you want, but refusing to adapt to this reality and foregoing fundamental soft skills is, ironically, even more counter productive.
- indymike 3y ago
  > What the GP explained is commonly true when it comes to interacting with other humans, Not really. It's company culture. It does not have to be that way at all, and is honestly a symptom of poor leadership and a misunderstanding of roles on a team. > foregoing fundamental soft skills is, ironically, even more counter productive. I disagree with having a culture that makes people afraid to submit a pull request because someone might help out and fix a problem with it, or make it better. Being angry comes from misunderstanding and fear that a developer's standing will be diminished because of the rewrite. A developer being angered by a correct or better rewrite is a symptom of bad leadership and a toxic culture.

toss1 3y ago

>>many times you don't have the full picture of why the code was written this way. Chesterton's Fence [0] [0] https://fs.blog/chestertons-fence/ https://fs.blog/chestertons-fence/

simiones 3y ago

I think the original blog post is badly worded. We can see by the proposed refactor that in fact those 10 lines were not identical - they were only similar. The formulas for resizing the top-left corner of a rectangle are different from the formulas for modifying the bottom-right of an oval. They look quite similar, but one will have a + instead of a minus here and another one there etc. The complexity is built into geometry itself in this case, and "abstracting" it away is only moving the mess around, it fundamentally can't (perhaps there is some clever mathematical abstraction that could using some special number group, but unless you have that built-in, it'll probably be much harder to implement the special arithmetic using built-in ints than you gain).

angarg12 3y ago

Assuming this story is real, there are several smells * Colleague writes quite a bit of code, and merges it without anyone reviewing it or providing feedback. * OP rewrites the code from a colleague without communicating, and again merges it without a review. * The manager calls a meeting with OP and ask them to revert their changes. The initial code might have been messy or not, and the refactor might have been a bad or good idea. Nevertheless I think OP is taking away the wrong lesson.