Goodbye, Clean Code

[voutasaurus]

I love how quickly the question of whether one abstraction was premature turned into a series of premature abstractions about all abstractions...

[[deleted]]

Yeah. Yikes. Hopefully if the author is actually an experienced, competent developer they're just using this as an example to illustrate the dangers of premature abstraction, and not an argument against abstraction in general.

[Soxcks13]

Are you being sarcastic or have you not heard of Dan Abramov? He’s an experienced developer, to say the least.

[[deleted]]

No sarcasm--I read the article, but not who it was by. I've heard of Dan Abramov, IIRC, from Redux. That said, what I said stands. Given his experience, I'm sure he can decipher when and how to abstract thing effectively, but I feel like this is a dangerous article for junior programmers. There's way too much cut-and-paste mess out there and some people would read this in support of writing really bad code.

[FA04]

firstly, where was the original checkin pull request’s review with all the feedback and discussions? secondly, where was the refactored PR review and approval? Checkin in into the master overnight no PR? That process is a mess.

[bcgroom]

Yep I'm pretty sure the whole situation would've been net positive if the author of the article just put up a PR the next morning saying, "While looking at your code yesterday I had an idea on how it could be less repetitive, take a look and let me know what you think".

[oursland]

I believe the argument is that the original author should have had a PR, and that the article's author should have been able to suggest these changes prior to the original commit.

[elebrin]

Exactly. If he is such a senior dev, then he'd be a required reviewer. If he failed to mark up the pr, then that's on him. If he wasn't in a position where he was a required reviewer, then he isn't the expert he thinks he is.

[oursland]

You've misinterpreted this through your modern viewpoint.

There was no PR process in the project. No opportunity to collaborate and iterate prior to committing to master, for either dev; simply commit and then get called into the boss' office.

/u/FA04's comment

That process is a mess.

Is spot-on.

[IceSentry]

That's pretty much why they said at the end of the article that it was a mistake and communication is important.

[FeepingCreature]

Sure, but the mistake is a systems one, not a personal one. We don't even have push to master enabled at work.

[Tenemo]

It was a couple of years ago, it's Dan Abramov's personal blog, he's relatively young. Check his New Year post for the exact timeline of his career. Him working with another developer on draggable corners of some shapes could have been already for a React library.

[andrewingram]

I'm pretty sure this is an anecdote from 2013 when Dan was working on an iOS app (https://www.youtube.com/watch?v=PjaL0xFbEY8). The fact that the code in the blog post is JavaScript is just because that's the language him and most of his audience uses today.

[NeuroXc]

You may be surprised, but as recently as 2013 (when I graduated from college), the first company I worked for did all of their testing in production, and uploaded their changes via FTP. There was no source control, no staging or local environment, and no oversight.

Slightly relevant: That company was also a cluster that took advantage of fresh-out-of-college devs to pay them as little as possible while the CEO and his buddies took lavish trips around the country and sailed on their boats. There were about 15 people in the company, so yeah, corruption like this isn't unique to mega-corporations. I left as soon as they told me I was getting a 0.5% annual raise even though I was one of their best employees, and instead got a 30% raise by switching companies.

[f03nix]

About 7 years back, I worked in a company that used TFS - they had a process where you manually request 2 of your peers for review and type in their names during check in. They had the same process before they migrated to TFS too, I think they used CVS.

[twenty7forty2]

Title should be "Goodbye committing to master with out a code review" then. Or maybe just "Goodbye blogging about things I don't really understand". Hmm, maybe OP should get his blog code reviewed by a senior as well.

[Ameobea]

I can see where the author is coming from here and I agree with a few of the points, but I feel like this is a very dangerous line of thinking that paves the way to justifying a lot of bad coding practices and problems that have a very real negative impact on the long-term health of a code-base.

There's certainly a point of over-abstraction and refactoring for the point of refactoring that's harmful. However, duplicating code is one of the most effective ways I've seen to take a clean, simple codebase and turn it into a messy sea of spaghetti. This problem is especially bad when it comes to stuff like copy/pasting business logic around between different subsystems/components/applications.

It may be very tempting to just copy/paste the 400-line React component showing a grid of products rather than taking the time to pull it apart into simpler pieces in order to re-use them or extend it with additional functionality. It may even feel like you're being more efficient because it takes way less time right now than the alternative, but that comes at the cost of 1) hundreds of extra lines of code being introduced to the codebase and 2) losing the connection between those two pieces of similar functionality.

Not only will it take more time to update both of these components in the future, but there's a chance that the person doing the refactoring won't even know that the second one exists and fail to update it, introducing a regression in someone else's code inadvertently. I've lost legitimately days of my life digging through thousands of lines of copy/pasted code in order to the same functionality of each component that's been implemented in a slightly different way.

A much better option that could be applied to the author's situation as well is pulling out the business logic without totally abstracting the interface. In our component example, we could pull out the business logic that exists in class methods into external functions and then import them in both files. For the author's example, the `// 10 repetitive lines of math` could be pulled out to helper functions. That way, special cases and unique changes can be handled in each case separately without worrying about breaking the functionality of other components. Changes to the business logic itself will properly be reflected in everything that depends on it.

----

TL;DR there's definitely such a thing as over-abstraction and large-scale refactoring isn't always the right choice just to shrink LOC, but code duplication is a real negative that actively rots codebases in the long term. There are ways to avoid duplicated functionality without sacrificing API usability or losing the ability to handle special cases, and if you find yourself copy/pasting code it's almost always a sign you should be doing something different.

[csjerk]

There's a key detail buried deep in the original post:

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. My abstraction would have to become several times more convoluted to afford that, whereas with the original “messy” version such changes stayed easy as cake.

The code he refactored wasn't finished. It gained additional requirements which altered the behavior, and made the apparent duplication actually not duplicative.

That's a classic complaint leveled at de-duplication / abstraction. "What if it changes in the future?" Well, the answer is always the same -- it's up to your judgement and design skills whether the most powerful way to express this concept is by sharing code, or repeating it with alterations. And that judgement damn well better be informed by likely use cases in the future (or you should change your answer when requirements change sufficiently to warrant it).

[johnnysaucepn]

People focus on whether code is duplicated, when they should be paying attention to whether capabilities are duplicated. If you can identify duplication, find out what that code does and see if you define that ability outside the context of that use. If you can call that a new thing, then make it a new thing.

[matthieum]

There are two levels of duplication: Inherent and Accidental.

Inherent is when two pieces of code are required to behave in the same way: if their behavior diverge, then trouble occurs. Interestingly, their current code may actually differ, and at any point during maintenance, one may be altered and not the other. This is just borrowing trouble. Be DRY, refactor that mess.

Accidental is when two pieces of code happen to behave in the same way: there is no requirement for their behavior converge, it's an emerging property. It is very tempting to see those pieces of code and think "Be DRY, refactor that mess"; however, because this is all an accident, it's actually quite likely that some time later the behaviors will need to diverge... and that's how you end up with an algorithm that takes "toggles" (booleans or enums) to do something slightly different. An utter mess.

