Rethinking inheritance when subclass implements interface

Question

Suppose if I have an abstract Weapon class, and the subclass ReloadableWeapon which implements the interface Reloadable.

interface Reloadable {
 void Reload();
}
public abstract class Weapon{
 @override
 public abstract void attack(Player enemy){}
}
public final class ReloadableWeapon extends Weapon implements Reloadable{}

It's a good idea, when declaring an object that implements an interface or inherits an abstract base class, to declare from the abstract or interface first and then assign the type you want.

Example:

Weapon weapon = new ReloadableWeapon(); 

Problem is, when I need to use my Reload method one of four things will eventually happen:

1. I can declare my ReloadableWeapon as a concrete type ReloadableWeapon chargeGun = new ReloadableWeapon()
2. Use instanceof and downcast
3. Use the visitor pattern
4. Declare it from the interface, Reloadable chargeGun = new ReloadableWeapon()

Each of these "solutions" has unique problems. For example, solution 3 introduces more code just to access a method of a specific type, while solution 4 means I can only access Reload(); and not the methods inherited from Weapon.

When subclasses begin to implement interfaces, is it an indication that you need to rethink your design?

• Kilian Foth: Why would it be a problem? IIUC, ReloadableWeapon knows about reloading (since it uses Reloadable) and about attack() (because is ISA Weapon). So ReloadableWeapon can simply override Weapon.attack() to use Reload().
• Andy: It depends. There's no clear answer. If you know that you will require the reload method, you should most likely instantiate a ReloadableWeapon directly, if you don't care about the reload method and just want to use a regular weapon, you probably will not instantiate a ReloadableWeapon in the first place.
• user306112: @KilianFoth - While you can use Reload() from attack(), what if I want to reload my weapon when my player is not attacking or resting?
• Kilian Foth: @Sveta Then you can't do that within Weapon - it has to be within ReloadableWeapon or in its subclasses (e.g. CrossBow). Of course, you can have a general callback Weapon.downtime() that gets called regularly, and override ReloadableWeapon.downtime() so that it calls Reload() as well as super.downtime().
• user306112: @KilianFoth - Or an even better approach, Weapon can have a prepare() method, guns can be loaded, a sword can be drawn. During player downtime I can call prepare() provided the Weapon in in use. At least this way, it avoids having the declare ReloadeableWeapon separately and I can keep my Weapon in a collection without having to downcast to use it.

Answer 1

There is no "right" solution, this is fully context dependend. In an ideal design, you start with

ReloadableWeapon rw = new ReloadableWeapon()

in a context where ReloadableWeapon is known, and now you can pass rw to functions which expect a Weapon, and also functions which expect a Reloadable. However, if you have to put a weapon into a collection of Weapon objects from a generic lib you cannot change easily, and then you need to retrieve objects from that collection at a different place in your program which deals with ReloadableWeapon objects again, then you might run into the situation where you cannot easily avoid a cast to the other type.

So in short, try to distribute the responsibilities in your code so you have one part dealing only with weapons, one part with reloadables, and one part with reloadable weapons, so you can avoid most downcasts. But if that is not possible in every case, only in 95% of all cases, then for heavens sake downcast in the remaining 5%, that's not the end of the world and won't turn your program immediately into a maintenance hell.

• user306112: I like this answer, but it makes me "uneasy" whenever I have to downcast. People are so quick to throw around the term "code smell" that it often makes me wonder if down casting the right approach.
• Doc Brown: @Sveta: downcasting is a code smell, but not every code smell is necessarily worth to be removed.

Answer 2

What I would like to point, is that by respect of the Liskov substitution principle : "objects in a program should be replaceable with instances of their subtypes without altering the correctness of that program.", the creation of the ReloadableWeapon should not create new instructions in higher level

In your example, I see two distinct situations :

1. Your code has to call reload. It means that it has the knowledge it's using Reloadable Weapon in its context. No point to use Weapon weapon = new ReloadableWeapon() here, or we could just shot ourselves in the foot by using Object weapon = new ReloadableWeapon(). We KNOW we are reloading weapons, we use that knowledge.
2. Your code has no clue if the weapon has to be reloaded. In this case, the code who would make the weapon attack would just call attack, and let the implementations decides what to do : no more ammo ? no action performed. If the reload has to be triggered automatically, the attack method would call reload if it hasn't any more ammo.

