Using the Strategy Pattern to avoid downcasting

[Andy Rosemond]

Hello, New to the Ranch!

Before I continue two things:

I asked this question of StackOverflow, but the answers were mixed, and I think I'm partly to blame because of the example problem I have.

https://stackoverflow.com/questions/46893229/violation-of-isp
https://stackoverflow.com/questions/46841630/using-the-strategy-pattern-to-avoid-downcasting
https://softwareengineering.stackexchange.com/questions/359542/is-it-ever-okay-to-violate-the-lsp/359548?noredirect=1#comment779531_359548

Although my problems has a gaming context(A text adventure game), don't take it too seriously, it could have just as easily be vehicles with different subclass for each type. Gaming was the quickest context I could come up with, but I'm starting to regret that. I have a feeling my problem is I'm trying to do too many things at once without a real justification. You'll see  

I'm currently reading about SOLID principals and also exploring Design Patterns.

This is where I learned LSP: https://www.tomdalling.com/blog/software-design/solid-class-design-the-liskov-substitution-principle/

From my understanding, you'd want to avoid using the keyword instanceof or getClass(), only in certain cases for example in the equals method. I don't really understand why you'd want to avoid instanceof or getClass().

I'll present my problem(s) and the solution I used to fix it and then ask for guidance.

Problem: Some subclasses have methods unique to that subclass. Which means at some point I would have to downcast.

My attempt at solving the problem: Some of the advice I've been given either violates a SOLID principal or just doesn't make sense. For example, someone suggested that Sword be made aware of Reload just don't do anything which to is a violation or LSP/ISP.

I prohibited inheritance with the final keyword and used the Strategy Pattern to implement the behavior. Example:

public final Weapon{

    private final String name;
    private final int damage;
    private final List<AttackStrategy> validactions;
    private final List<Actions> standardActions;

    public Weapon(String name, int damage, List<AttackStrategy> standardActions, List<Actions> attacks)
    {
        this.name = name;
        this.damage = damage;
        standardActions = new ArrayList<Actions>(standardActions);
        validAttacks = new ArrayList<AttackStrategy>(validActions);
    }

    public void standardAction(String action){} // -- Can call reload or aim here.  

    public int attack(String action){
         //Call any actions that are attacks. 
         //If list contain action do it, if not the action is ignored.
        //The caller doesn't know if the weapon is reloadable or not.
    } 

}

public interface AttackStrategy{
    void attack(Enemy enemy);
}

public class Shoot implements AttackStrategy {
    public void attack(Enemy enemy){
        //code to shoot
    }
}

List<AttackStrategy> actions = new ArrayList<AttackStrategy();
actions.add(new Shoot())

List<Actions> standardActionactions = new ArrayList<Actions>();
actions.add(new Reload(10))

Weapon rifle = new Weapon("Sniper Rifle", 5, standardActionactions, actions);

I understand, it's a lot of work for very little. The criticism I've received is that it violates ISP(A comment suggested that Weapon was an ISP violation machine), but I don't think so, because Sword is no longer and shouldn't be aware the Reload method/class. The client is not forced to implement methods it isn't going to use. If for some reason the game allows sword to fire something, for example, fireball, then the client can go ahead and make Sword implement reload, but that's at the client's discretion. Another solution is to have my Weapon class have a prepare method, for Sword it would remove it from inventory, Gun it would be loaded and aimed, I get that, but again, I might end up in a situation where I have a subclass specific method(for example, Sword could have wipeBloodofBlade() or Gun could have adjustLaserSight), and to me anyway the Strategy Pattern seemed to solve my problems.

How so:

I can maintain a List<Weapons> collection without the need for downcasting, the weapons will be loading with the necessary actions.

Any weapon that isn't Reloadable, won't have the concrete class Reload. This mean no throwing or leaving the method blank, which is a violation of LSP as stated in the Tom Dalling article above

Questions:
1. Why avoid the use of instanceof or getClass?
2. Is the Strategy Pattern, they way I implemented it, a good solution? If not, why?
3. How would you suggest I fix my code without violating SOLID?

