Hacker News Reader

Comment by Tainnor | Hacker News Reader

I feel some people here are misunderstanding the blog post.

Sandi Metz IMHO doesn't claim that the problem occurs at step 2 or 3. She doesn't claim that it's wrong to introduce abstraction when there is duplication.

What she is saying instead is that the problem occurs from step 6 onwards: when you find yourself wanting to reuse an abstraction that, regardless of whether it made sense in the first place or not, has outlived its usefulness.

I think this is in agreement with other points that she often makes, about being bold, but methodical about refactorings.

The whole discussion about "you should never abstract away code before you see the third duplication" has little to do with the article, and I'm also really not sure it's good advice.

View on HN

> What she is saying instead is that the problem occurs from step 6 onwards: when you find yourself wanting to reuse an abstraction that, regardless of whether it made sense in the first place or not, has outlived its usefulness.

You're 100% correct in this. And what's even more amazing to me is that even after you explicitly calling this out, the majority of people replying to you (and presumably have read the article) still think the problem is between 2 & 3.

The argument she is making is not "don't make abstractions until you're 100% certain they are correct". She is essentially saying make abstractions where appropriate. Some of these abstractions will be wrong. When you start seeing yourself making certain behaviors it's probably because it's the wrong abstraction, so back it out and refactor.

Ultimately that abstraction seemed right based on the info known at the time it was created, now that you know more don't try to cling to it because it was already made. Be ok with backing it out and refactoring.

If you see an abstraction does not fit, you have the choice to consider it incomplete or unsuitable. If incomplete, you can fix it (assuming write access). If unsuitable, you should "back it out" as you say.

In my opinion this distinction is applicable and thus useful in contrast to whining about leaky abstractions: http://beza1e1.tuxen.de/leaky_abstractions.html

It seems to me that a straightforward fixing of an incomplete abstraction is exactly what Sandi Metz warns against (i.e. steps 5+6). The abstraction is "almost perfect", so it should not fall in the "unsuitable" category.

It just so happens that complecting several slightly different uses in one abstraction comes at a significant cost. Backing out (inlining the abstraction, eliminating unused code) is a simple recipe to let you see the true amount of overlap, which may or may not itself be a suitable candidate for a smaller abstraction.

I rather see Sandis post as a criterium when an abstraction should be considered unsuitable: When you use only a small fraction of it because of conditionals.

I think that's probably correct in describing where you end up, but not any particular step along the way. It's one of those "the road to hell is paved with good intentions" situations.

When you first modify the abstraction, it's nearly perfect. Just one tiny conditional and it's a perfectly suitable abstraction again. The problem is, when this process repeats itself, you slowly get to the point where any one client of the abstraction is only using a small fraction of it, but there was never a singular point where someone made a decision to use the abstraction when it was anything less than "almost perfect".

yep.

Causes of this I've seen:

- sunk cost fallacy

- not spending enough time to understand the codes behaviour (and its side effects)

- code being written so it becomes hard to understand its behaviour and any side effects.

- trying to "win the PR game" by avoiding refactoring

- giving up on refactoring because thinking is too hard

Sandi Metz us not the only person with opinions on abstraction, nor is the article a definitive opus on abstraction, so it's not surprising people also include other ideas.

I think it's fair to say that abstractions should have to prove themselves as a necessity and that we make things abstract way too early. Most really good abstractions in an app fall out of well-written code to solve a specific problem.

In day to day life as an engineer, I find that we have very few _enduring_ abstractions - there are very deep ones, like the concept of streams, things like filesystems and related ideas, the concept of a virtual machine in the process sense, and so on - and a lot of faddish abstractions that have a pretty wide blast radius when they start to go wrong. A lot of the good ones (networking has a _lot_ of these, such as the abstractions above and below the model of an interface in professionally-written network device code) are focused on layering.

I disagree, and this is one of the things where it's really hard to get to a shared understanding because I don't know what kind of problems you've worked on and what kind of code you've seen and vice versa.

But in my daily work, I routinely see abstractions just come up very naturally all the time. Sometimes they turn out to be slightly wrong, but often also not. I need to perform some calculation (e.g. for billing)? That can be abstracted away. I need to parse some unstructured user request into something structured? That's an abstraction. And so on. A lot of these things are clear to me even before I start writing code.

