10

I'm following up on this question, but I'm switching my focus from code to a principle.

From my understanding of the Liskov substitution principle (LSP), whatever methods are in my base class, they must be implemented in my subclass, and according to this page, if you override a method in the base class and it does nothing or throws an exception, you're in violation of the principle.

Now, my problem can be summed up like this: I have an abstract Weapon class, and two classes, Sword and Reloadable. If Reloadable contains a specific method, called Reload(), I would have to downcast to access that method, and, ideally, you'd want to avoid that.

I then thought of using the Strategy Pattern. This way each weapon was only aware of the actions it's capable of performing, so for example, a Reloadable weapon, can obviously reload, but a Sword can't, and isn't even aware of a Reload class/method. As I stated in my Stack Overflow post, I don't have to downcast, and I can maintain a List<Weapon> collection.

On another forum, the first answer suggested to allow Sword to be aware of Reload, just don't do anything. This same answer was given on the Stack Overflow page I linked to above.

I don't fully understand why. Why violate the principle and allow Sword to be aware of Reload, and leave it blank? As I said in my Stack Overflow post, the SP, pretty much solved my problems.

Why isn't it a viable solution?

public final Weapon{

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

    private 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. 

    public static Weapon Sword(String name, damage, List<AttackStrategy> standardActions, List<Actions> attacks){
        return new Weapon(name, damage,standardActions, attacks) ;
    }

}

Attack Interface and Implementation:

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

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

public class Strike implements AttackStrategy {
    public void attack(Enemy enemy){
        //code to strike
    }
}
  • 2
    You can do class Weapon { bool supportsReload(); void reload(); }. Clients would test if supported before reloading. reload is defined contractually to throw iff !supportsReload(). That adheres to the LSP iff drived classes adhere to the protocol I just outlined. – usr Oct 22 '17 at 14:41
  • 3
    Whether you leave reload() blank or whether standardActions does not contain a reload action is just a different mechanism. There's no fundamental difference. You can do both. => Your solution is viable (which was your question).; Sword does not need to know about reload if Weapon contains a blank default implementation. – usr Oct 22 '17 at 15:44
  • 27
    I wrote a series of articles exploring a variety of problems with various techniques for solving this problem. The conclusion: don't try to capture the rules of your game in the language's type system. Capture the rules of the game in objects which represent and enforce rules at the level of the game logic, not the level of the type system. There is no reason to believe that whatever type system you're using is sophisticated enough to represent your game logic. ericlippert.com/2015/04/27/wizards-and-warriors-part-one – Eric Lippert Oct 22 '17 at 15:46
  • 2
    @EricLippert - Thanks for your link. I've come across this blog so many times, but some of the points made I don't quite understand, but it isn't your fault. I'm learning OOP on my own and came across SOLID principals. The first time I came across your blog, I didn't understand it at all, but I learned a little more and read your blog again, and slowly began to understand parts of what were being said. One day, I'll fully understand everything in that series. I hope :D – user286462 Oct 22 '17 at 16:17
  • 6
    @S.R. "if it does nothing or throws an exception, you're in violation" - I think you misread the message from that article. The problem wasn't directly that setAltitude did nothing, it was that it failed to fulfill the postcondition "bird will be drawn at the set altitude". If you define the postcondition of "reload" as "if sufficient ammo was available, weapon can attack again", then doing nothing is a perfectly valid implementation for a weapon that doesn't use ammo. – Sebastian Redl Oct 23 '17 at 7:50
17

The LSP is concerned about subtyping and polymorphism. Not all code actually uses these features, in which case the LSP is irrelevant. Two common use cases of inheritance language constructs that are not a case of subtyping are:

  • Inheritance used to inherit the implementation of a base class, but not its interface. In nearly all cases composition should be preferred. Languages like Java can't separate inheritance of implementation and interface, but e.g. C++ has private inheritance.

  • Inheritance used to model a sum type/union, e.g.: a Base is either CaseA or CaseB. The base type does not declare any relevant interface. To use its instances, you must cast them to the correct concrete type. The casting can be done safely and is not the issue. Unfortunately, many OOP languages are not able to restrict the base class subtypes to only the intended subtypes. If external code can create a CaseC, then code assuming that a Base can only be a CaseA or CaseB is incorrect. Scala can do this safely with its case class concept. In Java, this can be modelled when the Base is an abstract class with a private constructor, and nested static classes then inherit from the base.

