Is it okay to have objects that cast themselves, even if it pollutes the API of their subclasses?

Question

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?

Answer 1

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));

Answer 2

From my perspective: your design is wrong.

Translated to natural language, you are saying the following:

Given we have animals, there are cats and fish. animals have properties, which are common to cats and fish. But that's not enough: there are some properties, which differentiate cat from fish, therefore you need to subclass.

Now you have the problem, that you forgot to model movement. Okay. That's relatively easy:

for(Animal a : animals){
   if (a instanceof Fish) swim();
   if (a instanceof Cat) walk();
}

But that is a wrong design. The correct way would be:

for(Animal a : animals){
    animal.move()
}

Where move would be shared behavior implemented differently by each animal.

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.

This means: your design is broken.

My recommendation: refactor Base, Sub1 and Sub2.

Answer 3

It's a little hard to imagine a circumstance where you have a group of things and want them to either make a sandwich or contact aliens. In most cases where you find such casting you will operate with one type - e.g. in clang you filter a set of nodes for declarations where getAsFunction returns non-null, rather than doing something different for each node in the list.

It might be that you need a perform a sequence of action and it's not actually relevant that the objects performing the action are related.

So instead of a list of Base, work on the list of actions

for (RandomAction action : actions)
   action.act(context);

where

interface RandomAction {
    void act(Context context);
} 

interface Context {
    Ingredients getRandomIngredients();
    double getFrequency();
}

You can, if appropriate, have Base implement a method to return the action, or whatever other means you need to select the action from the instances in your base list (since you say you can't use polymorphism, so presumably the action to take is not a function of the class but some other property of the bases; otherwise you'd just give Base the act(Context) method )

Answer 4

How about if you have your subclasses implement one or more interfaces that define what they can do ? Something like this:

interface SandwichCook
{
    public void makeASandwich(String[] ingredients);
}

interface AlienRadioSignalAwarable
{
    public void contactAliens(int frequency);

}

Then your classes will look like this:

class Sub1 extends Base implements SandwichCook
{
    public void makeASandwich(String[] ingredients)
    {
        //some code here
    }
}

class Sub2 extends Base implements AlienRadioSignalAwarable
{
    public void contactAliens(int frequency)
    {
        //some code here
    }
}

And your for-loop will become:

for (Base base : bases) {
    if (base instanceof SandwichCook) {
        base.makeASandwich(getRandomIngredients());
    } else if (base instanceof AlienRadioSignalAwarable) {
        base.contactAliens(getFrequency());
    }
}

Two major advantages to this approach:

• no casting involved
• you can have each subclass implement as many interfaces as you want, which does provide some flexibility for future changes.

PS: Sorry for the names of the interfaces, I couldn't think of anything cooler at that particular moment :D .

Answer 5

The approach can be a good one in cases where almost any type within a family will either be directly usable as an implementation of some interface which meets some criterion or can be used to create an implementation of that interface. The built-in collection types would IMHO have benefited from this pattern, but since they don't for purposes of example I'll invent a collection interface BunchOfThings<T>.

Some implementations of BunchOfThings are mutable; some aren't. In many cases, an object Fred might want to hold something it can use as a BunchOfThings and know that nothing other than Fred will be able to modify it. This requirement may be satisfied in two ways:

1. Fred knows that it holds the only references to that BunchOfThings, and no outside reference to that BunchOfThings ir its internals exists anywhere in the universe. If nobody else has a reference to the BunchOfThings or its internals, nobody else will be able to modify it, so the constraint will be satisfied.

2. Neither the BunchOfThings, nor any of its internals to which outside references exist, can be modified via any means whatsoever. If absolutely nobody can modify a BunchOfThings, then the constraint will be satisfied.

One way of satisfying the constraint would be to unconditionally copy any object received (recursively processing any nested components). Another would be to test whether a received object promises immutability and, if not, make a copy of it, and do likewise with any nested components. An alternative, which is apt to be cleaner than the second and faster than the first, is to offer an AsImmutable method which asks an object to make an immutable copy of itself (using AsImmutable on any nested components that support it).