This is not homework. I know it sounds foolish to say, but this problem comes from years of reading and researching, but never being able to come up with a valid solution. My attempt at solving the problem was the Strategy Pattern. Given the amount of code, it probably isn't the best, this is why I'm asking.
 

[Campbell Ritchie]

Welcome to the Ranch

Why would you want to avoid instanceof? Apart from the fact that is has the same problem as the isFlightless() method in the Tom Dalling blog, you mean? It would be the same problem as in this sort of code:-

if (!(bird instanceof Penguin || bird instanceof Emu ||
      bird instanceof Dodo    || bird instanceof Kiwi))
{
   bird.setAltitude(1000);
}

The code shown on that blog looks like C++; since C++ supports multiple inheritance, you have to do something different in Java®. You would make flying animals (flies, beetles, bats and most birds) implement a “Flightful” interface, and then work out what to do about public final class Dodo extends Pigeon. (Dodo must of course be a final class ‍) If you start using instanceof except, as you said, in the equals() method, you start to think there is something wrong with your inheritance hierarchy. A particularly bad example is the existence of the ensureCapacity() method in some Lists and not all. If you want to increase the capacity of a liked list, well what't the point; its capacity is always exactly the same as its number of elements. But as array list, now that is something different.

List<Foo> list = ...;
// ...
if (list instanceof Vector)
{
   ((Vector)list).ensureCapacity(100000000);
}
else if (list instanceof ArrayList)
{
   ((ArrayList)list).ensureCapacity(10000000);
}

. . . and what if you have a custom list implementation to cast to, or you didn't count the 0s correctly? Isn't that horrible code? Why isn't there a ResizableList interface:-if (list instanceof ResizableList) {   ((ResizableList)list).ensureCapacity(100000000); }Does that go any way to helping you?

[Andy Rosemond]

Does that go any way to helping you?

It helps in understand why you won't use instanceof. Thanks!  

Now, to work around that, I used the strategy pattern, but that leads to another set of problem as I outlined above. Not sure how to go about solving those.

[Junilu Lacar]

The use of instanceof is a strong code smell when it replaces polymorphism. It's related to how downcasting can be a code smell.  The example that Campbell gave should make you think, "Why can't I write more generic code? Why do I have to care about the specific type(s) of these objects for this logic to work?" Writing if-statements that check for specific types and performing actions accordingly means that you, the programmer, are dictating what course of action to take rather than leaving it to the objects and polymorphism to figure out what the appropriate course of action should be.

I'm actually having trouble recognizing your code as a proper use of the pattern though. I was expecting something more along the lines of this:

   private AttackStrategy attackMode;

   public void attack(Enemy target) {
        attackMode.attack(target, this.getInventory());
   }

which is more in line with the typical Strategy example of Document and PrintStrategy:

class Document {

   public void print(PrintStrategy destination) {
        destination.print(this);
   }
}

[Junilu Lacar]

One of your concerns about violating LSP is with regards to one article you cited that says you're probably violating the principle if the subclass does nothing or just throws an exception. There are no absolutes when it comes to designing so take what that article says with a grain of salt. Also, that article says "probably violating."  The Null Object pattern is a perfect example of using LSP to your advantage and the core of that pattern is a subclass that does nothing at all. Also, there are number of List implementations that can throw an UnsupportedOperationException when you call the remove() method. Context matters and sometimes there are other considerations that might outweigh the impact of violating a principle.

I would argue that the real problem wasn't so much a violation of LSP. Given that the final solution presented was to separate the problematic setAltitude() method to a separate FlightfulBird class, the author was really separating interfaces (Bird vs. FlightfulBird), thus implying that ISP was really the principle that was violated in the initial design.

[Junilu Lacar]

I think I see what your problem is and it has more to do with violating ISP and trying to do too much at once.

The problem you have with downcasting comes from trying to multi-purpose your List<Weapon>. When you declare something to be a List<Weapon>, the only interface you should care about when using that reference is what you can do with Weapon objects. The problem arises when you want to pick out specific objects in a List<Weapon> and treat them as a BladedWeapon or Firearm.

You're looking for a solution where you are still able to continue just using a List<Weapon> for multiple purposes. I doubt you'll find a way to do that without eventually downcasting somewhere in your code. So, rather than trying to keep using a List<Weapon>, I would try backing up and use more specific types rather than the general type whenever I want to invoke specific subclass behaviors.

