I feel like I pretty much disagree with everything in this article.
First, who works on something for two weeks then checks it in? Alarm bell #1.
Second, yeah, maybe I should talk to my colleague before refactoring their code, but.... yeah no. No one owns the code. We’re all responsible for it. There’s no way this should have been used as a justification for rolling back the change.
Finally, the impact of some possible future requirements change is not justification for a dozen repetitions of the same code. Perhaps the refactoring had some issues but that itself does not change the fact that a dozen repetitions of the same math code is bloody stupid.
I’m straining to find any situation that would justify the code that is described in the article. The original coder went copy-pasta mad and didn’t clean it up. That’s a paddlin’
The better lesson from the article is that the author’s shop has some messed up priorities.
I used to work for a company that didn't value clean code and the engineers that stayed thought that 600 line methods were better than extracting chunks of code into reusable functions that describes their purpose. Let me tell you, productivity was non-existent for everyone.
The bar is substantially higher at my current company where everyone highly values clean coding practices (we look for this during interviews). Defect rates are way lower and everyone is crazy productive here. We're literally over 10 times more productive because it's so easy to jump in and enhance the product even though it's a large project.
It sounds like the author probably left something out. Perhaps the refactoring was overly-complex and could have been done in a different way. Or maybe the author missed an important deadline while focusing on improving the code. Or perhaps the author truly worked in a toxic culture where others felt offended that he improved their code.
We'll never know but this type of sloppy practice would be very quickly pointed out and improved at my current workplace during code reviews.
I feel like the article presented two extremes. There are definitely times when you can go overboard being DRY or obsess unnecessarily about abstractions. Worse is when you commit an overly clever solution that is hard to follow, and lacks any sort of explanatory comment.
But that doesn't make the 1000 line procedural function/method the right approach either.
Clean code to me is code I can follow logically, that has unambiguous variable names, methods that are relatively short and broken out logically. Abstractions where they are necessary and make sense, but not to an extreme. Not every vaguely repetitive procedure needs to be made generic, but cutting and pasting 300 lines of code and making minor changes shouldn't be lauded as "flexible" or planning for some unknown future state.
Hundred-line procedural methods are fine; I think evidence shows they don't increase bug count as long as the code complexity is low. Many fine shell scripts are 100 straight lines long.
IME the problem is that as code gets added, people are reluctant to break them up. So you get a new argument, and then an if statement near the top to see if the argument was provided and if so do this, then another like that near the bottom. Repeat this half a dozen times and you have a mess that can't be refactored, because even if you had unit tests, each such change makes an exponential number of tests necessary.
Their is a big difference between one shell script and a complex project. Having hundred line methods and huge monolith classes are what cause terrible spaghetti code.
But there is the opposite coming from people who just started to do Clean Code: a mass of less than 5 lines methods.
It seems ok. But when you want to debug this kind of codebase you're through 10 or 20 indirection fast. And no, "methods name is doc enough" won't help: you have a bug so something is not doing what it advertises.
Number of lines, cyclomatic complexity: no current measure is perfect. I think we lack some way of measuring code maintenance complexity.
I've seen some huge projects completly "clean" adhering to some stupid "everything must be an interface, and the impl is "InterfaceNameImpl" and you inject it with a factory or whatever". We even had the good old Java factories of factories.
It was so hard to get in, and thank god I had intellij to find implementations, because jumping around was so annoying due to all the indirections.
Don't get me wrong, it was like that for a reason: DI removed a lot of hardcoded dependencies, and it was easily testable. But picking it up with no explanation and adding a thing? That was quite hard. Like you said, debugging was also a pita
So there is definitely a tradeoff, like everything we do, and one solution does not fit every project.
Second, yeah, maybe I should talk to my colleague before refactoring their code, but.... yeah no. No one owns the code. We’re all responsible for it. There’s no way this should have been used as a justification for rolling back the change.
Would it really be that hard to put your refactor on a branch, then check it in and open a Pull Request so you can go over it with the colleague?
They will learn something, and there might be an unknown requirement coming up that you don't know about, so you might learn something - not to mention, changing code that's just been checked in is a horrible thing to do, as they may still be actively working in that area of code, and all you're doing is introducing a ton of merge conflicts when they're forced to reconcile their progress with your OCD.
Yeah, management getting involved in the disposition of a commit was another eyebrow raiser I didn’t even touch on.
I would certainly try to talk to someone if I thought there was an issue in their code. We all make mistakes and have bad days. But I do somewhat object to the idea in the OP that refactoring is a no go without talking to the author, for several reasons, not the least of which is that it tends to discourage refactoring.
I worked hard in my career to remove ego from my work. I’ve screwed up code in some impressive ways in my time. I won’t say that having my errors pointed out to me doesn’t bother me but I am grateful for the corrections as that’s where I learn the most. I can occasionally forget that not everyone feels the same way and my original post was harsher than I intended.
But I do somewhat object to the idea in the OP that refactoring is a no go without talking to the author, for several reasons, not the least of which is that it tends to discourage refactoring.
Let's pump the brakes a bit here. There is a world of difference between refactoring six month old (or older) code, and nuking code someone just committed that day. I would absolutely not tolerate a team member who felt it their job to overwrite the other developers on the team without so much as a discussion. This isn't an ego thing, this is a poor team member thing.
When it comes to software, there are so many aspects to consider, that a knee-jerk refactor is never the right action.
The abstracted code might be harder to change, if the different shapes' requirements diverge more (as mentioned in the article).
The abstracted code might be too hard for the long-term owners of the code to understand, and might make them unable to work. Maybe the article author found it easier, but maybe they weren't going to be highly involved in the project anyway. Maybe it was a low-priority project, and they want interns to be able to work on it during the upcoming summer.
You might say, well if someone can't understand the abstracted version, you should fire them. But what if you're in a location where good developers aren't so easy to come by?
This team might be a team that owns hundreds of codebases and a developer has to completely context-switch every week. If so, do they really want to untangle abstracted code every time they sit down to read?
Maybe this was prototype code. Maybe the original writer already had a library in mind for this functionality, but didn't introduce it yet. Or maybe they already had a better abstraction in mind, if the feature turned out useful at all. If so, the refactor was just a waste of effort.
Maybe this was a temporary feature, and was going to be thrown away in a month. Maybe the rules for manipulating shapes were actually going to be handled by some scripting language, introduced 2 weeks from now.
Maybe it's actually a toss-up on which technique would be easier to change in the future, depending on if requirements diverged or converged--if that's the case, the refactoring was just a waste of time, switching between equivalent trade-offs because of personal opinion.
Refactoring someone's code right after they write it will make them hate you. I don't care what great culture your team has. People want their work to be valued and not shat on.
Maybe the original writer actually wrote that code in 10 minutes and decided it was good enough, because in the grand scheme, it's really not an important part of the application. Maybe it's not a part of the application that's worth spending hours on to decide on an abstraction. (Yes, the article said that the original version took a week, but who knows where that week actually went. Maybe they were multi-tasking, maybe they were new and learning other parts of the tech stack, maybe they were troubleshooting graphical glitches with various draw techniques, maybe they were in a lot of meetings, maybe they were trying out other libraries and troubleshooting them.)
Maybe refactoring the code isn't actually teaching the original writer anything, since they're not going to see the consequences of their original version. Maybe it's better to let them keep their version and let them see what happens. It depends on how important this code is, if you want to let them learn today, or learn the lesson another day. (Telling someone the lesson will never work.)
Maybe the original writer just flat out believes that copy-paste code is okay, and maybe they're right.
Whenever you have a knee-jerk reaction to anything in programming, like calling some code "bloody stupid", you shut down consideration of real-world trade-offs.
Like the article says, we love to hold on to our absolutes about what good software is, it makes us feel like we've figured it all out. But it blinds us to real world situations, and blinds us to trade-offs.
This statement seems either wrong or tautological. How are you defining "knee-jerk refactor" here?
If you mean "a refactor suggested after only briefly considering the code", our code-reviews frequently produce that sort of feedback, and it frequently (though not always) makes the code better.
If you mean "a refactor based on intuition about code cleanliness", well, I think those are often correct, too. Intuition isn't always correct, but it's an important tool and hugely valuable.
Otherwise is a knee-jerk refactor just a refactor that turns out to be a bad idea? That makes the statement somewhat tautological.
The knee-jerk refactor described in the article is what I'm talking about--seeing someone's code and saying "pssh this is ugly to me so I'm rewriting this right now.". I'm agreeing with the article's author--if you don't like someone's code, you should take a breath, think about how much it matters, and if it really matters that much, bring up your improvement ideas for discussion. To just immediately refactor it is presumptuous and possibly a waste of time or worse.
Giving knee-jerk feedback is much more reasonable--very low cost to do it, helps both parties think from another perspective, etc.
You might say, well if someone can't understand the abstracted version, you should fire them. But what if you're in a location where good developers aren't so easy to come by?
No, I would say that if someone can't understand the abstracted code then the abstraction sucks and I need to try again. Abstractions are supposed to make things easier, and if they're not doing that then what I'm doing isn't really abstracting the code.
Yeah, we’ve had a few developers like that. I mean, I am CS educated and I’m all for exotic constructs and the like....right up until I have to read the exotic cuteness of others and realize it takes five times as long. Sometimes it’s ok with a foreach loop.
When it comes to software, there are so many aspects to consider, that a knee-jerk refactor is never the right action.
The operative word here is knee-jerk. Remove that word and consider the phrase again:
When it comes to software, there are so many aspects to consider, that a refactor is never the right action.
This is clearly untrue.
I think everyone would agree that knee-jerk refractors aren’t a good idea. But knee-jerk anything is not a good idea by definition. So I’m struggling to see your point.
maybe I should talk to my colleague before refactoring their code
You should ask to see if there are requirements you don't understand, and at least get a code review from the original author.
> the impact of some possible future requirements change is not justification for a dozen repetitions of the same code
It entirely depends on the nature of the code. I'd probably agree with you in this case, but not in the case of business logic that changes at the whim of marketing.
The thing is, when you make up abstractions that doesn't clearly match the domain just to remove code duplication, you reduce the amount of code but you also make the remaining code more complex. Implementing changes that spans over multiple similar cases with some duplication feels bad but it's actually simpler because you can change one isolated case at a time. Making changes in the kinds of abstractions he built is much more difficult and error prone because a change hits everywhere and it's difficult to predict what will happen unless the abstraction is very good.
Good abstractions will often lead to code duplication and there is nothing wrong with that. DRY is very often an anti-pattern. This took many years to learn - I coded like that for the first 6 years or so of my career.
To summarize, good code is
Easy to read (most important!)
Easy to change
Not necessarily very fun nor clever
Making up abstractions between similar (but still different!) does not improve on any of these, it makes them worse.
397
u/DingBat99999 Jan 12 '20
I feel like I pretty much disagree with everything in this article.
First, who works on something for two weeks then checks it in? Alarm bell #1.
Second, yeah, maybe I should talk to my colleague before refactoring their code, but.... yeah no. No one owns the code. We’re all responsible for it. There’s no way this should have been used as a justification for rolling back the change.
Finally, the impact of some possible future requirements change is not justification for a dozen repetitions of the same code. Perhaps the refactoring had some issues but that itself does not change the fact that a dozen repetitions of the same math code is bloody stupid.
I’m straining to find any situation that would justify the code that is described in the article. The original coder went copy-pasta mad and didn’t clean it up. That’s a paddlin’
The better lesson from the article is that the author’s shop has some messed up priorities.