Related methods may also be provided for asDetached (for use when code receives an object and doesn't know if it will want to mutate it, in which case a mutable object should be replaced with a new mutable object, but an immutable object can be kept as-is), asMutable (in cases where an object knows that it will hold an object earlier returned from asDetached, i.e. either an unshared reference to a mutable object or a sharable reference to a mutable one), and asNewMutable (in cases where code receives an outside reference and knows it's going to want to mutate a copy of the data therein--if the incoming data is mutable there's no reason to start by making an immutable copy which is going to be immediately used to create a mutable copy and then abandoned).

Note that while the asXX methods may return slightly different types, their real role is to ensure that the returned objects will satisfy program needs.

Answer 6

Ignoring the issue of whether you have a good design or not, and supposing it's good or at least acceptable, I would want to consider the capabilities of the subclasses, not the type.

Therefore, either:

---

Move some knowledge of the existence of sandwiches and aliens into the base class, even though you know some instances of the base class can't do it. Implement it in the base class to throw exceptions, and change the code to:

if (base.canMakeASandwich()) {
    base.makeASandwich(getRandomIngredients());
    // ... etc.
} else { // can't make sandwiches, must be able to contact aliens
    base.contactAliens(getFrequency());
    // ... etc.
}

Then one or both subclasses overrides canMakeASandwich(), and only one implements each of makeASandwich() and contactAliens().

---

Use interfaces, not concrete subclasses, to detect what capabilities a type has. Leave the base class alone, and change the code to:

if (base instanceof SandwichMaker) {
    ((SandwichMaker)base).makeASandwich(getRandomIngredients());
    // ... etc.
} else { // can't make sandwiches, must be able to contact aliens
    ((AlienContacter)base).contactAliens(getFrequency());
    // ... etc.
}

or possibly (and feel free to ignore this option if it doesn't suit your style, or for that matter any Java style you'd consider sensible):

try {
    ((SandwichMaker)base).makeASandwich(getRandomIngredients());
} catch (ClassCastException e) {
    ((AlienContacter)base).contactAliens(getFrequency());
}

Personally I don't usually like the latter style of catching a half-expected exception, because of the risk of inappropriately catching a ClassCastException coming out of getRandomIngredients or makeASandwich, but YMMV.

Answer 7

Here we have an interesting case of a base class that downcasts itself to its own derived classes. We know full well this is normally bad, but if we want to say we found a good reason, let us look and see what the constraints on this might be:

1. We can cover all the cases in the base class.
2. No foreign derived class would have to add a new case, ever.
3. We can live with the policy for which call to make being under the control of the base class.
4. But we know from our science that the policy of what to do in a derived class for a method not in the base class is that of the derived class and not the base class.

If 4 then we have: 5. The policy of the derived class is always under the same political control as the policy of the base class.

2 and 5 both directly imply that we can enumerate all derived classes, which means that there aren't supposed to be any outside derived classes.

But here's the thing. If they're all yours you can replace the if with an abstraction that is a virtual method call (even if it's a nonsense one) and get rid of the if and self-cast. Therefore, don't do it. A better design is available.

Answer 8

Put an abstract method in the base class which does one thing in Sub1 and the other thing in Sub2, and call that abstract method in your mixed loops.

class Sub1 : Base {
    @Override void doAThing() {
        this.makeASandwich(getRandomIngredients());
    }
}

class Sub2 : Base {
    @Override void doAThing() {
        this.contactAliens(getFrequency());
    }
}

for (Base base : bases) {
    base.doAThing();
}

Note that this may require other changes depending on how getRandomIngredients and getFrequency are defined. But, honestly, inside the class that contacts aliens is probably a better place to define getFrequency anyway.

Incidentally, your asSub1 and asSub2 methods are definitely bad practice. If you're going to do this, do it by casting. There's nothing these methods give you that casting doesn't.

Answer 9

You could push both makeASandwich and contactAliens into the base class and then implement them with dummy implementations. Since the return types/arguments can't be resolved to a common base class.

class Sub1 extends Base{
    Sandwich makeASandwich(Ingredients i){
        //Normal implementation
    }
    boolean contactAliens(Frequency onFrequency){
        return false;
    }
 }
class Sub2 extends Base{
    Sandwich makeASandwich(Ingredients i){
        return null;
    }
    boolean contactAliens(Frequency onFrequency){
       // normal implementation
    }
 }

There are obvious downsides to this sort of thing, like what that means for the contract of the method and what you can and cannot infer about the context of the result. You can't think since I failed to make a sandwich the ingredients got used up in the attempt etc.

Answer 10

What you are doing is perfectly legitimate. Do not pay attention to the naysayers who merely reiterate dogma because they read it in some books. Dogma has no place in engineering.

I have employed the same mechanism a couple of times, and I can say with confidence that the java runtime could have also done the same thing in at least one place that I can think of, thus improving performance, usability, and readability of code that uses it.

Take for example java.lang.reflect.Member, which is the base of java.lang.reflect.Field and java.lang.reflect.Method. (The actual hierarchy is a bit more complicated than that, but that's irrelevant.) Fields and methods are vastly different animals: one has a value that you can get or set, while the other has no such thing, but it can be invoked with a number of parameters and it may return a value. So, fields and methods are both members, but the things you can do with them are about as different from each other as making sandwiches vs. contacting aliens.

Now, when writing code that uses reflection we very often have a Member in our hands, and we know that it is either a Method or a Field, (or, rarely, something else,) and yet we have to do all the tedious instanceof to figure out precisely what it is and then we have to cast it to get a proper reference to it. (And this is not only tedious, but it also does not perform very well.) The Method class could have very easily implemented the pattern that you are describing, thus making the life of thousands of programmers easier.

Of course, this technique is only viable in small, well-defined hierarchies of closely coupled classes that you have (and will always have) source-level control of: you don't want to be doing such a thing if your class hierarchy is liable to be extended by people who are not at liberty to refactor the base class.

Here is how what I have done differs from what you have done:

• The base class provides a default implementation for the entire asDerivedClass() family of methods, having each one of them return null.

• Each derived class only overrides one of the asDerivedClass() methods, returning this instead of null. It does not override any of the rest, nor does it want to to know anything about them. So, no IllegalStateExceptions are thrown.

• The base class also provides final implementations for the entire isDerivedClass() family of methods, coded as follows: return asDerivedClass() != null; This way, the number of methods that need to be overriden by derived classes is minimized.

• I have not been using @Deprecated in this mechanism because I did not think of it. Now that you gave me the idea, I will put it to use, thanks!

C# has a related mechanism built-in via the use of the as keyword. In C# you can say DerivedClass derivedInstance = baseInstance as DerivedClass and you will get a reference to a DerivedClass if your baseInstance was of that class, or null if it was not. This (theoretically) performs better than is followed by cast, (is is the admittedly better named C# keyword for instanceof,) but the custom mechanism that we have been hand-crafting performs even better: the pair of instanceof-and-cast operations of Java, as well as the as operator of C# do not perform as fast as the single virtual method call of our custom approach.

I hereby put forth the proposition that this technique should be declared to be a pattern and that a nice name should be found for it.

Gee, thanks for the downvotes!

A summary of the controversy, to save you from the trouble of reading the comments:

People's objection appears to be that the original design was wrong, meaning that you should never have vastly different classes deriving from a common base class, or that even if you do, the code which uses such a hierarchy should never be in the position of having a base reference and needing to figure out the derived class. Therefore, they say, the self-casting mechanism proposed by this question and by my answer, which improves the use of the original design, should never have been necessary in the first place. (They don't really say anything about the self-casting mechanism itself, they only complain about the nature of designs that the mechanism is meant to be applied to.)

However, in the example above I have already shown that the creators of the java runtime did in fact choose precisely such a design for the java.lang.reflect.Member, Field, Method hierarchy, and in the comments below I also show that the creators of the C# runtime independently arrived at an equivalent design for the System.Reflection.MemberInfo, FieldInfo, MethodInfo hierarchy. So, these are two different real world scenarios which are sitting right under everyone's nose and which have demonstrably workable solutions using precisely such designs.

That's what all the following comments boil down to. The self-casting mechanism is hardly mentioned.