Some concepts like conceptual hierarchies of real-world objects map very badly into object oriented models. Thoughts like “A gun is a weapon, and a sword is a weapon, therefore I'll have a Weapon base class from which Gun and Sword inherit” are misleading: real-word is-a relationships do not imply such a relationship in our model. One related issue is that objects may belong to multiple conceptual hierarchies or may change their hierarchy affiliation during run time, which most languages cannot model since inheritance is usually per-class not per-object, and defined at design-time not run-time.

When designing OOP models we should not think about the hierarchy, or how one class “extends” another. A base class is not a place to factor out the common parts of multiple classes. Instead, think about how your objects will be used, i.e. what kind of behaviour the users of these objects need.

Here, users may need to attack() with weapons and maybe reload() them. If we are to create a type hierarchy, then both of these methods must be in the base type, though non-reloadable weapons may ignore that method and do nothing when called. So the base class does not contain the common parts, but the combined interface of all subclasses. The subclasses don't differ in their interface, but only in their implementation of this interface.

It is not necessary to create a hierarchy. The two types Gun and Sword may be entirely unrelated. Whereas a Gun can fire() and reload() a Sword may only strike(). If you need to manage these objects polymorphically, you can use the Adapter Pattern to capture the relevant aspects. In Java 8 this is possible rather conveniently with functional interfaces and lambdas/method references. E.g. you might have an Attack strategy for which you supply myGun::fire or () -> mySword.strike().

Finally, it is sometimes sensible to avoid any subclasses at all, but model all objects through a single type. This is particularly relevant in games because many game objects do not fit nicely into any hierarchy, and may have many different capabilities. E.g. a role playing game may have an item that is both a quest item, buffs your stats with +2 strength when equipped, has a 20% chance on ignoring any damage received, and provides a melee attack. Or maybe a reloadable sword because it is *magic*. Who knows what the story requires.

Instead of trying to figure out a class hierarchy for that mess, it is better to have a class that provides slots for various capabilities. These slots can be changed at runtime. Each slot would be a strategy/callback like OnDamageReceived or Attack. With your weapons, we may have MeleeAttack, RangedAttack, and Reload slots. These slots may be empty, in which case the object does not provide this capability. The slots are then called conditionally: if (item.attack != null) item.attack.perform().

  • Sort of like the SP in a way. Why does the slot have to empty? If the dictionary doesn't contain the action, simply do nothing – user286462 Oct 22 '17 at 14:13
  • @S.R. Whether a slot is empty or doesn't exist doesn't really matter, and depends on the mechanism used to implement these slots. I wrote this answer with the assumptions of a fairly static language where slots are instance fields and always exist (i.e. normal class design in Java). If choose a more dynamic model where slots are entries in a dictionary (like using a HashMap in Java, or a normal Python object), then the slots don't have to exist. Note that more dynamic approaches give up a lot of type safety, which is usually not desirable. – amon Oct 22 '17 at 16:02
  • I agree that real world objects don't model well. If I understand your post, your saying I can use the Strategy Pattern? – user286462 Oct 22 '17 at 18:49
  • 2
    @S.R. Yes, the Strategy Pattern in some form is likely a sensible approach. Compare also the related Type Object Pattern: gameprogrammingpatterns.com/type-object.html – amon Oct 22 '17 at 18:56
3

Because having a strategy for attack isn't sufficient for your needs. Sure, it allows you to abstract away what actions the item can do, but what happens when you need to know the range of the weapon? Or the ammo capacity? Or what sort of ammo it takes? You're back to downcasting to get at that. And having that level of flexibility will make the UI a bit more difficult to implement, since it will need to have a similar strategy pattern to deal with all of the capabilities.

All that said, I don't particularly agree with the answers to your other questions. Having sword inherit from weapon is horrific, naive OO which invariably leads to no-op methods or type-checks strewn about the code.

But at the root of the matter, neither solution is wrong. You can use both solutions to make a functioning game that is fun to play. Each come with their own set of trade-offs, just like any solution you pick.

  • I think this is perfect. I can use the SP, but they're trade offs, just have to be aware of them. See my edit, for what I have in mind. – user286462 Oct 22 '17 at 13:35
  • 1
    Fwiw: a sword has infinite ammo: you can keep using it without reading forever; reload does nothing because you have infinite use to begin with; a range of one/melee: it's a melee weapon. It is not impossible to think about all of the stats/actions in a way that works for both melee and ranged. Still, as I've gotten older I use less and less inheritance in favor of interfaces, competition, and whatever the name is for using a single Weapon class with a sword and gun instance. – CAD97 Oct 22 '17 at 18:08
  • Fwiw in Destiny 2 swords use ammo for some reason! – user69767 Oct 22 '17 at 18:11
  • @CAD97 - This is the type of thinking I've seen regarding this problem. Have sword with infinite ammo, so no reloading. This just pushes the problem around or hides it. What if I introduce a grenade, what then? Grenades don't have ammo or shoot, and shouldn't be aware of such methods. – user286462 Oct 22 '17 at 18:31
  • 1
    I'm with CAD97 on this. And would create a WeaponBuilder that could build swords and guns by composing a weapon of strategies. – Chris Wohlert Dec 5 '17 at 13:07