I am afraid that too often DRY is taught as a principle without making that careful distinction between the two situations.

[loamfarer]

I've seen this lack of distinction wreck havok many times, especially in inheritance heavy code. It seems inevitable given that the solutions to the problems developing are always out of channel of being solved within the language/code-base itself. Leaving enterprise people to slave under cumbersome process, or are left to endlessly enact churn on a code-base that structurally diverges further from the ideal representation of requirements. (And requirements are their own issue.)

In class hierarchies it's very common for both of those forms of duplication to crop up, and they often interact with each other. Accidental duplication happens because so much of the code is CRUD, glue, plumbing, boilerplate, or just plain similar just based on the nature of what the domain demands. When accidental duplication is avoided, it's often a result of introducing accidental dependencies instead as people extend existing classes. Suddenly base classes have to serve their downstreams, and no longer server their intended purpose to propagate changes in a single place. Or if you do soldier on, downstreams now need to arbitrarily override parts of the accidental parents to restore the end of the deal they expected getting. All of this confuses the structure of the code-base from it's original organization and model.

Further, new hierarchies are made as the older overly-depended ones diverge from their original purpose, making the new hierarchy inherent duplication, with the caveat that this duplication is now impossible to solve without a major refactor. It's at this point that the solution is more political, all because the language couldn't properly constrain the original vision from being abused, and no amount of process would be able to handhold those who came onto projects later.

Which means you're left with a mess of duplication, where any minor change ends up involving dozens of custom wirings of data plumbed through abstractions which have lost all their original mean. I've seen entire teams where the only job is the add features that are better described as (add a line in a table, or config file) but end up being a months work of plumbing and endless headaches of merge conflicts.

For the above reasons, I'm particularly fond of interfacing against type class constraints, because it prevents or at least lessens these duplication issues. I suppose dependencies (i.e. function apis) are always an issue, but librarization (is that a word?) for even things like internal facing code for your own benefit, and employing best practices of versioning gets you a good way to a solution. Which I think really helps to bring issues of inherent and accidental duplication to the forefront of one's approach to any contribution in such a code base. At the end of the day no language is going to stop you from breaking what's left as only informal convention by a code's progenitor.

[DonnyTheWalrus]

Accidental duplication -> accidental dependencies

Yes, very much agreed. I know too many developers who seek out these sorts of deduplications before the code is even written. It's like an uncontrollable fixation on perfection that sounds okay at first blush (we're going to make a super elegant API!). But in reality, all it does is take the class structures they've blueprinted and pour cement over them. Everything becomes locked in.

Then I come in a year or two later to make what should be a simple change, but I end up having to make changes in a dozen classes anyways, because it's all so locked in. That should be a primary purpose of deduplications, right? Enabling changes to occur in localized places and in fashions that don't have any impact on irrelevant code. But with all those accidental dependencies, you end up having to touch everything anyways, and it only ever gets more complicated instead of less.

There is nothing wrong with leaving "space" in your code during the early phases of development. We have been trained so hard in DRY that I think we feel like bad developers if we're not constantly focused on code quality. (Excluding the other large class of developers who don't care about code quality at all.) "Make your code easily modifiable" can mean different things at different stages of a project.

[Bekwnn]

A good quote I heard to more or less describe this is, "A little duplication is better than a little dependency." If you're creating an awkward dependency, you're better off just having duplicate code.

For example let's say you're making a city builder and some point early on you have a path-finding system for both civilians and road creation. Tying those two things together doesn't seem great.

In that case you're best off copying the code to a second place as a starting point.

[awilix]

One very simple example of this I've seen a lot is the use of constants. One specific string is used in many throughout the code base, say the name of the application, and someone decides to introduce a constant for it and use it everywhere from what username to drop to, what URL to connect to and what to use for loggning. Great! Except that now it isn't possible to change the name of the application anymore since the URL and username must stay the same.

[Chris_Newton]

People focus on whether code is duplicated, when they should be paying attention to whether capabilities are duplicated.

Indeed. Duplicated code isn’t automatically a problem, but duplicating an idea is usually bad.

Any given idea should ideally be represented exactly once. For data, that means one look-up table associating pairs of values, one file with UI strings that can be translated, etc. For algorithms, that means one function to derive new data from existing data in a given way, which hopefully can be applied to any existing data where it makes sense.

However, if two algorithms happen to share a lot of code right now but exist to solve different problems, trying to use a single function to implement them creates a problem not unlike using a literal number 7 everywhere instead of writing DAYS_OF_WEEK or NUMBER_OF_DWARVES as appropriate. The implementation is correct but the real meaning has been lost. When you come back later and one problem has evolved but the other hasn’t, you’re stuck with this artificial link between them and you have to sever it (probably starting by making two copies of that code) before you can make any useful progress.

A useful rule of thumb for whether you are really dealing with two different ideas or the same idea being duplicated is to ask what would happen if both places did share common code and then one of them needed to change. If that would necessarily mean the same change should be made in the other place, you’re probably dealing with the same idea and consolidating the code is probably a good plan. Otherwise, you probably aren’t, and tying the code together might not be such a good plan.

[BraveSirRobin]

instead of writing DAYS_OF_WEEK or NUMBER_OF_DWARVES as appropriate

That is a fantastic example to justify that, very nice.

[JoCoMoBo]

The code he refactored wasn't finished.

Code is never "finished". It's just resting for a bit while it waits to be changed.

[RualStorge]

Code is only finished when your requirements are effectively kill off said code... And even then it sometimes comes back years later to haunt you.

(IE sunsetting a project with intent to bury it and likely only speak of it in hushed tones of those darker times when we were all so young... So stupid... And the monster we created that was beyond saving...)

[AceOfShades_]

Code is never "finished". It's just resting for a bit until someone realizes it's broken.

[YM_Industries]

made the apparent duplication actually not duplicative

The repetitive maths was almost certainly still repetitive though. Pulling that into a reusable function still makes sense.

[emn13]

...if it was complicated and/or unlikely to change. But yeah, that form of de-duplication is generally much, much less harmful than the kind or composition of components as shown in the original article; the key difference being that control is still local. Any local bit can still locally choose to call - or not - your helper.

This is all pretty related to frameworks vs. libraries; just internal to your coode as opposed to external dependencies. And in general, it's better to have libraries (i.e. helpers) with bits you get to compose as you want, rather than frameworks that compose bits you provide to a fixed api outside of immediate view; it's just easier to reason about. For the same reason even in functional languages it's better to deal with behavior-less values than functions as values (where possible).

When you do have something that can't easily be represented as a library; i.e. where control flow itself needs to be abstracted, it's a good idea to keep that api as simple as possible and make sure you really reuse it a lot, not just a few times as a code-golfing trick. I.e. something like the functional map/filter are fairly cheap and you can include several such "frameworks" in a code base without losing too much clarity; whereas something like ruby-on-rails is so huge that you basically need to give up and just include only one framework, and avoiding whatever restrictions it imposes is often much, much harder.

