4 ms·

Your colleague wrote lots of code using copy and paste. You refactored it post commit. He complained to your boss, who had a go at you, and now you're going to

Your colleague wrote lots of code using copy and paste. You refactored it post commit. He complained to your boss, who had a go at you, and now you're going to leave mess in the codebase next time. There's a lesson to be taken from this experience but it isn't that copy&paste is better than writing functions. Your refactor should have gone up as a change to be reviewed, tagging the author. However the escalation to boss who tells you to revert is a really bad sign, as opposed to your colleague talking to you directly.

lakpan 3y ago
> tagging the author Sounds like trouble to me. “Hey I rewrote what you wrote because it sucked. Got problems with that?” I suppose the best solution to this problem would be a “late review” asking for an explanation, offering a solution, and hoping it doesn’t just get ignored, which is the easiest thing to do for the author.
- viraptor 3y ago
  > Hey I rewrote what you wrote because it sucked. Got problems with that? Reading it like that is just insecurity speaking. If anyone wants to rewrite my code to be more readable, I'd love for them to tag me in the review and show me how they did it, so I can learn. Assuming good intentions and being open to discussion goes a long way :) I'll always tag the author for review if I'm rewriting something recent. It's more "Hey, I was too late to mention this in the review, but I think this improvement was worth the time. Let me know what you think in case I missed something important about the original."
  - lakpan 3y ago
    > I'd love for them to tag me That’s generally what I like too, but often in teams my suggestions are either approved with zero comments, ignored or followed by wasteful reviews by people who don’t know the language they use every day, and don’t bother to google their answers.
- oytis 3y ago
  I don't think a minor deduplication justifies a separate discussion even before a single line is changed. That would make a huge deal out of it and waste everyone's time. I wouldn't probably make a separate PR for that - just refactor the code next time I need to touch it - and tag the author for review of course.
- JonChesterfield 3y ago
  It's a context thing. The point of adding the author as reviewer is to keep them in the loop, show an alternative solution and discuss which way to go. Also to learn why the did it the copy/paste route in the first place - it might be carefully reasoned, it might be a rush job, they might not know any alternatives.
  - developer93 3y ago
    Also to put the conversation on record somewhere so that if someone comes later with the same complaint they can see why the code was written like that.
- karmakaze 3y ago
  I wouldn't necessarily tag the author for a review. I might ping them to notify that I was doing some refactoring in the area and leave it at that. Of course it all depends on the team size, diversity/range of work, typical interactions between members, etc.
- sumtechguy 3y ago
  If someone had just summerly re-written my code I would personally go talk to them and found out 'whats up'. As it shows a communications breakdown. Second the person who did it should have approached the other dev and said 'hey I see you have blah why? would not this way be better?'. This happens many times in codebases. People begin to 'own' sections of it and they become very protective of what is going on in their area. They take it very personally if something changes and they were not in on it.
benterix 3y ago
I guess everybody has a different way of doing things. Personally I'd ask my colleague on Slack (privately), "Hey, you repeat 10 lines of math in each of these functions, wouldn't it make sense to make it a separate function?". The two probable answers are: "No, because... (reason)" or, less likely, "You're right, I'll do it tomorrow". No fuss, no problems for anyone, and someone can learn a thing or two.
- moistoreos 3y ago
  It also doesn't waste an entire evening obsessing over a refactor. A lesson I've learned to do with refactoring my own code.
Waterluvian 3y ago
It all seems like a sign of social dysfunction to me. That this blog post lacks most of the important details that have nothing to do with the code.
berkes 3y ago
If it is copy-paste out of laziness, then certainly "writing functions" (abstractions) would be the better solutions for repetition (DRY). But the author adds crucial context: > My code traded the ability to change requirements for reduced duplication, and it was not a good trade. For example, we later needed many special cases and behaviors for different handles on different shapes. It shows the difference between *accidental DRY* and *inherent DRY*. The context makes it clear that what we saw initially was *inherent*. Quite often, code is the same because the underlying libs A and B are consistent with eachother. Or business needs state that something has to be 15 items large, and another thing also has to be 15 items large. And so on. But when we look closely, the libs' consistency is really just chance: our wrapper around lib A and a wrapper around lib B may contain the same code, but they are wrapping something entirely different and abstracting that makes the result worse. Same with the "15 items". When business states "every list must always show 15 items", then sure, we should abstract the 15 somehow. But if it says "the top 15 on the homepage shows 15 songs" and "the comments under a song, by default show 15 items" then abstracting that 15 is a worse option.
- Tainnor 3y ago
  Without knowing too much about the context, it probably would have been possible to keep the structure where every shape is structurally independent from other shapes, but where calculations are abstracted away in pure functions that are just being called by the shapes. That keeps the maths in one place (and you can always add more calculation functions later if needed), but it doesn't entangle different shapes with each other needlessly.
JohnAaronNelson 3y ago
There was a significant bit of the article at the end where the author discussed future changes to the codebase that requires special casing handles for different shapes, how it would have been difficult with the refactor but simple with the repetitive code. I think that’s the critical bit is that you have to realize that the code will change in the future. Requirements will change, related code will change. If you make abstractions that restrict that ability to change, you’re hurting future you’s ability to adapt the codebase. More so because we often hold on, emotionally, to these abstractions we’ve invested so much time and effort into that we waste more time trying to elegantly fit the changes into our elegant solution. KISS
ainiriand 3y ago
For me the biggest red flag in all of this is changing a coworker submission without any kind of communication. Second red flag is that the refactor is not a good refactor, it lacks modularity and is still coupled, yes, you reduced some duplication but that's mostly it.

