9

I'm facing problems with what I feel is too much abstraction in the code base (or at least dealing with it). Most methods in the code base have been abstracted to take in the highest parent A in the codebase, but the child B of this parent has a new attribute that affects the logic of some of those methods. The problem is that those attributes can't be checked in those methods because the input is abstracted to A, and A of course doesn't have this attribute. If I try to make a new method to handle B differently, it gets called out for code duplication. The suggestion by my tech lead is to make a shared method that takes in boolean parameters, but the problem with this is that some people view this as "hidden control flow," where the shared method has logic that may not be apparent to future developers, and also this shared method will grow overly complex/convoluted once if future attributes need to be added, even if it is broken down into smaller shared methods. This also increases the coupling, decreases cohesion, and violates the single responsibility principle, which someone on my team pointed out.

Essentially, a lot of the abstraction in this codebase helps reduce code duplication, but it makes extending/changing methods harder when they are made to take the highest abstraction. What should I do in a situation like this? I'm at the center for blame, even though everyone else can't agree on what they consider good, so it's hurting me in the end.

  • 10
    add a code sample to illiterate the ""problem"" would help to understand the situation a lot more – Seabizkit Sep 22 '19 at 8:34
  • I think there are two SOLID principles broken here. Single Responsibility - if you pass in a Boolean to a function which is supposed control the behavior, the function will no longer have a single responsibility. The other one is Liskov Substitution principle. Imagine there is a function that takes in class A as parameter. If you pass in class B instead of A, will the functionality of that function be broken? – bobek Sep 24 '19 at 13:37
  • I suspect method A is pretty long and does more than one thing. Is that the case? – Rad80 Sep 27 '19 at 8:45
27

If I try to make a new method to handle B differently, it gets called out for code duplication.

Not all code duplication is created equal.

Say you have a method that takes two parameters and adds them together called total(). Say you have another one called add(). Their implementations look completely identical. Should they be merged into one method? NO!!!

The Don't-Repeat-Yourself or DRY principle is not about repeating code. It's about spreading a decision, an idea, around so that if you ever change your idea you have to rewrite everywhere you spread that idea around. Blegh. That's terrible. Don't do it. Instead use DRY to help you make decisions in one place.