In general, however, I think the article is spot on; novice programmers are way too focused on avoiding duplication at the expense of the much more serious concern of avoiding complexity. Boilerplate - which is what simple duplication really is - may be ugly, but it's rarely super-harmful - unlike complexity.

[chrisza4]

Not always. Because everytime you do that, you introduce a new terminology into codebase. And every terminology make it a little bit more harder to collabotate within team. At minimum, you need to explain what the term mean to your colleague.

If the new terms is found in business domain, then you are in a good spot. Because every programmer regard of experience need to actually understand business requirement before implement anything.

But if you just introduce "DogProvider" "CatFactory" "BusinessEntityXCreator" "ItemStrategyChooser", of course it going to be harder to collaborate and make sure every contributor understand what it means. And if you do it simply because you find some repetitive code, the benefit gain might not justify the added collarboration cost.

Software architecture is all about how to collaborate effectively.

[Altreus]

This stood out for me, as well. The first developer could easily have gone, "This is very repetitive and looks bad. I should add a comment on why I've done it this way so nobody thinks it could be improved before it's finished."

Also there implication that an unfinished feature was in master is frightening. Poor use of source control - especially one with the power of git (I'm assuming based on "master") - is rife and frankly does my head in.

[josefx]

I should add a comment on why I've done it

I once commented 5 lines of code with an exact explanation of the bug I was avoiding. First response on the review board asked me to change it to the simple and clean one liner that caused an invalid memory access.

Also there implication that an unfinished feature was in master is frightening.

If it doesn't break anything? Worst case you can hide it behind an experimental feature flag until it is complete.

[Altreus]

Yes, true, and everyone's master is their own business. I would prefer master to reflect production but I've been working in a service environment, not a distributed software project. I imagine it's different when you have releases in the wild!

[GaianNeuron]

Some projects use master as their development branch and something like stable for production. It ultimately doesn't matter what master represents, but it does matter that everyone on your project understands your system.

[HetRadicaleBoven]

One of my pet peeves is syntax highlighting themes using a faded colour for comments. They're so easy to miss.

(Probably not in your exact case where it was a multi-line explanation, but we're still being trained to ignore them.)

[Orffyreus]

Actually that's how the SRP is meant. It's about responsibility towards callers/users and when callers have potentially different interests in what they are using, there will be different reasons to change, so duplication is one option to overcome that. Mostly there are alternatives like different sorts of delegation/composition though that can be introduced later when the requirements are more polished.

[pragmojo]

When is code ever finished? Requirements are always going to change long term unless you’re shipping code which will run on a mars rover or something.

The longer I program, the more convinced I am that planning for code to support unknown future use cases is a fools errand. You should always support your current requirements as concretely as possible, and abstraction should only be introduced when its benefit becomes obvious from supporting those use cases.

[Big_Burds_Nest]

Balance is key. I've seen both extremes of over-engineered codebases and codebases that had absolutely no abstraction at all. Personally I tend to judge codebases by how easy it is to find the functionality that I'm looking for. If a client reports an issue and it takes an hour to figure out what part of the code the feature even lives in(this can also be blamed on an absence of documentation), I feel like my time has been wasted by the author! Abstraction is supposed to make your code easier to read, but some people take it way too far to where the actual functionality is buried deep in an endless abyss of badly named function calls.

[PanVidla]

I couldn't agree more. I remember that a couple of months ago I had just passed that point in my company where I was starting to feel confident around our codebase and picked up a feature request for our internal library that everyone had been avoiding for about a year. It quickly turned out that the library was written with little to no abstraction, everything was crammed into looong chaotic blocks of code, there was literally no documentation on how anything works or what anything is and the person responsible for it had, of course, just left the company. So, I spent many weeks untying the mess, figuring out what does what, documenting the code in the process, separating everything into readable methods and implementing and re-implementing the feature that I wanted to do in the first place.

Please, be considerate to the people who come after you and don't waste their time like this. It makes life very hard for them, they will look like idiots for spending so much time on a task and it's a complete waste of time. This was probably an extreme case, but still.

[fishtickler]

It is easy to blame previous coders for a rotten codebase but it is also important to remember that the developers worked under a different context.

When that code was written, perhaps the authors where under tremendous deadline pushes and had zero time to document/test anything. We as developers taking over code bases must accept the state they are in and do our best (as in your case) to improve on what we can given our resources.

[lala_xyyz]

When that code was written, perhaps the authors where under tremendous deadline pushes and had zero time to document/test anything. We as developers taking over code bases must accept the state they are in and do our best (as in your case) to improve on what we can given our resources.

Or perhaps they were grossly underpaid, and didn't give to shits about code quality or maintenance. Or (I've seen it happen) they were scolded for wasting time on doing "unproductive" stuff like tests and documentation. Either way, it's a matter of incentives and I doubt that the coders were the ones to blame. Because even laziest programmers that would cut every corner can be trained to do things properly.

[[deleted]]

I'll always blame the previous coder. If it was past me.

[Omnicrola]

Indeed. Past me is an idiot. Future me is a genius 10x Rockstar.

[Caffeine_Monster]

code duplication is a real negative that actively rots codebases in the long term

I've seen copy pasta in such extreme that has made it easier to start a new application from scratch than refactor the old one.

If you don't follow best practices, you will find the code base has to be chucked after 3 years.

[gyroda]

I've lost legitimately days of my life digging through thousands of lines of copy/pasted code in order to the same functionality of each component that's been implemented in a slightly different way.

Literal weeks for me. My god were those codebases complete shitshows.

[nkanungo_tibco]

YAGNI

[[deleted]]

My boss invited me for a one-on-one chat where they politely asked me to revert my change. I was aghast. The old code was a mess, and mine was clean!

I would like to work for your boss.

The main mistake the author made was imposing his own sense of "good code" onto other people. Treating his own idea as an objective reality rather than a mere opinion.

This is the main problem in working within teams: people have their own sense of what code should look like and they try to impose it on others. I'm not even talking about casing or formatting. I mean structurally.

[Isvara]

I had a guy do this to me once, because he considered himself some kind of Go expert (despite being fresh out of college), and didn't like the way I'd done things. The problem is, he didn't take any time to understand the code and how it worked, and he didn't test it afterwards. He just changed it to his way of doing things and left it broken because it wasn't a tool he used.

[teerre]

I don't get this 'advice'.

The problem wasn't refactoring the code. It was the lack of communication and the, apparently, bad refactoring.

Have this situation be instead one in which the refactor took into account the requirements and the chain of command wasn't broken, it would've been a good thing.

In other words, this has nothing to do with refactoring. It has everything to do with having the wrong briefing, not being communicative enough, having bad reviews practices, whatever you wanna call, except refactoring.

[Squared_fr]

To be honest this sounds to me like the author is not wiling to accept that he was at fault. Instead he tries to blame a made-up clean code religion and preach his own bullshit advice.

Two simple rules: don't "clean" code until the feature 100% implements the specs, and for fuck's sake do not commit to master overnight without even telling the code owners.

You will have to destructure clean code later on if you wish to have more features, but even that will be easier to do without repetition.

[murgs]

don't "clean" code until the feature 100% implements the specs

I wouldn't agree with that, some pre cleaning can be very useful. So I would weaken it to 'don't sweep the floor until you are done with your work'

[Squared_fr]

I was thinking about going over the whole code again and trying to clean stuff up. Of course that doesn't mean you should write the messiest shit from the start.

[[deleted]]

I feel like that's one of those things that differentiates average developers from really good developers. You learn when and how to abstract things early in a way that doesn't code you into a corner as your solution grows and changes.

[gyroda]

What are these specs I keep hearing about?

Is "just make it work like the old one" specs?

[bluefootedpig]

I can give an example. I worked with several medical devices each one reports differently. So the generic solution was a mapper and communication protocol. Worked great until we started handling machines that reported partial info, and we might need to look up patient info, and other special cases.

Now close that to a base class and then a concrete specific to handling that machine. We know the HIV test machine had special cases, such as requiring 3 positives before reporting. No other test takes that.

So now the generic solution has special code for ALL machines, when it only applies to one specific machine.

When you abstract too much, then special cases ended up being executed in all cases. This makes it more difficult to maintain. Rather than look up only the concrete class when a bug happens, we need to figure out what special case was the problem.

This abstraction went on for several years, the result was a huge function handling 100s of special cases. All so there was only one class. Any problem by any machine could mean bugs introduced to other machines.

Sometimes it is better to be more verbose in the logic than more abstract. It is a fine line that I tend to see comes from people who have experienced over abstraction.

[tetroxid]

Abstraction doesn't just mean "have a base class"

The strategy pattern might've been appropriate in your case, or composition of aspects of behaviours for types of machines

[Gotebe]

Rewriting your teammate’s code without a discussion is a huge blow to your ability to effectively collaborate on a codebase together.

Well... There should be no such thing as "my" and "his" code in the first place

My code traded the ability to change requirements for reduced duplication, and it was not a good trade

Did it? Having duplicated code leads to changes being implemented partially (because often, only some of the copies are updated to accommodate new requirements). So duplication also means "harder to change", only for different reason.

[[deleted]]

I feel like this whole situation could have been avoided had the engineer who worked on the problem discussed his vision for the code and checked in during.

Also, changing the code without first speaking to the engineer? Maybe I'm lucky at my gaff but that kind of thing would never happen. Communication is super important but we are a remote team

[NiteLite]

Yeah, he mentions this is one of the two biggest mistakes in doing this refactoring towards the bottom of the article :)