3

Of course it's a viable solution; it's just a very bad idea.

The problem isn't if you have this single instance where you put reload on your base class. The problem is that you also need to put the "swing", "shoot" "parry", "knock", "polish", "disassemble", "sharpen", and "replace the nailes of the pointy end of the club" method on your base class.

The point of LSP is that your top level algorithms need to work and make sense. So if I have code like this:

if (isEquipped(weapon)) {
   reload();
}

Now if that throws a not-implemented exception and makes your program crash then it's a very bad idea.

If your code looks like this,

if (canReload(weapon)) {
   reload();
}
else if (canSharpen(weapon)) {
  sharpen();
}
else if (canPollish(weapon)) {
  polish();
}

then your code can become cluttered with very specific properties that has nothing to do with the abstract 'weapon' idea.

However if you are implementing a first person shooter and all your weapons can shoot/reload except that one knife then (in your specific context) it makes very much sense to have the reload of your knife do nothing since that is the exception and the odds of having your base class cluttered with specific properties is low.

Update: Do try to think about the abstract case/terms. For example, maybe every weapon has a "prepare" action which is a reload for guns and an unsheath for swords.

  • Let's say I have an internal weapons dictionary that holds the actions for the weapons, and when the user passes in "Reload" it checks the dictionary, ex, weaponActions.containsKey(action) if so, grab the object associated with it, and do it. Rather than a weapon class with multiple if statements – user286462 Oct 22 '17 at 13:27
  • See edit above. This is what i had in mind when using the SP – user286462 Oct 22 '17 at 13:31
0

Obviously it is Ok if you don't create a subclass with the intent of substituting an instance of the base class, but if you create a subclass using the base class as a convenient repository of functionality.

Now whether that is a good idea or not is very debatable, but if you never substitute the subclass for the baseclass, then the fact that it doesn't work is no problem. You may have problems, but LSP is not the problem in this case.

0

The LSP is good because it allows the calling code not to worry about how the class works.

eg. I can call Weapon.Attack() on all the Weapons mounted on my BattleMech and not worry that some of them might throw an exception and crash my game.

Now in your case you want to extend your base type with new functionality. Attack() isnt a problem, because the Gun class can keep track of its ammo and stop firing when it runs out. But Reload() is something new and not part of being a weapon.

The easy solution is to downcast, I don't think you need to worry about the performance overly, your not going to be doing it every frame.

Alternatively you can reassess your architecture and consider that in the abstract all Weapons are reloadable, and some weapons just never need reloading.

Then you aren't extending the class for guns anymore, or violating the LSP.

But it is problematic long term because you are bound to think up more special cases, Gun.SafteyOn(), Sword.WipeOffBlood() etc and if you put them all in Weapon, then you have a super complicated generalised base class that you keep having to change.

edit: why the strategy pattern is Bad(tm)

It's not, but consider the setup, performance and overall code.

I have to have some config somewhere which tells me that a gun can reload. When I instantiate a weapon, I have to read that config and dynamically add all the methods, check there are no duplicate names etc

When I call a method I have to loop through that list of actions and do a string match to see which to call.

When I compile the code and call Weapon.Do("atack") instead of "attack" I wont get an error on compilation.

It can be a suitable solution for some problems, say you have hundreds of weapons all with different combinations of random methods, but you lose a lot of the benefits of OO and strong typing. It doesn't really save you anything over downcasting

  • I think the SP can handle all that(see edit above), gun would have SafteyOn() and Sword would have wipeOffBlood(). Each weapon isn't aware of the other methods(and they shouldn't be) – user286462 Oct 22 '17 at 13:43
  • the SP is fine, but its equivalent to downcasting without type safety. I guess I was kinda answering a different question, let me update – Ewan Oct 22 '17 at 13:51
  • 2
    By itself the strategy pattern does not imply dynamic lookup of a strategy in a list or dictionary. I.e. both weapon.do("attack") and the type-safe weapon.attack.perform() may be examples of the strategy pattern. Looking up strategies by name is only necessary when configuring the object from a config file, though using reflection would be equally type-safe. – amon Oct 22 '17 at 16:12
  • that wont work in this situation as there are two separate actions attack and reload, which you need to bind to some user input – Ewan Oct 22 '17 at 17:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy