Design Patterns – Avoiding instanceof vs Abstract God Class

design-patternsobject-orientedpolymorphism

I have a large number of classes with an abstract base class (A) that contains behaviour that must be supported by all the sub classes say [B..I].

In my code, I end up with a collection of objects that is created based on input from an external system. These objects belong to subclasses described above and they go through various operations in my code.

All of these operations require some core and common behaviour so I use the abstract base class and I can call methods on a collection of objects using only the base type ( A.DoTheThing() ).

The problem is, only a small subset of these types must support a common set of functionality. If I add the methods to the base class with a default implementation that does nothing or returns null then I'm going towards a God class. Only a few subtypes would override these methods but I can keep using the base type to process the collection. The large group of subclasses would do nothing in response to method calls based on the default empty implementation in the base class. The ones that override the default behaviour would do what they need to do.

If I don't want to put behaviour where it mostly does not belong, I'd have to define an interface (X) and implement it for the small subset of subtypes that'll be included in the collection. However, I now have a collection based on the type A and at some point after using methods from A, I need to perform operations on the subset of objects that implement X. The only option I'm left with is to filter the collection based on instanceof(X) and call relevant methods.

Which one is the lesser evil and do I have another choice here?

Best Answer

Don't avoid things just because they are considered evil - understand first why they are considered evil, and then decide how to avoid that evilness.

You are warned about these evil things because they are usually the easy, straightforward way to solve the problem - otherwise people wouldn't always attempt to do them and the warning would be redundant. The problem is that if you don't understand why method A is evil, you may end up using method B which is evil for the exact same reason as method A, but because method B is more awkward than A it was not very popular so nobody felt the need to warn you of it.