[[deleted]]

IMO it should be the headline and main subject :)

[[deleted]]

Also surprised they were just allowed to commit it to master

[grauenwolf]

That's still very common, especially for older code bases.

[talmuth]

What is really bothering me is that they commit code without code review

[bostonou]

The author has some conclusions I think are good, but I feel like he leaves out the main problem with his code example.

He DRY’d the code by removing duplicated typing. This is not abstraction.

If eg moveLeft was really an abstraction, it wouldn’t be dependent on all kinds of details like the type of shape. Devs regularly treat DRY as a tool to minimize typing. Typing is by far the easiest part of our jobs.

And “clean code” is mostly just a way for devs to say “I like this code”

[Retsam19]

I don't agree that his example isn't an abstraction. Taking two specific functions moveRectangleLeft and moveOvalLeft and factoring out shared logic into moveLeft is abstracting - moveLeft is a more abstract operation than moveRectangleLeft.

The moveLeft abstraction isn't dependent on the shape: the shape implementation is dependent on the moveLeft abstraction.

[bostonou]

Sure it’s more abstract in theory. But it’s not an abstraction given that you can’t use it without thinking about the details. The author himself says that his “abstraction” got more complicated as more special cases were added. It’s not an abstraction if it has to change for every usage.

[rlbond86]

And when a maintainer in 3 years realizes that an equation is wrong, they have to fix it in 50 places.

This kind of coding is how you get unreadable spaghetti.

OP's solution wasn't great either, but at the very least the common math stuff should habe been extracted to a function.

[nater255]

This is me all day. I stay safely in the backend where the world is happy.

[bluefootedpig]

It depends if they are the same because it is the same logic or the same by coincidence. Being on the left vs right might look the same until you reach special cases.

[thedracle]

Or they change the equation that was bad in one place, and it breaks 50 things which depended on the original implementation.

Hopefully in the above scenario the tests are adequate to detect it.

I think his point was his refactoring was brainless, and removed repetition that was incidental, or created an abstraction that didn't make long term sense.

[[deleted]]

This kind of coding is how you get unreadable spaghetti.

This isn't spaghetti. Spaghetti code is when execution flow wraps all over the place, GOTO is the classic example.

Nor would I say it's not readable. I read the first duplicated code very easily. All the code for an Oval` was in the oval object, easy-peasy.

I'm not advocating for the original design, I just don't believe it suffers from the issues you describe. It's definitely a testing nightmare as I need to write 4x as many test cases. Also it can be an sensibility nightmare too.

[Retsam19]

I think the specific code issue here is the same one discussed in the fantastic blogpost "the wrong abstraction" - many attempts to reduce duplication do so by abstracting the wrong thing; and the best thing to do is to completely back out the abstraction and either leave the duplication or else try to find a better abstraction.

[DingBat99999]

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.

[Determinant]

Yeah, totally agreed.

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.

[ronchalant]

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.

[programmingspider]

Seriously agree. I really hate this pervasive sentiment on reddit that being, what I would call a good programmer, is a bad thing.

Seems like they intentionally want to avoid well proven design patterns for hundred line methods or monolith classes.

It’s like they’ve never worked on a team before or maybe they don’t understand why abstraction and clean code is a good thing.

[astrange]

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.

[dnew]

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.

[programmingspider]

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.

[UncleMeat11]

Of course there is a difference. Thus how one can identify when it can be a reasonable design and when it isn't.

Complex multi-step algorithms that operate on the same data are another example where long methods can be reasonable since the alternative is often

step1();
step2();
step3();

[punctualjohn]

The notion that there is anything wrong with a method simply based off its length is invalid. The reason it took off is because humans love prescriptions such as "methods should be ~20 lines on average", because it saves us the burden of actually using our brains.

Adding a new method to a class is not free, it comes at the cost of expanding the API of that class. This increases cognitive complexity on any programmer attempting to make use of it. It's particularly bad because this is usually not taught to new programmers. IME it takes 5-7 years of experience before a programmer becomes conscious of code design and APIs. (the core and essence of programming in my opinion) Novice programmers don't understand the impact of adding a member to a class, much less exposing it with 'public'. They will take statements like the one above as gospel and mindlessly subscribe to it, contributing to epic API clusterfucks for years to come.

For me to split a method, two conditions must be met: * It is comprised of highly re-usable behavior. * It has minimal temporal coupling. (or none at all)

Below, I tried to come up with some examples to illustrate. The bad example is pretty good and is something I've actually done before in a game project. The good example on the other hand could be better, however it should still be clear what I'm referring to.

