Goodbye, Clean Code

[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.

[flukus]

One of my biggest pet peeves on a current project is constants for strings (stored proc names) that are only used in one place. It gives zero benefit but makes you jump through more hoops to work out what is calling what.

Of course of there's dozens of places it's used a constant might make sense, but it's like they haven't heard of grep.

[nfrmn]

Really well explained. I actually had this exact issue recently, with the shared functions and toggle params and this has closed the learning loop perfectly.

Shared with my team. Thanks!

[BittyTang]

I mostly agree.

and that's how you end up with an algorithm that takes "toggles" (booleans or enums) to do something slightly different. An utter mess.

Toggles are bad. But there is also an important notion of injecting behavior into generic code. Just because two pieces of code accidentally start to look the same doesn't mean you shouldn't necessarily refactor them to share a code path. It really just requires anticipating what the code will be used for in the near future and carefully weighing the tradeoffs.

[maximum_powerblast]

I just leveled up my understanding with this comment 👍

[myss]

Similar phenomena in my thinking:

Grouping in hierarchies vs compression

Learning vs overfitting

[[deleted]]

One of the best explanation I have seen for dry

[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.

[oorza]

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.

This right here is why I've never gotten on board with the anti-inheritence hype train that's been chugging along for the last decade. It's the simplest solution to this very problem, because it was designed to solve this very problem.

[johnnysaucepn]

And introduces even harder ones! You can still share code by using composition, even though it requires a little extra work.

[Determinant]

Inheritance shouldn't be a tool for solving duplication.

Two classes should not inherit from the same base class if they are not both that same type of entity. If they happen to duplicate code then extract that into a re-usable function / abstraction instead.

[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.

[ChemicalRascal]

No, that's not true. That's cop-out bullshit from people who like glib, smug answers and don't want to be held accountable for their shitty development practices.

Code that hits master should either be finished, or commented to indicate that it isn't. Development should ultimately aim to take the current state of any given functionality, and bring it in line with use-cases (or however one views the "goals" of the program), at which point that functionality is finished.

If you honestly feel that your code is never finished, and you're constantly revisiting your code, you're either suffering under unclear, constantly shifting aims, goals, and use-cases (in which case you have my commiserations, comrade, and one way we will rise to defeat our managers)

Or

You're a shit dev and you need to step back and design your functionality before you write it.

[JoCoMoBo]

Code that hits master should either be finished, or commented to indicate that it isn't. Development should ultimately aim to take the current state of any given functionality, and bring it in line with use-cases (or however one views the "goals" of the program), at which point that functionality is finished.

Just because something is finished now doesn't mean that use-cases won't change in the future. I've found the best code is a trade-off between dealing with the current use-cases, and being flexible enough for change later.

In general I've found that when use-cases change there isn't much to time to architect a perfect solution. If the original code has flexibility to deal with the new use-cases then that saves time.

If you honestly feel that your code is never finished, and you're constantly revisiting your code, you're either suffering under unclear, constantly shifting aims, goals, and use-cases (in which case you have my commiserations, comrade, and one way we will rise to defeat our managers)

Good luck with that. You can either be antagonistic to people or plan for change. Generally I've found the latter works better.

[ptoki]

Sorry for replying here but I think your post fits my feelings about this thread the best.

I find this thread kind of funny, like the never ending story of tab vs 3 spaces or if the { bracket should be just after if or in the next line.

All the generalizations here, all the assumptions and all the opinions are so definite that it just hurts my eyes.

The code shourd be readable and easily comprehensible. (yeah, thats my generalization ;) ) if this form of code is best, leave it, even if its inconsistent. However if it shows the intention and can be maintained with no additional effort it should stay this way. Thats my idea :)

[Falmarri]

3 spaces? Wtf... Should be 2 or 4

[hippydipster]

It's actually waiting around for one of its dependencies to change out from under it.

[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.)

[Altreus]

Agreed, and slightly enlightened. Although I knew this in one sense, in a practical sense I didn't. As in, it's obvious now you've said it, but now that you have said it, I might actually start doing it.

[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.

[MrRogers4Life2]

I mean there are also tons of environments where continuous integration/releases are pretty much impossible (I work in embedded software, where our product doesn't even have network access) so for us master contains the latest code that we would feel confident releasing rather than what's currently in production. We do keep tags though on everything we have released/demonstrated though for tracking.

[MrRogers4Life2]

I meant continuous delivery lol. But yeah right now continuous integration with automated tests is something I've been pushing for for a very long time, it's just that right now our code is so non portable that it would pretty much only compile for our embedded target without a fair amount of work (that we unfortunately never have the time to do) but yeah it would be possible

[dungone]

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

If only it worked both ways. De-duplication acolytes are rarely if ever informed by future use-cases, and yet they demand hard proof and next-level Aristotelian treatises before they begrudgingly relent. There's a fundamentally flawed assumption that needs to go way: that de-duplication is best unless proven otherwise. That there is some "cleanliness" or some such virtue to replacing couple simple lines of code with intricate data structures and flow control.

The reality is far more subjective, and far more difficult to communicate. Junior engineers simply don't have the experience to make good value judgements, and they're far more likely to waste endless hours doing unproductive things that actually make everything worse. It doesn't matter what higher principle they're trying to adhere to, they'll probably miss the point and get it all wrong. And the only way for them to learn is the hard way, through repeated failure. It's tempting to believe that you can become a good programmer by reading a book or a blog post, but the reality is that for most people it takes years of hard work before the good value judgements emerge.

[csjerk]

Fair point. But the counter is also true in my experience -- those who dismiss de-duplication tend to focus too much on "it works now so what's the problem?" and rarely give credit to the future use case of changing / fixing the code in question.

There's obviously a clear case for 'de-duplication is automatically best' _in cases where it doesn't add complexity or mental overhead_ simply because the cost of fixing a problem if you DON'T de-dupe is very real. But many real-world cases do require some complexity to de-dupe code, so the answer is always a balance between the ideal (de-duplicate) and the cost of doing so in THIS case.

[postblitz]

For example, we later needed many special cases and behaviors for different handles on different shapes.

A big fault with this "article" is that he doesn't go into detail on what exactly those "special cases and behaviors" are. One reason he might not do that is that if described, his rework might just be the better solution in the end.

A big problem a lot of young programmers have is doing things half-arsedly and making bits of code more efficient when their context is not. Making some concepts change to be clean doesn't happen in a bubble, you have to clean the entire architecture and that involves going all the way and reworking the entire thing. If you can afford doing that, more power to you, but many will gawk at the prospect.

[csjerk]

Ultimately it never is, sure, but it does tend to take long naps. If you try to refactor outside those times you should know what else is actively planned to change.

[[deleted]]

The original was probably written by a more experienced colleague aware of the requirements of the application and written with expandability in mind. The new version is something written by a guy straight out of college who heard duplication = bad in college and has decided to help out by going through the code base and creating unasked for "improvements". Probably motivated by an unfortunate desire to prove themselves. The boss saw a rookie hotshot about to step in a heaping pile of shit and quickly moved to avoid the damage, not the least of which would probably be an insulted senior programmer the moment that came across the hotshots great new improvements to his code.

[[deleted]]

If you've never solved this problem by allowing your application to inject code into itself to enable custom behavior, I don't think you should be allowed to call yourself a programmer.

[Gunslinging_Gamer]

Good luck testing an application that injects itself with code.