I can't find it, but I remember seeing avoiding singleton by making all the class' fields static so that each new object of that class will use the same state(but there is no single instance so it's not a singleton!)

Anyways, this is what you are doing with that master base class. Downcasting is not "evil" in your case, but that master base class suffers from the same problem that downcasting usually suffers from!

Why is downcasting considered evil?

Consider this:

public abstract class Base {
    // bla bla bla
}

public class ImplA extends Base {
    // bla bla bla
}

public class ImplB extends Base {
    // bla bla bla
}

// somewhere else

void doit(Base base) {
    if (base instanceof ImplA) {
        ImplA implA = (ImplA)base;
        // ImplA specific code
    } else if (base instanceof ImplB) {
        ImplB implB = (ImplB)base;
        // ImplB specific code
    }
}

Why is this bad? Because it doesn't handle ImplC. But there is no ImplC!!!

Well, there is no ImplC now, but nothing stops me - or someone else - from writing it next year, making it extend Base. And then they will create an instance of ImplC, and that instance will be passed to doit - which will probably handle it wrong. Because, while we don't know what ImplC means and how doit should handle it, if doit needs special code for ImplA and special code for ImplB, we should assume it'll need special code for ImplC. But you may not be able to add that code(because doit may be part of a third party library), or you may simply not know you need to, because there is no compilation warning that doit does not have special code to handle ImplC. You'll realize eventually though, after a few hours/days of debugging trying to figure out why your program doesn't work...

This is why downcasting is frowned upon, and it's recommended to favor polymorphism and method overriding:

public abstract class Base {
    // bla bla bla
    public abstract void doit();
}

public class ImplA extends Base {
    // bla bla bla
    @Override
    public void doit() {
        // ImplC specific code
    }
}

public class ImplB extends Base {
    // bla bla bla
    @Override
    public void doit() {
        // ImplB specific code
    }
}

With this design, ImplC can have it's own override of doit with it's own specific code, and if you forget to write it you'll get a compilation error.

The problem with the master base class

I argue that your base class suffers from a similar problem - you need to modify the base class to add new functionality. It's not a worst case here, since you have access to the code and you won't forget to add the base methods because you need to use them. But still - you are avoiding downcasting by creating a construct that suffers from downcasting's general problem...

Downcasting is OK in this case

The problem with doit was that it was meant to deal with any Base, but in practice only handled the known types of Base - ImplA and ImplB.

Your case is different. You are looping over a collection of A, looking for, let's say, only instances of B, downcasting them to B and using them as B. This loop is not meant to handle all instances of A - it's only meant to handle the Bs in the collection. C, D, ... I will have their own loops, probably elsewhere, that deal with them. And if you add a new subclass J it'll also need new loop(s) - but writing these loops is a fundamental part of subclassing A, not some random method somewhere that needs to be amended.

What if you need to handle multiple subclasses in the same loop?

If you find yourself writing something like this:

for (A a : theBigCollection) {
    if (a instanceof B) {
        B b = (B)a;
        // B specific code
    } else if (a instanceof G) {
        G g = (G)a;
        // G specific code
    }
    // Other subclasses are not handled here
}

You are repeating doit's problem - if this loop needs to handle both A and G, how do we know it doesn't need to handle J?

In this case, you should try to bundle this behavior with a mid subclass or with an interface - let's call it X:

public interface X {
    // bla bla bla
}

public class B extends A implements X {
}

public class G extends A implements X {
}

// the loop from before
for (A a : theBigCollection) {
    if (a instanceof X) {
        X x = (X)a;
        // X specific code
    }
}

Now if J needs to, it can implement X and get handled in this loop. This is probably what you need, since you mentioned a small set of subclasses implementing the same methods. You can have multiple such interfaces if you need to.

Epilogue

The problem with downcasting is the possibility of adding new subclasses that will go through the same code paths but won't have specialized code that handles them. So when considering whether or not you should use downcasting always think what will happen if someone adds a new subclass.

I believe this is a good place to use the wider interpretation of the Zero-One-Infinity Rule. In any code "unit", you should handle zero, one or infinite(==all possible) subclasses of A:

zero: You don't use A in that code. Not interesting
one: One subclass, or one interface that you try to cast to. But not "one" as in "I have one apple" - it should be one as in "God is one". It should be conceptually impossible to add alternative downcasts, no matter what other subclasses will be added there you are still allowed to change the one type to some interface - as long as it remains one.
infinity: You deal with all possible subclasses of A - which means you don't downcast and use A itself.

Note that there may be multiple levels - in my last example I was first downcasting to X(one), and then writing code for X which will work on all possible subclasses of X(infinity).

Related Solutions

Object-oriented – Changing behaviour of abstract class without modifying subclasses

First question--why do you need to change the logic of the abstract class? You seem to indicate that you don't want any of its current subclasses to inherit the changes, so I have to assume you want to write new subclasses which will. Given that the new subclasses don't exist yet, add a little indirection first:

abstract class SuperClass {
    protected function doMagic() { 
        ... I need to change this ...
    }

    abstract public function execute();
}

abstract class ExtendedSuperClass extends SuperClass {
    protected function doMagic() { 
        ... NEW CODE ...
    }
}


class SubClass1 extends SuperClass {
    protected public execute() {
        $this->doMagic();
        ... some other code ...
    }
}

class SubClass2 extends SuperClass {
    protected public execute() {
        $this->doMagic();
        ... some different code ...
    }
}

class SubClass3 extends ExtendedSuperClass {
    protected public execute() {
        $this->doMagic();
        ... yet more different code ...
    }
}

That is, extend the abstract class with a new abstract class, and have the classes needing the new logic extend THAT.

C# – Help with Inheritance Hierarchy Design

You have created your own, unnecessary problem here.

Everything must be runnable
The default implementation of run requires proc
Anybody implementing their own run can ignore proc (but all that useless code still adorns the class and any descendants).
Your concrete, exception-throwing implementation of proc enables people to set up themselves up for runtime errors. Why do that?

OK, so you wanted to be helpful by providing a default implementation but you have bolted on implementation details which you know will not always be desired. You say you want to avoid code duplication but you have encumbered your hierarchy with code which will be obsolete as soon as somebody extends it.

I would strongly urge you to go the interface/abstract-base-class route here:

Abstract base class runnable
Abstract base class proccer
Create proc-calling concrete implementation of runnable.
Wherever possible (even in proc-caller class methods) refer to runnables and not proc-callers).

So your default run implentation can be used where desired but can be dropped without penalty. Also, this way people are forced to implement proc if they mix in proc-caller but you don't have to write that "Not implemented" version of proc - they can't compile their code if they do not implement proc. Why create that runtime trap when you can require them to fix things at compile time?

If you think there are other useful behaviours which should be available, you can avoid code duplication by providing "interfaces" in the same way I have shown for proc. Anybody who wants that can mix it in.