The DRY (Don't Repeat Yourself) Principle states:

Every piece of knowledge must have a single, unambiguous, authoritative representation within a system.

wiki.c2.com - Don't Repeat Yourself

But DRY can be corrupted into a habit of scanning code looking for a similar implementation that seems like it's a copy and paste of somewhere else. This is the brain dead form of DRY. Hell, you could do this with a static analysis tool. It doesn't help because it ignores the point of DRY which is to keep the code flexible.

If my totaling requirements change I may have to change my total implementation. That doesn't mean I need to change my add implementation. If some goober smooshed them together into one method I'm now in for a bit of needless pain.

How much pain? Surely I could just copy the code and make a new method when I need it. So no big deal right? Malarky! If nothing else you cost me a good name! Good names are hard to come by and don't respond well when you fiddle with their meaning. Good names, that make intent clear, are more important than the risk that you copied a bug that, frankly, is easier to fix when your method has the right name.

So my advice is to stop letting knee jerk reactions to similar code tie your codebase in knots. I'm not saying you're free to ignore the fact that methods exist and instead copy and paste willy nilly. No, each method should have a damn good name that supports the one idea that it's about. If its implementation happens to match some other good idea's implementation, right now, today, who the hell cares?

On the other hand if you have a sum() method that has an identical or even different implementation than total(), yet ever time your totaling requirements change you have to change sum() then there's a good chance these are the same idea under two different names. Not only would the code be more flexible if they were merged, it'd be less confusing to use.

As for Boolean parameters, yes that's a nasty code smell. Not only is that control flow stuff a problem, worse it's showing that you've cut in an abstraction at a bad point. Abstractions are supposed to make things simpler to use, not more complicated. Passing bools to a method to control its behavior is like creating a secret language that decides which method you're really calling. Ow! Don't do that to me. Give each method its own name unless you have some honest to gosh polymorphism going on.

Now, you seem burned out on abstraction. That's too bad because abstraction is a wonderful thing when done well. You use it a lot without thinking about it. Every time you drive a car without having to understand the rack and pinion system, every time you use a print command without thinking about OS interrupts, and every time you brush your teeth without thinking about each individual bristle.

No, the problem you seem to be facing is bad abstraction. Abstraction created to serve a different purpose than your needs. You need simple interfaces into complex objects that let you request that your needs be fulfilled without ever having to understand those objects.

When you write client code that uses another object you know what your needs are and what you need from that object. It doesn't. That's why client code owns the interface. When you're the client nothing gets to tell you what your needs are but you. You put out an interface that shows what your needs are and demand that whatever is handed to you fulfill those needs.

That is abstraction. As the client I don't even know what I'm talking to. I just know what I need from it. If that means you have to wrap something up to change its interface before handing it to me fine. I don't care. Just do what I need done. Stop making it complicated.

If I have to look inside an abstraction to understand how to use it the abstraction has failed. I shouldn't need to know how it works. Just that it works. Give it a good name and if I do look inside I shouldn't be surprised by what I find. Don't make me keep looking inside to remember how to use it.

When you insist that abstraction works this way the number of levels behind it doesn't matter. So long as you're not looking behind the abstraction. You're insisting that the abstraction conforms to your needs not adapting to its. For this to work it has to be easy to use, have a good name, and not leak.

That's the attitude that spawned Dependency Injection (or just reference passing if you're old school like me). It works well with prefer composition and delegation over inheritance. The attitude goes by many names. My favorite one is tell, don't ask.

I could drown you in principles all day. And it sounds like your coworkers already are. But here's the thing: unlike other engineering fields this software thing is less than 100 years old. We're all still figuring it out. So don't let someone with a lot of intimidating sounding book learning bully you into writing hard to read code. Listen to them but insist they make sense. Don't take anything on faith. People who code some way just because they were told this-is-the-way without knowing why make the biggest messes of all.

| improve this answer | |
  • I wholeheartedly agree. DRY is a three-letter-acronym for the three-word-catchphrase Don't Repeat Yourself, which is in turn a 14 page article on the wiki. If all you do is blindly mumbling those three letters without reading and understanding the 14 page article, you will run into trouble. It is also closely related to Once And Only Once (OAOO) and more loosely related to Single Point Of Truth (SPOT) / Single Source Of Truth (SSOT). – Jörg W Mittag Sep 22 '19 at 13:58
  • "Their implementations look completely identical. Should they be merged into one method? NO!!!" – The converse is also true: just because two pieces of code are different doesn't mean they aren't duplicates. There's a great quote by Ron Jeffries on the OAOO wiki page: "I once saw Beck declare two patches of almost completely different code to be "duplication", change them so that they WERE duplication, and then remove the newly inserted duplication to come up with something obviously better." – Jörg W Mittag Sep 22 '19 at 14:00
  • @JörgWMittag of course. The essential thing is the idea. If you're duplicating the idea with different looking code you're still violating dry. – candied_orange Sep 22 '19 at 14:22
  • I have to imagine a 14 page article on not repeating yourself would tend to repeat itself a lot. – Chuck Adams Oct 15 '19 at 4:51
7

The usual saying that we all read here and there is:

All problems can be solved by adding another layer of abstraction.

Well, this is not true ! Your example shows it. I’d therefore propose the slightly modified statement (feel free to reuse ;-) ):

Every problem can be solved by using THE RIGHT level of abstraction.

There are two different problems in your case:

  • the over-generalization caused by adding every method at the abstract level;
  • the fragmentation of the concrete behaviors that lead to the impression of not getting the big picture and feeling lost. A little bit like in a windows event loop.