To me, the visitor pattern/down-casting has sense when it comes to "different reactions to a same event", but without enough shared behaviour to make it a strategy. For example, a Visitor could have different implementations for Dogs and Cats (siblings of their parent Animal class). If you create ReloadableWeapon to extend Weapon, you should not have code that knows if something is Reloadable or not, or it means your extension of Weapon breaks the correctness of your uses of a generic weapon.

• user306112: A well written answer! Upvoted.
• user306112: I agree with your answer, so in this case all weapons can be prepared in someway, just differently (A gun can be loaded, a sword can be drawn). Would it be okay to have an public abstract void prepare(); and let the sub classes decide what to do? The term prepare is ambiguous, the Weapon class knows it has a prepare() method but not what it's preparing and if it can be reloaded or not.

Answer 3

In a complex system it is understandable that you may have to dynamically perform an action without statically knowing whether this object supports the action. In my experience, it is best to ask the object – not by casting, but by guarding access to the method. For example:

@FunctionalInterface
interface ReloadAction { void reload(); }
public abstract class Weapon {
 public abstract void attack();
 public ReloadAction getReloadAction() { return null; }
}
public ReloadableWeapon extends Weapon {
 ...
 @Override
 public ReloadAction getReloadAction() {
 return () -> { this.ammo = this.maxAmmoCapacity; };
 }
}

In client code:

void handleReloadEvent(Weapon w) {
 ReloadAction reload = w.getReloadAction();
 if (reload != null) reload.reload();
 else showErrorMessage("this weapon can't be reloaded");
}

Ideally, the UI adapts itself to prevent impossible events.

To make the signature more explicit you could return a Optional<ReloadAction>.

How does this relate to your suggestions?

This is similar in effect to a downcast but is a bit more typesafe. It is also far more extensible (in the sense of the Open/Closed Principle), since it is now the Weapon object itself and not the client code which decides whether a weapon is reloadable. In particular, you can now have different kinds of weapons that are reloadable without having to inherit from Reloadable. This composability also allows you to handle weapons that support a combination of multiple interfaces, without having to create a new class for each combination: the main Weapon class is often sufficient if you supply the available actions through the constructors.

This solution is the exact opposite of using a Visitor. With the visitor you cannot add arbitrary new classes but can add more actions through visitors. This is because the visitor interface describes which classes are supported. But here we have added a new method to the Weapon interface. So we cannot add arbitrary new actions, but we can create more subclasses that implement these actions. Whether a visitor or these method objects are more appropriate depends on how you expect to extend your code in the future: is it more likely to add new actions (then prefer a visitor) or more likely to add new implementations (then prefer method objects)?

Related concepts:

• Entity-Attribute-Value systems: describing a data model dynamically instead of using the type system of the host language.
• Object Adapter Pattern: these method objects behave like adapters from the Weapon class to the Action interface.
• Type Object Pattern: composition instead of inheritance, especially for complex behaviours that are common in games.
• Virtual Constructor idiom, especially in C++: use methods instead of downcasting. Related to the Template Method Pattern.

• RubberDuck: While I like your solution, I’m compelled to downvote because you return null instead of a no op action. It unnecessarily complicated client code.
• Hangman4358: I agree, I like the idea but if you are declaring a functional interface I would also declare a no-op implementation to avoid null checks.
• amon: @RubberDuck I considered that while writing, but consciously decided against it. One use case is to run the action if it is present; here a null object like () -> {} would help. But it can also be desirable to merely ask if the action is available. A null object pattern complicates that use case: I'd need a singleton null object marker that can be checked with identity, or a special null object class that I can check with instanceof, or the action object needs an additional method to ask if it is really present (which complicates both a @FunctionalInterface and client code). ...
• amon: ...But I do understand that returning null can be problematic, especially as Java doesn't support elegant C++-isms like if (auto reload = w.reload_action()) (*reload)();. It then may be desirable to make the interface more explicit by returning an Optional<> value as suggested in my answer. The Java equivalent then becomes something like w.getReloadAction().ifPresent(reload -> reload.reload());. I think in C# this discussion would be moot because we have the safe navigation operator, so returning null would be appropriate in that language?
• amon: @Sveta as usual, this depends on your exact requirements. A tryReload() method is certainly the most elegant approach if all you want to do is perform a reload, or ignore the action if it's not supported. In contrast, my suggested design also allows us to query whether the action is supported without immediately performing it. This is more general, but you're right: this generality might be unneeded. You could alternatively add a canReload() check in addition to tryReload(), but then you have to maintain two methods together and are duplicating information.