Java Inheritance – Is It Okay to Have Objects That Cast Themselves?

inheritancejavatype casting

I have a base class, Base. It has two subclasses, Sub1 and Sub2. Each subclass has some additional methods. For example, Sub1 has Sandwich makeASandwich(Ingredients... ingredients), and Sub2 has boolean contactAliens(Frequency onFrequency).

Since these methods take different parameters and do entirely different things, they're completely incompatible, and I can't just use polymorphism to solve this problem.

Base provides most of the functionality, and I have a large collection of Base objects. However, all Base objects are either a Sub1 or a Sub2, and sometimes I need to know which they are.

It seems like a bad idea to do the following:

for (Base base : bases) {
    if (base instanceof Sub1) {
        ((Sub1) base).makeASandwich(getRandomIngredients());
        // ... etc.
    } else { // must be Sub2
        ((Sub2) base).contactAliens(getFrequency());
        // ... etc.
    }
}

So I came up with a strategy to avoid this without casting. Base now has these methods:

boolean isSub1();
Sub1 asSub1();
Sub2 asSub2();

And of course, Sub1 implements these methods as

boolean isSub1() { return true; }
Sub1 asSub1();   { return this; }
Sub2 asSub2();   { throw new IllegalStateException(); }

And Sub2 implements them in the opposite way.

Unfortunately, now Sub1 and Sub2 have these methods in their own API. So I can do this, for example, on Sub1.

/** no need to use this if object is known to be Sub1 */
@Deprecated
boolean isSub1() { return true; }

/** no need to use this if object is known to be Sub1 */
@Deprecated
Sub1 asSub1();   { return this; }

/** no need to use this if object is known to be Sub1 */
@Deprecated
Sub2 asSub2();   { throw new IllegalStateException(); }

This way, if the object is known to be only a Base, these methods are un-deprecated, and can be used to "cast" itself to a different type so I can invoke the subclass's methods on it. This seems elegant to me in a way, but on the other hand, I'm kind of abusing Deprecated annotations as a way to "remove" methods from a class.

Since a Sub1 instance really is a Base, it does make sense to use inheritance rather than encapsulation. Is what I'm doing good? Is there a better way to solve this problem?

Best Answer

It doesn't always make sense to add functions to the base class, as suggested in some of the other answers. Adding too many special case functions can result in binding otherwise unrelated components together.

For example I might have an Animal class, with Cat and Dog components. If I want to be able to print them, or show them in the GUI it might be overkill for me to add renderToGUI(...) and sendToPrinter(...) to the base class.

The approach you are using, using type-checks and casts, is fragile - but at least does keep the concerns separated.

However if you find yourself doing these types of checks/casts often then one option is to implement the visitor / double-dispatch pattern for it. It looks kind of like this:

public abstract class Base {
  ...
  abstract void visit( BaseVisitor visitor );
}

public class Sub1 extends Base {
  ...
  void visit(BaseVisitor visitor) { visitor.onSub1(this); }
}

public class Sub2 extends Base {
  ...
  void visit(BaseVisitor visitor) { visitor.onSub2(this); }
}

public interface BaseVisitor {
   void onSub1(Sub1 that);
   void onSub2(Sub2 that);
}

Now your code becomes 

public class ActOnBase implements BaseVisitor {
    void onSub1(Sub1 that) {
       that.makeASandwich(getRandomIngredients())
    }

    void onSub2(Sub2 that) {
       that.contactAliens(getFrequency());
    }
}

BaseVisitor visitor = new ActOnBase();
for (Base base : bases) {
    base.visit(visitor);
}

The main benefit is that if you add a subclass, you'll get compile errors rather than silently missing cases. The new visitor class also becomes a nice target for pulling functions into. For example it might make sense to move getRandomIngredients() into ActOnBase.

You can also extract the looping logic: For example the above fragment might become

BaseVisitor.applyToArray(bases, new ActOnBase() );

A little further massaging and using Java 8's lambdas and streaming would let you get to

bases.stream()
     .forEach( BaseVisitor.forEach(
       Sub1 that -> that.makeASandwich(getRandomIngredients()),
       Sub2 that -> that.contactAliens(getFrequency())
     ));

Which IMO is pretty much as neat looking and succinct as you can get.

Here is a more complete Java 8 example:

public static abstract class Base {
    abstract void visit( BaseVisitor visitor );
}

public static class Sub1 extends Base {
    void visit(BaseVisitor visitor) { visitor.onSub1(this); }

    void makeASandwich() {
        System.out.println("making a sandwich");
    }
}

public static class Sub2 extends Base {
    void visit(BaseVisitor visitor) { visitor.onSub2(this); }

    void contactAliens() {
        System.out.println("contacting aliens");
    }
}

public interface BaseVisitor {
    void onSub1(Sub1 that);
    void onSub2(Sub2 that);

    static Consumer<Base> forEach(Consumer<Sub1> sub1, Consumer<Sub2> sub2) {

        return base -> {
            BaseVisitor baseVisitor = new BaseVisitor() {

                @Override
                public void onSub1(Sub1 that) {
                    sub1.accept(that);
                }

                @Override
                public void onSub2(Sub2 that) {
                    sub2.accept(that);
                }
            };
            base.visit(baseVisitor);
        };
    }
}

Collection<Base> bases = Arrays.asList(new Sub1(), new Sub2());

bases.stream()
     .forEach(BaseVisitor.forEach(
             Sub1::makeASandwich,
             Sub2::contactAliens));
Related Topic