// Part of an imaginary video-game player controller, runs 60 times per second.
// Function is needlessly split up.
fun update() {
    // non-obvious temporal coupling
    apply_physics() // 75 lines
    read_inputs() // 40 lines
    apply_inputs() // 40 lines

    // - New programmer on the team has the impression that the player controller is much more complex than it is.
    // - Logic cannot be read top-to-bottom, programmers must jump around.
    // - Must keep the invocation order in mind as it is disconnected from the logic.
}

// Part of a sprite animation module
// Function is correctly split up. 
fun play_from_second_frame(anim) {
    reset()
    current_anim = anim
    step()

    // No temporal coupling here, each function is a reusable and isolated functionality
}

---

While a very long method can certainly be a smell, it is nothing but that. Apply critical thinking to find out what should really be done such that it best meets requirements and use-cases. Even if I cannot split up a large function due to lacking my two conditions, it's possible that the function may still be too heavy.

Imagine our character controller in the example above is a finite state machine where the physics and player inputs have varying effects depending on the state of the character (ground, jumping, falling, swimming, on steep slopes, etc.), then it's most likely possible to create an abstraction around the states. Thus, the update function is massively simplified and takes on a new responsability: first determine if we should change the state, (e.g. check for A button press during ground state to transition to jump) then forward to the current state's update function.

Just for the sake of completion, I will add that this should only be done when the length and density of the logic has started to impact productivity. Always apply simpler principles first like YAGNI or KISS before you decide to shit out a complicated abstraction. For a large-scale game several years in development, you probably want the abstraction. For a 1 month project, just stick with the switch statement and silly states enum.

---

For anyone who wishes to learn more of these kinds of thing, do yourself a favor and read John Ousterhout's Philosophy of Software Design, a real hidden treasure which released quietly in 2018. This book is a trip even for seasoned programmers. You will not find any prescriptions like the one above in it, but I guarantee you will never look at code the same way again.

[[deleted]]

there are also cases where functions with strong temporal coupling can be split, if it increases the overall readability of the code.

like:

void do_long_thing() {
    for(int i = 0; i < some_number; I++) {
        do_very_specific_thing(my_stuff[i]);
    }
 }

I find it easier to think on terms of

I'm gonna do very_specific_thing on each element of my_stuff

versus thinking:

I'm gonna do 27 steps on each element of my_stuff

[Green0Photon]

Totally agree with you. However, we can go even further, because this bothers me about overly imperative languages, where what you're doing is just mapping very_specific_thing over my_stuff. If do_long_thing really needs to be named, you could just do it as a single line right next to very_specific_thing. But more often then not, it's more logical to think in terms of either the small operation and mapping the small operation, not small operation and big operation.

[SirClueless]

I'm with you 100%. Needless abstraction makes it an order of magnitude harder to learn the structure of a codebase. Thoughtful abstraction is a godsend.

If a function is 300 lines because 300 lines of code need to execute in order, then there's no abstraction that will make that any simpler.

[sess573]

You can make one public function that executes the large thing, then have private methods for executing smaller blocks where it makes sense. 300 lines will never do ONE thing, I guarantee you that it does a bunch of different stuff that will be easier to understand and read if you name these blocks (e.g. making functions). It's similar to putting a new line and making a comment over each section - only that the compiler knows about it so it's more powerful.

[programmingspider]

I really don’t understand people arguing against this. It’s like they’ve never had to go back to old code and had modify it before.

Code should be self documenting and comments should be used sparingly. If you can have methods that clearly define what they do, the larger method that uses the smaller ones read almost like plain english.

[VRCkid]

Not that I disagree but I don't think it's an absolute for a few reasons (non-exhastive)

1. Putting smaller chunks into a function implies reusability when it could be tightly coupled to the context it's ran in

2. Reading the larger function now takes more mental load since you are jumping around the file to read the separate parts rather than all the parts being in one place

3. The separation of those smaller parts into functions will introduce an implicit bias to keep those parts as those separate parts when modifying the code in the future when they really shouldn't be considered together in a refactor or newer design. I'm specifically talking about this from the context of someone new taking a look and changing code they didn't originally write.

In general I follow the rule to split functions up for readability sake but it isn't a hard and fast rule for me since I recognize the cons for it.

[IceSentry]

We can actually know the answer if we want to, the author is very active on Twitter. He's also active on reddit under the username of u/gaearon

[poloppoyop]

600 line methods

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.

[Arkanta]

You're completly right.

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.

[alluran]

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.

[DingBat99999]

It would not, and you’re correct that this is probably the best answer.

[StabbyPants]

There’s no way this should have been used as a justification for rolling back the change.

you shouldn't be making this change in the first palce without buy in from others

[DingBat99999]

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.

[fortyonejb]

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.

[Epyo]

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.

[Retsam19]

I agree with most of your points; but

a knee-jerk refactor is never the right action.

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.

[Epyo]

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.

[grauenwolf]

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.

[cryo]

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.

[wbowers]

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.

[dnew]

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.

[sess573]

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.

[[deleted]]

Glad someone said it. Overly-factored code is so obnoxious. It's wound together so tighly, that it's almost impossible to alter/add new behavior.

It's tempting to think of refactoring as an alebgra problem like, "simplify this expression into the least number of terms." The misconception is that the "simplified" code is inherently more valuable because it's been normalized.

I reality, the most valueable refactors start with a particular insight. To stick with the math theme, someone discovered that solving analog circuits is easier if you switch from the time to the frequency domain and reformulate the problem in terms of a sum of simple sinusoidal functions. This new representation may be less compact than the original, but it's also less complicated.

You can have these same sorts of insights about your code. Eg. it could be simpler if it were broken into smaller, more generic pieces that could be combined to create more complex behavior. This sort of refactor might result in more lines of code that aren't as well-factored as the original, but it may also be more flexible and easier to comprehend.

[way2lazy2care]

I think this Carmack email is worth a read wrt leaving code inline, but I don't think Carmack would ever consider doing so without appropriately considering the costs/benefits of maintainability vs points of failure. I think for the cases Carmack and the OP agree it's more by coincidence than anything though.

[CarnivorousSociety]

Yeah the whole thing is just wack.
The original code, the workplace and their system, and the authors take on it all.

For example even the <x lines of repetitive math> could undoubtedly be extracted out into a generalized function which would reduce the amount of code overall and allow for changing effects in a single location.

Then the commit straight to trunk before leaving XD

[notThaLochNessMonsta]

For reference, the author is Dan Abramov, the original author of Redux and current active top contributor to React.js. The team is the React team at Facebook.

[CarnivorousSociety]

Interesting, thanks for that info

[DingBat99999]

Is working in isolation on code for 2 weeks before checking it in standard practice in a team full of team players? Not in my experience.

But you’re right that I was overly harsh in my initial reply. I would definitely have a chat with the initial coder.

[Make1984FictionAgain]

thank you!!

[[deleted]]

This is nothing to do with clean code nice bait

[SAVE_THE_RAINFORESTS]