Both are corelated:

  • if you abstract a method where every specialization does it differently, everything is fine. Nobody has a problem grasping that a Shape can calculate its surface() in a specialised way.
  • If you abstract some operation where there is a common general behavioral pattern, you have two choices:

    • either you’ll repeat the common behavior in every specialisation: this is very redundant; and difficult to maintain, especially to ensure that the common part is stays in line across the specializations:
    • you use some kind of variant of the template method pattern: this allows you to factor in the common behavior by using additional abstract methods that can be easily specialized. It’s less redundant, but the additional behaviors tend to become extremely split. Too much would mean that it’s perhaps too abstract.

In addition, this approach might result in an abstract coupling effect at the design level. Every time you want to add some kind of new specialized behavior, you’ll have to abstract it, change the abstract parent, and update all the other classes. That’s not the kind of change propagation that one may desire. And it’s not really in the spirit of abstractions not depending on specialization (at least in the design).

I don’t know your design and cannot help more. Perhaps it is truly a very complex and abstract problem and there is no better way. But what are the odds ? The symptoms of overgeneralisation are here. May be it’s time to look at it again, and consider composition over generalisation ?

| improve this answer | |
5

Whenever I see a method where the behavior switches on the type of its parameter, I immediately consider first if that method actually belongs on the method parameter. For example, instead of having a method like:

public void sort(List values) {
    if (values instanceof LinkedList) {
        // do efficient linked list sort
    } else { // ArrayList
        // do efficient array list sort
    }
}

I would do this:

values.sort();

// ...

class ArrayList {
    public void sort() {
        // do efficient array list sort
    }
}

class LinkedList {
    public void sort() {
        // do efficient linked list sort
    }
}

We move the behavior to the place that knows when to use it. We create a real abstraction where you don't need to know the types or the details of the implementation. For your situation, it might make more sense to move this method from the original class (which I will call O) to type A and override it in type B. If the method is called doIt on some object, move doIt to A and override with the different behavior in B. If there are data bits from where doIt is originally called, or if the method is used in enough places, you can leave the original method and delegate:

class O {
    int x;
    int y;

    public void doIt(A a) {
        a.doIt(this.x, this.y);
    }
}

We can dive a little deeper, though. Let's look at the suggestion to use a boolean parameter instead and see what we can learn about the way your co-worker is thinking. His proposal is to do:

public void doIt(A a, boolean isTypeB) {
    if (isTypeB) {
        // do B stuff
    } else { 
        // do A stuff
    }
}

This looks an awful lot like the instanceof I used in my first example, except that we are externalizing that check. This means that we would have to call it in one of two ways:

o.doIt(a, a instanceof B);

or:

o.doIt(a, true); //or false

In the first way, the call point has no idea what type of A it has. Therefore, should we be passing booleans all the way down? Is that really a pattern we want all over the code base? What happens if there is a third type we need to account for? If this is how the method is called, we should move it to the type and let the system choose the implementation for us polymorphically.

In the second way, we must already know the type of a at the call point. Usually that means we either are creating the instance there, or taking an instance of that type as a parameter. Creating a method on O that takes a B here would work. The compiler would know which method to choose. When we are driving through changes like this, duplication is better than creating the wrong abstraction, at least until we figure out where we are really going. Of course, I am suggesting that we aren't really done no matter what we've changed to this point.

We need to look more closely at the relationship between A and B. Generally, we are told we should favor composition over inheritance. This isn't true in every case, but it is true in a surprising number of cases once we dig in. B inherits from A, meaning that we believe B is an A. B should be used just like A, except that it works a little differently. But what are those differences? Can we give the differences a more concrete name? Is it not B is an A, but really A has an X that could be A' or B'? What would our code look like if we did that?

If we moved the method onto A as suggested earlier, we could inject an instance of X into A, and delegate that method to the X:

class A {
    X x;
    A(X x) {
        this.x = x;
    }

    public void doIt(int x, int y) {
        x.doIt(x, y);
    }
}

We can implement A' and B', and get rid of B. We've improved the code by giving a name to a concept that might have been more implicit, and allowed ourselves to set that behavior at runtime instead of compile time. A has actually become less abstract as well. Instead of an extended inheritance relationship, it is calling methods on a delegated object. That object is abstract, but more focused only on the differences in implementation.

There is one last thing to look at though. Let's roll back to your co-worker's proposal. If at all the call sites we explicitly know the type of A we have, then we should be making calls like:

B b = new B();
o.doIt(b, true);

We assumed earlier when composing that A has an X that is either A' or B'. But maybe even this assumption is not correct. Is this the only place where this difference between A and B matters? If it is, then maybe we can take a slightly different approach. We still have an X that is either A' or B', but it doesn't belong to A. Only O.doIt cares about it, so let's only pass it to O.doIt:

class O {
    int x;
    int y;

    public void doIt(A a, X x) {
        x.doIt(a, x, y);
    }
}

Now our call site looks like:

A a = new A();
o.doIt(a, new B'());

Once again, B disappears, and the abstraction moves into the more focused X. This time, though, A is even simpler by knowing less. It is even less abstract.

It is important to reduce duplication in a code base, but we must consider why the duplication happens in the first place. Duplication can be a sign of deeper abstractions that are trying to get out.

| improve this answer | |
  • 1
    It strikes me that the example "bad" code you are giving here is similar to what I would be inclined to do in a non OO language. I wonder if they learned the wrong lessons and brought them into the OO world as the way they code? – Baldrickk Sep 24 '19 at 9:05
  • 1
    @Baldrickk Each paradigm brings its own ways of thinking, with their unique advantages and disadvantages. In functional Haskell, pattern matching would be the better approach. Though in a language like that, some aspects of the original problem wouldn't be possible either. – cbojar Sep 24 '19 at 12:16
  • 1
    This is the correct answer. A method that changes implementation based on the type it operates on should be a method of that type. – Roman Reiner Sep 24 '19 at 17:31
0

Abstraction by Inheritance can become quite ugly. Parallel class hierarchies with typical factories. Refactoring can become a head-ache. And also later development, the spot where you are in.

There exists an alternative: extension points, of strict abstractions, and tiered customization. Say one customisation of government customers, based on that customisation for a specific city.

A warning: Unfortunately this works best when all (or most) classes are made extendale. No option for you, maybe in small.

This extendibility works by having an extendable object base class holds extensions:

void f(CreditorBO creditor) {
    creditor.as(AllowedCreditorBO.class).ifPresent(allowedCreditor -> ...);
}

Internally there is a lazy mapping of object to extended objects by extension class.

For GUI classes and components the same extendibility, partly with inheritance. Adding buttons and such.

In your case a validation should look whether it is extended and validate itself against the extensions. Introducing extension points just for one case add incomprehensible code, not good.

So there is no solution but trying to work in the current context.

| improve this answer | |
0

'hidden flow control' sounds too handwavy to me.
Any construct or element taken out of context may have that characteristic.

Abstractions are good. I temper them with two guidelines:

  • Better not to abstract too soon. Wait for more examples of patterns before abstracing. 'More' is of course subjective and specific to the situation which is hard.

  • Avoid too many levels of abstraction just because abstraction is good. A programmer will have to keep those levels in their head for new or changed code as they plumb the codebase and go 12 levels deep. The desire for well-abstracted code may lead to so many levels that they are hard for many folks to follow. This also leads to 'ninja maintained only' codebases.

In both cases 'more and 'too many' are not fixed numbers. It depends. That's what makes it hard.

I also like this write-up from Sandi Metz

https://www.sandimetz.com/blog/2016/1/20/the-wrong-abstraction

duplication is far cheaper than the wrong abstraction
and
prefer duplication over the wrong abstraction

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.