I also tend to use (at least some amount of) DDD, to write small, composable functions with few side effects and to be as declarative as possible. All of this might help with coming up with lasting abstractions.

But I'm not denying that I'm running into lots of situations in my daily work where it turns out some abstraction was wrong. Just that I find that many more of them actually turn out to be correct or at least correct for the most part (it might be that something needs to receive an additional parameter or to returns some slightly different structure to account for error conditions or so, but it's still basically the same abstraction).

I sometimes review PRs where people are encouraged (by other reviewers) to create new methods because there's a single line of code duplicated between two other methods. I don't think that rate of abstraction construction - and every method is another abstraction - is helpful for the health of the code, or its readability.

I can't judge that without more context. If this is just accidental duplication, it's pointless to abstract it away. But if it's a line of code that is necessary to deal with some gotcha of a particular library etc., it's probably good to extract and add an explanatory comment.

“Accidental duplication” well said. Not enough people even consider this before deciding that they must remove the duplicate code. Will each duplicate section change for the same reason? If not then it’s not really duplicate code, it just looks the same.

If that line of code deals with a particular piece of business logic, that should be consistent within the application, it’s a good thing.

Why are your two examples in the second paragraph necessarily "abstractions"?

Because "calculate_vat" is more abstract than the exact sequence of calculations performed?

Are we talking about that kind of general function labelling abstraction?

I was under the impression this was about generalizing the abstraction to something reusable.

I.e. calculate_vat will always be used by the same thing and there will only ever be one of them.

The generalization would be:

    interface SalesTaxCalculator { 
        func calculate_sales_tax 
    }

This kind of abstraction is more uncommon.

What im guessing the article is struggling with is that making abstractions like salestax calculation os usually kind of useless, there is only 1-3 types of sales tax calculation and its best defined inline next to the proce calculation or put in a function like calculate_vat.

A lot of developers use abstractions to remove code duplication.

Its more fun using this kind of code than code where devs dont try to remove duplicstion but i just wish instead of seeing duplication as evil, they instead saw it a sign that their design is wrong.

If youre seeing duplication, there is an abstraction there, whether it is worth it to implement is usually more to do with if its just hiding code duplication or if it is creating a new concept that hides implementation details.

Yes, if you restrict your definition of abstraction to "polymorphism", then it becomes a different discussion (Go developers would like to have a word).

But I think there are lots of abstractions often missing from code that I have seen that don't need any sort of polymorphism. A common example is performing input validation all over the place instead of at the boundary with some sort of form object that only accepts valid params.

Im not restricting the definition to that at all.

Input validation, and any abstraction that smells like a framework or library make great code folds for abstraction.

The question is all about cost. Does a single layer abstraction, at the boundaries, with clear inputs and outputs have much of a cost?

I don't think so.

Does forcing vat calculation into a new class far away from the widget price class have a cost? I think so, the cost is having the same concern separated for no material gain other than perhaps reducing one form of duplication for the cost of creating another.

You may not think that input validation at the boundary has a cost, but the fact is that the "typical" Ruby on Rails application is happy to just pass controller params along to the model layer unchanged and unvalidated (there is a security layer that allows you to whitelist which parameters you want to forward, but nothing more than that). Validation is supposed to happen in the model layer. I think this is horrible design, but it's unfortunately a strong convention and breaking conventions also has associated costs. By contrast, if you write your application in e.g. Spring Boot you typically have validation at the controller level built in, so yes, in that case, it's easy to do the right thing.

I think you're right that it may not be necessary to extract "calculate_vat" into another class. It just really depends: if the VAT code is so complex that it should be properly unit tested, I think it deserves its own class. If it's fairly straightforward and only used inside of a single other price calculation class, then yeah, it can remain a private method or so.

But I don't think this is a question of whether to abstract or not, rather about what the right level of cohesion and coupling is for a given situation.

Does it make sense to characterise abstractions as “right” and “wrong” in the first place? This feels too absolute to me.

Abstraction is just hiding some complexity in implementation details behind a simpler interface. It offers benefits from reducing the need to deal with the full complexity everywhere else. It also has costs. The interface establishes a new concept, albeit a simpler one, that must also be understood and maintained wherever client code uses the abstraction. Moreover, if you need to understand or modify the detailed implementation later, there is now a barrier to doing so.

When we define an abstraction, hopefully we do so because the benefits outweigh the costs at that time. The simpler the interface relative to the complexity of the implementation it hides, the more likely this is to be true. However, that balance is inevitably subject to change as a program evolves and the relevance of the hidden details to different parts of the system changes.

So it feels like abstractions might be better characterised by whether they represent good value under the current circumstances. It is perfectly reasonable for an abstraction to be cost-effective at the time it is added, but to become more or less so as the context evolves. If it reaches a point where it is no longer cost-effective, it should be removed. Either the relevant parts of its implementation can then be inlined at each place that previously used it, or some new abstraction(s) can be defined that better reflect the relevance of different implementation details at that time.

I also (want to) understand the article this way, but I think the article is a bit vague about this point. It focuses on Programmer X, who is coming after many iterations of trying to retrofit features into a very wrong abstraction.

The article agrees that the abstraction was useful when Programmer A wrote it, but it remains vague about whether Programmer A's judgement was correct at step 2 or 3!

I'm not quite sure what Sandi Metz is thinking right now, but the original 2014 talk[1] linked in the article is pretty clear about _keeping the duplication_, and _waiting for the right abstraction_. This statement sounds more in line with the Rule of Three, or the Go proverb 'a little copying is better than a little dependency', which is what people are arguing here.

Personally, I believe that any non-trivial duplication should be eliminated at the very first chance, to avoid unintended business logic divergence. I find the later case more risky than having to deal with the wrong abstraction, since - as Sandi eloquently explains here - wrong abstractions can (and should!) be refactored.

[1] https://youtu.be/8bZh5LMaSmE?t=893

> but it remains vague about whether Programmer A's judgement was correct at step 2 or 3!

Which emphasizes the OP’s point that the article isn’t about what happens at steps 2 or 3 at all. So it’s not concerned with whether the judgment there was right or wrong.

Not to take this on a huge tangent, but I really _do_ think it's good advice. Unrolling complicated abstractions is a lot of work. Keeping two pieces of nearly identical code in sync is work too, but I've never found it all that onerous. But there's obviously a continuum; on one side it's obvious that it's a shared concept, and on the other, code just happens to be similar almost by chance, and not for much longer. But lately duplication has been turned into a code smell to be linted out, causing a lot of people to get rid of all of it, at all cost.

If you only have 2 instances but are spending effort keeping them both in sync for changes then that might be a good time to abstract anyway. The fact that you’re keeping them in sync means they aren’t just coincidentally the same. But this is very situational.

The cost of maintaining two (or three ...) copies might well be less than creating an abstraction. The danger rather lies in situations where not everyone potentially modifying that code (now or later) is aware that there are N copies (and where they are) which need to be maintained.

That's a big part of it for me. The abstraction would end up being the best way to document the duplication, imo. Far better than the likes of /* See also... */.

It depends of course, but I personally feel the work of 'simplifying an abstraction', is easier than the problem of 'tracking down anything that might need to be edited'.

Then such person needs better IDE, because finding callers is one shortcut away.

To be fair, Sandi suggests adding dup tags (essentially comments with links to other duplicates, I guess) in the linked video.

I can see how this could work in some cases, but I remain skeptical about this technique. I have a strong gut feeling that over 20% of the junior engineers touching this code will not understand the dup tag and will end up modifying one case without keeping the other case in sync. This number is already high enough to be worrisome for me.

That's an amazingly mechanical view of code.

If two blocks of code refer to exactly the same thing (event, process, object, rule) in the application domain, then the duplication can be eliminated.

You can't eliminate duplication without asking, "what does this code mean in this context?".

I think there are a couple of things at play here.

One is the use of code quality tools like CodeClimate. It's true that those can sometimes be extremely aggressive when it comes to duplicate code to the point that I find their complaints to be uselessely beside the point. This is especially true if you have typical "structural" duplication like "many controllers start with the same sequence of steps" etc. or is even worse when you have to use configuration DSLs etc.

OTOH, it has been my personal experience that many people, if they use CodeClimate etc., routinely just ignore them for the most part, so I'm not always sure what the point of them is. But maybe other people have different experiences and some people really are routinely overabstracting the most coincidental of duplication in which case I agree that that is not a very useful thing to do.

As for the advice itself: it is definitely problematic if it is used as some sort of hard "rule". If it is taken as a heuristic / "rule of thumb", then it might be ok as long as you make sure people don't overemphasise it where other/better rules of thumb would be appropriate.

For example, if I were to write some billing code and somebody else just duplicated that code somewhere instead of using a shared abstraction, I would probably find that to be a serious code health issue as you really shouldn't perform billing calculations in two separate places: this is something that needs to be kept in sync across the code base; one sibling (nephew?) comment is right in pointing out that here you have to consider the cost of things that need to stay in sync accidentally going out of sync.

There are many more examples which is why I think that if you use "refactor on 3" as _one_ heuristic, it's fine, but if it's the sole one, then less so.

> you should never abstract away code before you see the third duplication" has little to do with the article, and I'm also really not sure it's good advice

Absolutes like that are rarely good advice.

Sure, but some comments on here are literally saying that. Not as a rule of thumb (although such comments can be found here too, which is ok), but as a "as an engineer I always enforce this rule" thing.

Abstractions have other purpose than deduplication. They make it easier to reason about your code as well. It might be the smartest thing sometimes to abstract away the first occurrence in a method imho.

I think you generally shouldn't create an abstraction until you have at least three uses for it.

That's very generally. You might want to create abstractions before then, but be prepared that they will be wrong, and don't invest in e.g. lots of unit tests, because when you break the abstraction you'll throw away that work. Some unit tests yes, but more in semi-integration tests that verify the stack sandwiching the under-proven abstraction.

We humans just can't help ourselves, but to invent mental shortcuts. Making a judgment "is this really a good abstraction or am I just mindlessly deduplicating code" is context-dependent, nuanced and requires some mental effort - much more work than "do I have it repeating 2 or 3 times already" which is mindless and mechanical.

You are correct that the problem lies at point 6. However a problem only exists if programmer B decides that it is acceptable to keep adding conditional logic to the method. Wrong. This is just a case of programmer B not knowing how to refactor properly. There really is nothing else to it. Yes remove duplication. Then if later on, that duplication then requires conditional logic then refactor to replace the conditional logic with polymorphism. Both these steps are clearly defined on Martin Fowler’s Refactoring book and just need to be applied when the time comes.

I think the article has the premise that once you remove duplication you have committed to using that abstraction and need to shoehorn all future changes into that function. That’s a ridiculous notion. You can simply refactor it again if the abstraction no longer makes sense.

> I think the article has the premise that once you remove duplication you have committed to using that abstraction and need to shoehorn all future changes into that function. That’s a ridiculous notion. You can simply refactor it again if the abstraction no longer makes sense.

I don’t understand how you read this. The premise is exactly the opposite...

I'm not sure it matters about applying "prefer duplication over the wrong abstraction" at step 3 or step 6 nearly as much as applying that advice at some point.

I often consider "is this abstraction going to be prone to misuse" (regardless of if it's the second, third, fourth... copy) and try to head it off with either strict typing or comments or internal visibility - to try to do step 3 without opening up as big of a door to step 6, but the important thing is less when to do stuff like this but just to try to avoid things reaching step 7.

> Sandi Metz IMHO doesn't claim that the problem occurs at step 2 or 3.

But the headline does.

I had to read quite a long way down the page to discover that all she is advocating is what i have always done: deduplicate when practical, undo the duplication when new requirements make it incorrect and push the unique parts into the callers.

> But the headline does.

That’s not really fair, it repeatedly says “wrong abstraction”, in the title and in the article. At steps 2 and 3 it is still the right abstraction, duplication only becomes better when it is the wrong abstraction.

Sandi is a he.

I'm pretty sure that Sandi has identified as female in the past, but in case this has changed or I'm simply wrong, would you mind pointing me to a reference?

That’s not what I get from the article. The problem does indeed occurs at step 2 and 3: leave duplication alone and don’t introduce abstraction if you are not sure about future requirements.

Taken to its logical conclusion, doesn’t that argument mean we would almost never introduce any abstractions at all? That doesn’t seem very practical compared to the alternative of introducing abstractions if they are useful at the time but remaining willing to change or remove them again later if the situation changes.

Yes my comment was poorly worded it misses something like "unless you have a strong case for it". Also there was a title change on HN, previously it was "Duplication is far cheaper than the wrong abstraction", she also says "prefer duplication over the wrong abstraction" at the beginning, so with that emphasis I might have misinterpreted the rest of the point.

Reply on news.ycombinator.com