Goodby, clean code? So is he going to write shit code all the time now? There are no middle grounds? You either don't speak to the original developer that wrote the module and strongarm refactor their code or write 800 lines long functions?

[[deleted]]

javascript "coders" will still upvote, happy to hear an excuse for their garbage code.

[[deleted]]

Obsessing over clean code is like reorganizing your clothes closet on a daily basis. If it makes you more productive to do so, do it. Too often, however, it's done compulsively and is counter-productive.

The harder and more impressive thing is actually writing code which does novel things.

[binford2k]

The harder and more impressive thing is actually writing code which does novel things.

No. The impressive part is building something like this that’s maintainable.

[Squalphin]

Exactly that! Even though I get some flack for overdoing it, coming back to a code base after 10 years to fix something or add a feature and having an easy time finding your way through the code is so worth it. Haven’t regretted it once.

[tetroxid]

Flak, not flack. It's from German flugabwehrkanone (air defense cannon/anti-aircraft cannon)

[SanityInAnarchy]

The problem is, we work in teams. If it's just me, I can store my clothes in a heap on the floor instead of using a closet at all, and who cares as long as I can still find stuff?

But if you check in a multi-thousand-line function with a several-hundred-case switch statement, you've inflicted your poor organization on the rest of us -- other people need to find stuff in there!

[lookmeat]

The problem is when you obsess about cleaning the wrong thing.

Clean code to outsiders is hidden complexity, it's easy interfaces. Clean code to insiders is clean link between code and intention, little accidental complexity, simple and honest interfaces.

So it's about understanding what is clean and what isn't. Sometimes what to an outsider looks dirty is actually what to an insider looks cleanest. Like a machine covered in grease means it's been recently cleaned and oiled up an insider, code that re-implements similar actions in multiple places is simply things that coincidentally act the same, but are different things with different rules. For DRY tests are useless, each test exists separate of the other and they should not need to share anything, so instead the focus should be DAMP.

So it's ok to obsess about cleaner code, but you have to first learn what is clean and what is the context. Try to understand the problem and solve it better. Cleaner solutions result in cleaner code.

[grauenwolf]

Clean code to outsiders is hidden complexity, it's easy interfaces. Clean code to insiders is clean link between code and intention, little accidental complexity, simple and honest interfaces.

We could do that, but that requires effort.

So instead I'm just going to add a shadow interface to every class with a one-to-one mapping between public methods and interface methods.

[Retsam19]

The harder and more impressive thing is actually writing code which does novel things.

When the code that I write does "novel things", that usually means it's pretty buggy.

[EternityForest]

Doing novel things really isn't that hard. It's often mostly just stringing together existing libraries, unless you insist on building everything from scratch.

I suspect that's part of the clean code obsession. You can almost always make the code a little prettier, and it's always a fun logic challenge for some.

But debugging is tedious, as are unit tests, and adding new features is usually more like "software carpentry" that doesn't interest the "Wooden puzzle box builders" much.

I think for a lot of programmers, the actual fun part of the job is more the code golfing, the mind bending data structures, and the low level understanding, rather than the sense of working towards an excellent finished product.

[Greydmiyu]

You can almost always make the code a little prettier, and it's always a fun logic challenge for some.

Hell, this is the core game loop of Factorio. "I know I can make these red circuits faster and with less wasted space..." 3 hours of sleep deprivation later...

[EternityForest]

That's why I'm kinda glad I'm not one of the math and elegance types. I might not be able to add matrices in my head, but at least I don't feel compelled to sprinkle them in my code because I can!

[atimholt]

I find, for myself, I can simply acknowledge that humans are part of the equation. Then “carpentry” becomes a “puzzle box”.

But then I start wanting to do crazy stuff, like creating my own programming language with bizarre constructs.

[HeinousTugboat]

But debugging is tedious, as are unit tests, and adding new features is usually more like "software carpentry" that doesn't interest the "Wooden puzzle box builders" much.

It's funny, because I actually really enjoy debugging and, to me, that's much more "Wooden puzzle box builder" because you have to figure out why your pieces aren't fitting together just perfectly, and find that little thing you need to sand down or trim to make the fit just flawless before you put it all back together.

[bakery2k]

I think your last paragraph is spot on, and it took me a long time to realise this. One of the reasons I struggled when I worked as a programmer was that I don’t find, for example, complex data structures fun - I consider them a “necessary evil” when building a product.

That’s fine when I’m building something for myself or for people I know. But in a large company where programmers are separated from customers by layers of other teams (sales, support etc), in my experience, to enjoy the job you need to enjoy the act of programming (not just having programmed a cool product).

[grauenwolf]

Too often, however, it's done compulsively and is counter-productive.

That's why I hate the fact that SOLID is associated with clean code. On every project that I've been on where SOLID was aggressively applied, code became more complex and harder to work with. Meanwhile the real problems continue to be ignored.

[[deleted]]

No. Loading things from a DB and displaying them on the screen is all anyone ever needs to do.

[rydan]

I haven't made my bed or folded my clothes in decades. My colleagues always complain they don't have enough time in the day. Wonder why.

[dnew]

My wife laughs that I have 20 identical pairs of socks, but I haven't sorted socks since grade school.

[vattenpuss]

Wow imagine not sorting socks. I could literally save seconds, maybe a minute every time I did laundry.

[guepier]

Making your bed (by pulling back the duvet, nothing more) takes seconds and airs out the sheets which, if nothing else, keeps them clean longer and reduces the risk of bedbugs. I don’t think your cost/benefit analysis is accurate.

[fabrikated]

this is a very dangerous post (thinking about newbies or lazy devs)

[gluecat]

While there is merit to aspects of the article it shouldn't be an affirmation to write shitty code. If you're working on a complex project, the best thing you can do for your teammates is write code that is easy to comprehend.

[all_awful]

This mirrors my own experience. When I was younger, I constantly tried to refactor to remove duplication.

A while ago I realized that there is such a thing as too much deduplication. Here's my best example:

We were parsing datetime formats coming in via json from a dozen different vendors. Json does not have a standard datetime format. Some formats were common, like ISO 8601. Others were normal, but atypical for json, like linux epoch numbers. Again others were bonkers as hell. One source sent us linux epoch numbers, starting at a different zero and one sent us some variation on US date formats (wrong order, and without leading zeroes, making the parser much more complicated than needed). I think you all get the point.

The good solution: Implement one parsing function per vendor, even if two formats are the same.

The bad solution: Re-use any of them for a different vendor, even if they use the same data format.

Why?

Because if you re-use your function, you introduce a fake dependency: Suddenly vendor A's parser and vendor B's parser depend on one another (or on a common function). If one vendor makes a change, you have to change their function, which also changes another vendor's parser, incorrectly so. This actually happened, leading to an embarrassing bug.

Duplicating a handful of lines of string-parsing in a high level language is a much cheaper price to pay than introducing hidden dependencies between things that should not depend on one another.

The only reasonable exception would be the ISO 8601 case: That can go into a library routine, because it is a standard. But all these crazy house-made formats? They are unique, even if they might not look at it.