lakpan 3y ago

> tagging the author Sounds like trouble to me. “Hey I rewrote what you wrote because it sucked. Got problems with that?” I suppose the best solution to this problem would be a “late review” asking for an explanation, offering a solution, and hoping it doesn’t just get ignored, which is the easiest thing to do for the author.

viraptor 3y ago
> Hey I rewrote what you wrote because it sucked. Got problems with that? Reading it like that is just insecurity speaking. If anyone wants to rewrite my code to be more readable, I'd love for them to tag me in the review and show me how they did it, so I can learn. Assuming good intentions and being open to discussion goes a long way :) I'll always tag the author for review if I'm rewriting something recent. It's more "Hey, I was too late to mention this in the review, but I think this improvement was worth the time. Let me know what you think in case I missed something important about the original."
- lakpan 3y ago
  > I'd love for them to tag me That’s generally what I like too, but often in teams my suggestions are either approved with zero comments, ignored or followed by wasteful reviews by people who don’t know the language they use every day, and don’t bother to google their answers.
oytis 3y ago
I don't think a minor deduplication justifies a separate discussion even before a single line is changed. That would make a huge deal out of it and waste everyone's time. I wouldn't probably make a separate PR for that - just refactor the code next time I need to touch it - and tag the author for review of course.
JonChesterfield 3y ago
It's a context thing. The point of adding the author as reviewer is to keep them in the loop, show an alternative solution and discuss which way to go. Also to learn why the did it the copy/paste route in the first place - it might be carefully reasoned, it might be a rush job, they might not know any alternatives.
- developer93 3y ago
  Also to put the conversation on record somewhere so that if someone comes later with the same complaint they can see why the code was written like that.
karmakaze 3y ago
I wouldn't necessarily tag the author for a review. I might ping them to notify that I was doing some refactoring in the area and leave it at that. Of course it all depends on the team size, diversity/range of work, typical interactions between members, etc.
sumtechguy 3y ago
If someone had just summerly re-written my code I would personally go talk to them and found out 'whats up'. As it shows a communications breakdown. Second the person who did it should have approached the other dev and said 'hey I see you have blah why? would not this way be better?'. This happens many times in codebases. People begin to 'own' sections of it and they become very protective of what is going on in their area. They take it very personally if something changes and they were not in on it.

benterix 3y ago

I guess everybody has a different way of doing things. Personally I'd ask my colleague on Slack (privately), "Hey, you repeat 10 lines of math in each of these functions, wouldn't it make sense to make it a separate function?". The two probable answers are: "No, because... (reason)" or, less likely, "You're right, I'll do it tomorrow". No fuss, no problems for anyone, and someone can learn a thing or two.

moistoreos 3y ago
It also doesn't waste an entire evening obsessing over a refactor. A lesson I've learned to do with refactoring my own code.

Waterluvian 3y ago

It all seems like a sign of social dysfunction to me. That this blog post lacks most of the important details that have nothing to do with the code.

berkes 3y ago

If it is copy-paste out of laziness, then certainly "writing functions" (abstractions) would be the better solutions for repetition (DRY). But the author adds crucial context: > My code traded the ability to change requirements for reduced duplication, and it was not a good trade. For example, we later needed many special cases and behaviors for different handles on different shapes. It shows the difference between *accidental DRY* and *inherent DRY*. The context makes it clear that what we saw initially was *inherent*. Quite often, code is the same because the underlying libs A and B are consistent with eachother. Or business needs state that something has to be 15 items large, and another thing also has to be 15 items large. And so on. But when we look closely, the libs' consistency is really just chance: our wrapper around lib A and a wrapper around lib B may contain the same code, but they are wrapping something entirely different and abstracting that makes the result worse. Same with the "15 items". When business states "every list must always show 15 items", then sure, we should abstract the 15 somehow. But if it says "the top 15 on the homepage shows 15 songs" and "the comments under a song, by default show 15 items" then abstracting that 15 is a worse option.

Tainnor 3y ago
Without knowing too much about the context, it probably would have been possible to keep the structure where every shape is structurally independent from other shapes, but where calculations are abstracted away in pure functions that are just being called by the shapes. That keeps the maths in one place (and you can always add more calculation functions later if needed), but it doesn't entangle different shapes with each other needlessly.

JohnAaronNelson 3y ago

There was a significant bit of the article at the end where the author discussed future changes to the codebase that requires special casing handles for different shapes, how it would have been difficult with the refactor but simple with the repetitive code. I think that’s the critical bit is that you have to realize that the code will change in the future. Requirements will change, related code will change. If you make abstractions that restrict that ability to change, you’re hurting future you’s ability to adapt the codebase. More so because we often hold on, emotionally, to these abstractions we’ve invested so much time and effort into that we waste more time trying to elegantly fit the changes into our elegant solution. KISS

ainiriand 3y ago

For me the biggest red flag in all of this is changing a coworker submission without any kind of communication. Second red flag is that the refactor is not a good refactor, it lacks modularity and is still coupled, yes, you reduced some duplication but that's mostly it.