Is it ever okay to violate the LSP?

Question

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
    }
}

Answer 1

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().

Answer 2

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.

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

Answer 4

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.

Answer 5

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