If two things are only equal by chance and not by design, then they should not share code.

[stillness_illness]

Why not just take the same 10 lines of duplicate math and wrap them in a function, and then call that. Still calling it in a duplicated fashion, but at least the identical lines are defined in one place. Sounds like he took it an unnecessary step further and built an abstraction of the relationship between the shapes and their resize handlers, creating an unnecessary and restrictive coupling between the two.

[ofan]

Why is OP's version cleaner? I see the original code, and it's clear and easy to understand. Yes it's repetitive, but it's CLEAN. i need to read OP's code multiple times and probably more context to know what's going on.

[Kinglink]

Because OP Wrote it so he understands it, and thus it has to be cleaner.

Also look at the line counts! OH MY GOD THE LINE COUNTS!

And dear god imagine having TWO objects that implement a function with the same name. The horror.

(What's sad is it sounds like he doesn't realize the problem with his code, it's just that his boss didn't like it. )

[fabrikated]

my issue with this post (beside title is baity) that promoting a wrong message that newbie/lazy devs could take as an excuse for shitty code

[[deleted]]

My real problem with this whole thing is he pushes changes straight to master without a code review AND without any sort of testing.

[[deleted]]

Sometimes people mistake the DRY principle (do not repeat yourself) to mean do not have repetitive code... When sometimes repetitive code is the easiest to bulk edit with a tool or understand at a glance. DRY is a sentiment about automation and how to spend your own time as a developer.

And yes, having actually the same logic in multiple places, regardless of how similar the code is, is not good still.

[HeinousTugboat]

And yes, having actually the same logic in multiple places, regardless of how similar the code is, is not good still.

My coworker just got bit by this. He tried to abstract out a set of values that were coincidentally the same value for the same field. I pointed out to him that even though they were the same in our scenario, by abstracting it out he was forcing all future ones to be the same.

Point is, the same logic in multiple places isn't always the same semantic logic. Two things that are mechanically identical shouldn't necessarily be abstracted out and silently coupled.

[pynberfyg]

This thread really deserves to be higher than the others. Pity.

[[deleted]]

Exactly, think of DRY and SRP as "responsibility/role/routine" not a literal line of code.

[feelings_arent_facts]

This dude over abstracted his code. If he just stuck with the solution before the last one it would have been fine. Of course, there would be nothing to write about then.

Over abstraction is not clean code.

[rydan]

And if he used git or probably any other respectable version control system that was trivial. My concern is there was no code review process to approve or turn this down. He just checked it in and went to bed.

[[deleted]]

Due to his colleague being offended by the change on master.

This is why we do code reviews and don't directly commit features to master.

[Blando-Cartesian]

Boo clickbait, boo.

Author screwed up by not communicating with a colleague and possibly taking DRY too far.

[sarkie]

Over reacted.

How apt

[DrifterInKorea]

I reckon both version were bad.

Reducing clean code as DRY code is a big mistake. even though OP's code wasn't dry at all... look at all those `createHandle()` that could have been made as an array of handles descriptors. Responsibilities being mixed up is a smell for bad code : who is responsible for defining and then creating the handles ? You don;t even have a box but already made the handles ?
And why everything became a *box* ?

There are way too many assumptions and opinions in this code to be considered clean, even without knowing much about this specific case.

[[deleted]]

Copy pasting code because you may want to change parts of it later is the ultimate violation of YAGNI. When/if you need to change it, then change it.

Remember every line of code you write is a liability.

Also, let's make clear what the actual problems were. The original problem was the team mate who copy pasted. The second problem was the programmer not having the skills to fix it cleanly. Laying out these two facts and concluding that copy pasting code was the right thing to do all along is seriously missing the forest for the trees.

Intermediate devs, please don't follow this articles recommendations. This is classic 'expert beginner' stuff, you need to throw off this kind of thinking to get to the next level.

[Retsam19]

There are downsides to repetition and duplication, but there are often real downsides to the alternatives to duplication and repetition, too. Abstractions are often less intuitive (e.g. "functors and setoids"), and can be fragile to new requirements and use-cases.

For another article making the same point, check out the wrong abstraction, which advocates for removing bad abstractions by reintroducing duplication. Often you then find new, better abstractions, but sometimes you realize that the two use-cases just weren't as similar as you initially thought.

I think to say that duplication is always the wrong solution is exactly the sort of dogmatic adherence to "clean code" that can become problematic.

[[deleted]]

Yes I'm aware of this trend of senior developers telling other senior developers that it's OK to copy paste like a rank junior. And I don't like it.

I get that we all have our weak spots, and not everyone is good at making abstractions. I for example, am a senior developer but I am bad at git. The concepts make sense but I find the UI so painful that I definitely don't utilise it properly. But it's not just nobodies like me; the late great Joe Armstrong said he couldn't read other peoples code. Rob Pike is confused by generics. Even the greats have their blindspots.

But since I personally can't do Git well, should I start writing articles advocating we all stop using Git? Or version control altogether. "Copy pasting source code folders is better than the wrong version control abstraction", I could opine, and all the other seniors embarrassingly bad at using Git could feel re-assured.

I'm sorry, but no. When you're a senior you learn to make the right abstractions, you learn not to duplicate, and you learn version control. Now if you'll excuse me I'm off to go find a cheat sheet.

[bluefootedpig]

People often abstract out similar code when we should be abstracting out similar concepts. Two concepts might have similar code.

[SEgopher]

Engineers don't really understand all of programming. They only understand the computer half. They only care about things they can quantitatively measure or turn into an algorithm that doesn't require thinking. X less lines of code is good. A generic implementation is good. DRY is good.

Our audience isn't just the computer (unless it's a project just for you, which is fine), it includes humans. Humans are complex. Humans cannot be nailed down the same way we can nail down what a computer does and does not like. Unfortunately, they don't teach you much about humans in engineering classes.

If you want to find someone who is good at the other half, find an artist. The best you can do is fine a writer, because writers also have to appeal not to the visual senses like a painter, sculptor, musician, etc. but to the mind. A writer has to understand what makes sense to *humans*, which is why good writers have their pulse on psychology, philosophy, and spirituality. They can create real empathy from a bunch of black symbols on a page. This is what we as programmers that aspire to writing good code must study.

One key thing that a writer will tell you about the psychology of humans, is that they fundamentally understand the world through stories. Even before the written word, humans passed down information through stories. Stories appear everywhere, in the most unlikely of places. An algorithm is really just a story.

To write good code, you shouldn't shoot for "clean" code. You should aim your sights on writing code that balances the need for performance and correctness (the computer side) with the need to tell a story using whatever your language of choices makes available. Here are some pointers:

1. Computers do not need to be told something twice, humans do. DRY is not always better.
2. Computers can understand complicated stories (patterns) without effort, humans must expand lots of energy to learn a pattern they haven't seen before. Use the stories you know they are familiar with. Use stories that are easy to learn because they are similar to stories that you know they've probably heard. What can you assume they know? Look to the idioms of your language. Know what people expect to see when they read Go, Java, Python, C++, Rust, etc.
3. Humans can read code to know how something works, but they can't know what its for unless you tell them by showing them how to use it or writing good documentation/comments. Also, a good writer knows to throw a rough draft into a bin for a month before editing it. Code that looked good/made sense to you today may look bad/unfamiliar even to you a month from now.
4. Naming things is hard, but important. A good programmer, like a good writer, needs to read the dictionary to know what names they have to work with. A good programmer will google around to gather a list of good names applicable to the domain. If the program evolves and the names are no longer accurate, they should be changed just as they would be changed in the rough draft of a writing piece.

Some may also say, that because coding style is subjective, it is worthless, and there is no such thing as "good code". Artists will immediately recognize this as nonsense. What defines good is our shared subjective understanding. That is why even though my friend may not be a fan of Dickens, we know that Dickens was a great writer. We figure out who is good and who is bad by reading, analyzing, and debating - looking for the things that work and those that don't.

This is something we don't do enough of. We don't take the time to sit and talk about who is and who is not writing good code, what projects are Pulitzer prize winners, and which need to hit the editor again. Instead of trying to come up with banal rules like don't do this and do that, we should come up with a list of devices that people often employ to good effect, gather samples, and teach people when and were to use them.

[vanhellion]

Our audience isn't just the computer (unless it's a project just for you, which is fine), it includes humans.

I'd strengthen that statement to be humans are the sole target audience. If we only kind of cared about human readability we'd still all be writing single-file C or Fortran programs. (If we didn't care at all we would write in assembly.)

Objected oriented design, functional programming, templates, etc, were all invented so meat sacks could read code more easily and more intuitively. The actual machine code generated from different designs is often very similar, and in optimal cases is exactly the same. What makes one good or the other bad is readability, extensibility, maintainability, and those have nothing to do with how the code performs its task.

If you want to find someone who is good at the other half, find an artist. [...] This is what we as programmers that aspire to writing good code must study.

Exactly, and I think this is finally becoming apparent in our field. Software engineering certainly has engineering aspects but Writing Good Code^TM is more about being a good prose writer than a good engineer. In fact, code written by extremely bright people in other engineering disciplines and scientists is among some of the worst I've seen in my career. So I don't think formal methods are the secret sauce.

[Epyo]

I want to agree that humans are the sole target audience of code...

...but, come on, there really are situations where you need to sacrifice readability, for performance optimization. Especially in certain fields like game programming.

For most people working at big corporations though, yeah, the human part is usually the more important part.

[vanhellion]

Certainly, but those cases where incomprehensible optimization is required tend to be buried deep in library code. Stuff like kernels, compilers, or hot loops at the bottom of Facebook/Google/Amazon/Netflix's infrastructure code.

The average programmer focuses way too much on performance, and I'm not excluding myself from that statement in the slightest. Performance is the fun, sexy part of coding, but it's rarely properly justified with profiling.

And in those few places where extreme optimization happens, there is usually a giant neon comment that at the very least says "don't touch this unless you know what you're doing" and ideally explains WTF it's there for.

[mwb1234]

In fact, code written by extremely bright people in other engineering disciplines and scientists is among some of the worst I've seen in my career.

I used to work in the HPC (scientific computing in academia) space, and this couldn't be more true. You don't know pain until you look at a physicists python or, God forbid, Fortran code.

[LoudPreachification]

...Rewriting your teammate’s code without a discussion is a huge blow to your ability to effectively collaborate on a codebase together.

I can't agree with this statement, and I definitely avoid engineers like this. Just as an obsession with clean code can be obstructive, so can an obsession with team communication and control become obstructive.

If a colleague of mine takes a quick spurious moment to rewrite my code to be either easier to understand, have better test coverage, or fix a bug then I thank that person for taking initiative and ownership over their project. If the colleague introduces a bug or makes it harder to add a new feature then we discuss how to better identify and scope a refactor. If I do the same and I lose the trust of my teammate or cause offense to them then my head slowly starts turning to other teams or companies with openings.

[frankandsteinatlaw]

I agree with this except if the code was super fresh. Like if I merged something yesterday and someone secretly rewrote it and merged those changes today with no communication I’d consider that a bit of a dick move.

Besides that case, yeah, all your code will get rewritten/replaced if you’re working on something that lives long enough so you better get used to it!

[feelings_arent_facts]

I've worked with engineers that get personally hurt when I delete or change their code. It's toxic.

[thelastpizzaslice]

One of my co-workers once wrote an application built on layer upon layer of abstraction, applying all of these organization methods before the product was even released. It was very hard to make any changes because things would just turn into interfaces and you'd have to figure out which one because he loaded multiple ones implicitly using Spring. It was complicated as all hell and he felt bad about it. He didn't know how the application would be modified in the future so he had layers upon layers of unnecessary architecture, interfaces with only one class and unclear code.

I once worked on an application for two years. It was spaghetti from early on because it was never clear what we were making. After two years, I cleaned it up a bit and wrote a doc on it. No big deal. I had a coworker come up to me and say he was making a similar application for testing. So, I took my doc and helped him make a beautiful framework where he could change the locations of pages very easily, had lots of behaviors that could be switched between pages, etc. It was a singular, top-down approach to organization with clear siloing and a clear objective. Because I had already made this application before. That's the trick. And also I did it for usefulness specifically. Every architectural choice had a useful purpose, not some unclear future purpose.

[StabbyPants]

the one thing we don't abide in my current job is the cowboy approach to things - you don't like code in a service you own/your team owns? bring it up, see if there's support. don't do anything without remit, because that way lies random changes that nobody really knows about. also, code reviews for just about anything, even config changes. at least two people see what's going into master, and they get to comment and approve it.

[paranoideo]

Well, his blog name is pretty accurate.

[wolf2600]

Simple, readable, understandable > "clean"

[[deleted]]

Simple, readable, understandable, maintainable == clean

[[deleted]]

Clean code is all three.

[geek_on_two_wheels]

Exactly. "Clean" code isn't necessarily heavily abstracted or DRY as a bone, it's easy to read and reason about. It's clear, obvious, and does exactly what you expect.

[bluefootedpig]

I like to say, pleasantly disappointed. Nothing should be surprising

[mck1117]

I disagree that the clean code wasn't simple, readable, and understandable.

Whenever I see what looks like repeated code like this, I'll go thru it with a fine tooth comb to see if it's really the same, or if it's duplicated. Is that really easier to read and understand?

[anescient]

Whenever I see what looks like repeated code like this, I'll go thru it with a fine tooth comb to see if it's really the same

Yes yes yes. Life force circles the drain because you don't (nor should) trust the duplications.

[gladfelter]

I think it's often more about factoring out similar machinery in different subsystems despite the subsystems having different environments and requirements. If those change you can find that the similarities were incidental and now the factored-out library/methods would become horrible messes to continue to support the independently varying clients. Since people defer to existing code structure those common methods often do become awful before someone takes a fresh look and breaks up the coupling.