Too much abstraction making code hard to extend

abstractionclass-designcode-reusedryobject-oriented-design

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.

Best Answer

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

Related Solutions

C# Design Patterns – Using Abstract Base Class with Interfaces as Behaviors

[1] Add virtual "getters" & "setters" to your properties, I had to hack another control library, because I need this feature:

abstract class Control
{
    // internal fields for properties
    protected int _Width;
    protected int _Height;
    protected int _BackColor;
    protected int _X;
    protected int _Y;
    protected bool Visible;

    protected int BorderWidth;
    protected int BorderColor;


    // getters & setters virtual !!!

    public virtual int getWidth(...) { ... }
    public virtual void setWidth(...) { ... }

    public virtual int getHeight(...) { ... }
    public virtual void setHeight(...) { ... }

    public virtual int getBackColor(...) { ... }
    public virtual void setBackColor(...) { ... }

    public virtual int getX(...) { ... }
    public virtual void setX(...) { ... }

    public virtual int getY(...) { ... }
    public virtual void setY(...) { ... }

    public virtual int getBorderWidth(...) { ... }
    public virtual void setBorderWidth(...) { ... }

    public virtual int getBorderColor(...) { ... }
    public virtual void setBorderColor(...) { ... }

    public virtual bool getVisible(...) { ... }
    public virtual void setVisible(...) { ... }

    // properties WITH virtual getters & setters

    public int Width { get getWidth(); set setWidth(value); }

    public int Height { get getHeight(); set setHeight(value); }

    public int BackColor { get getBackColor(); set setBackColor(value); }

    public int X { get getX(); set setX(value); }

    public int Y { get getY(); set setY(value); }

    public int BorderWidth { get getBorderWidth(); set setBorderWidth(value); }

    public int BorderColor { get getBorderColor(); set setBorderColor(value); }

    public bool Visible { get getVisible(); set setVisible(value); }

    // other methods

    public Rectangle ClipRectange { get; protected set; }   
    abstract public void Draw();
} // class Control

/* concrete */ class MyControl: Control
{
    public override bool getVisible(...) { ... }
    public override void setVisible(...) { ... }
} // class MyControl: Control

I know this suggestion is more "verbose" or complex, but, its very useful in real world.

[2] Add an "IsEnabled" property, do not confuse with "IsReadOnly":

abstract class Control
{
    // internal fields for properties
    protected bool _IsEnabled;

    public virtual bool getIsEnabled(...) { ... }
    public virtual void setIsEnabled(...) { ... }

    public bool IsEnabled{ get getIsEnabled(); set setIsEnabled(value); }
} // class Control

Means you may have to show your control, but don't show any information.

[3] Add a "IsReadOnly" property, do not confuse with "IsEnabled":

abstract class Control
{
    // internal fields for properties
    protected bool _IsReadOnly;

    public virtual bool getIsReadOnly(...) { ... }
    public virtual void setIsReadOnly(...) { ... }

    public bool IsReadOnly{ get getIsReadOnly(); set setIsReadOnly(value); }
} // class Control

That means the control can display information, but cannot be changed by the user.

Programming – Can Too Much Abstraction Be Bad?

The very first words of TC++PL4:

All problems in computer science can be solved by another level of indirection, except for the problem of too many layers of indirection. – David J. Wheeler

(David Wheeler was my thesis advisor. The quote without the important last line is sometimes called "The first law of Computer Science.")

Best Answer

Related Solutions

C# Design Patterns – Using Abstract Base Class with Interfaces as Behaviors

Programming – Can Too Much Abstraction Be Bad?

Related Topic