Also, it might help if you review the use of Java Bounded Parameters. You may not realize this but even though BladedWeapon extends Weapon, it doesn't follow that List<BladedWeapon> extends List<Weapon>. You have to do something like the following instead.

Assuming:

interface Weapon {
    void attack(Enemy e);
}

interface Blade extends Weapon {
    void slash();
    void stab();
}

interface Firearm extends Weapon {
    void shoot();
    void reload();
}

Then

List<Blade> blades = new ArrayList<>();
blades.add(new Broadsword());
blades.add(new Dagger());
blades.add(new Katana());

List<Firearm> guns = new ArrayList<>();
guns.add(new Revolver());
guns.add(new SniperRifle());
guns.add(new Shotgun());

List<Weapon> allArms = new ArrayList<>();
allArms.addAll(guns);
allArms.addAll(blades);

// use blades to call slash() or stab()

// use guns to call shoot() or reload()

// use any of the three to call attack()

You have to start with these references first and then use each according to the kind of behaviors you want. If you want to just call attack() then you can use any of the three references. If you specifically want to call stab(), then you have to use the blades reference variable. If you want to call shoot() or reload(), you should use the guns reference variable.

I think the bottom line is that you should only program to the level of abstraction that each type reference is designed to represent. When you start wanting to use a superclass type reference to invoke specific subclass behavior, you're already starting from a backwards perspective so you're bound to end up violating a principle or two.

[Junilu Lacar]

@Andy: It's good that you're trying to learn how to properly apply SOLID because, unfortunately, it seems you're in a minority. You should be careful not to treat principles as absolutely inviolable though. Again, context matters and there are some situations where strict adherence to a principle is precluded or has to be preempted by other considerations.

With regard to instanceof, there are contexts in which its use is warranted, otherwise why would the designers of Java include it as part of the language, right? As I mentioned before, using instanceof is a strong code smell when it replaces polymorphism. However, when you're more interested in the subtype itself rather than specific behaviors (methods) of a subtype, checking the class with instanceof or getClass() is not a smell, IMO. Consider the examples in this article: https://www.leveluplunch.com/java/examples/filter-collection-by-class-type/

This static method below might be useful to provide with your Weapon class:

	@SuppressWarnings("unchecked")
	public static <T extends Weapon> List<T> filter(List<? super T> list, Class<T> subtype) {
		List<T> subset = new ArrayList<>();
		for (Object o : list) {
			if (subtype.isAssignableFrom(o.getClass())) {
				subset.add((T) o);
			}
		}
		return subset;
	}

I used getClass() and isAssignableFrom() in this case but the principle is pretty much the same: I'm not checking the object type to replace polymorphism, I'm just extracting a subset of objects from a list based on the object type.

Modifying some of the declarations from my previous example, here are some ways you can use this filter() method:

interface Reloadable extends Weapon {
    void reload();
}

interface Firearm extends Reloadable {
    ...
}

class RifleBayonet implements Firearm, Blade {
    ...
}

class FireBallSword implements Firearm, Blade {
    ...
}

class Catapult implements Reloadable {
   ...
}

...

allArms.add(new RifleBayonet()); // can shoot(), reload(), slash(), stab()
allArms.add(new Catapult());  // can reload(), attack()
allArms.add(new FireBallSword()); // can shoot(), reload(), slash(), stab()
		
System.out.println("\nEverything:");
for (Weapon w : allArms) {
	w.attack();
}
		
System.out.println("\nBlades:");
for (Blade edge : filter(allArms, Blade.class)) {
	edge.slash();
	edge.stab();
}
		
System.out.println("\nFirearms:");
List<Firearm> shootables = filter(allArms, Firearm.class);
for (Firearm gun : shootables) {
	gun.shoot();
}
		
System.out.println("\nReloadables:");
List<Reloadable> hasAmmo = filter(allArms, Reloadable.class);
for (Reloadable empty : hasAmmo) {
	empty.reload();
}

I think this shows a lot of examples for applying the ISP in your design and code to get more flexibility without sacrificing clarity and meaningful semantics.