Does it violate SRP to reject subclass method parameter

Question

I was having a good read on Eric Lippert's blog about Wizard and Warriors.

It suggests the creation of a Rules class, quote:

We keep on talking about “rules”, and so apparently the business domain of this program includes something called a “rule”, and those rules interact with all the other objects in the business domain. So then should “rule” be a class? I don’t see why not!

The rules:

A warrior can only use a sword.
A wizard can only use a staff.

Maybe I'm not thinking about it in the right way, but suppose I have the following GameRules class:

public final class GameRules {

    public static boolean verifyifwizardcancarry(Weapon weapon){
        boolean canCarry  = false
        if weapon is a a staff set canCarry to true
        return canCarry;
    }
}

and Player:

public abstract class Player{   

   private List<Weapon> weapons;    
   public abstract void add(Weapon weapon);

}


public final class Wizard extends Player{

   @Override 
   public void add(Weapon weapon){

      if(GameRules.verifyifwizardcancarry(weapon){
          // - code to add weapon to inventory
      }
    }
 }

Does rejecting a weapon base on type (regardless of where that logic is placed) violate LSP?

In my add(Weapon weapon) method I'm promising that I will accept a Weapon of any kind, so to reject it based on type is a violation of LSP, correct? If so, how would I enforce the above rules?

Wizards don't extend Player - you're encoding rules implicitly in the type, rather than explicitly in actual running code. — Telastyn, Jun 05 '18 at 17:54
@Telastyn - I'm still learning here, but could you provide an example of what you're saying? `Wizard` in my example does `extend` `Player` — , Jun 05 '18 at 17:57
You need not promise that `add()` will always add. For example, [java.util.Collection.add()](https://docs.oracle.com/javase/7/docs/api/java/util/Collection.html#add(E)) might return false if the item were already there, or throw an IllegalWeaponException. Or, I don't see why the Wizard can't carry a sword, just don't allow him to **use** it later. — user949300, Jun 05 '18 at 18:21
@user949300 - So throwing an exception is not violation of LSP? I'm only following the rules stated in the blog, so I don't think I can prevent the Wizard from using it if the character has it, but it might beg the question of why I allowed the character to pick it up if they couldn't use it. Short answer, don't allow them to pick it up. The only way I can think of is to throw an exception based on type or return false. The `GameRules` class makes sense, because as stated in blog, rules can enforce other rules or depend on each other. It makes sense to keep the rules in place. — , Jun 05 '18 at 18:26
Throwing a Exception violates LSP iff your base class doesn't declare that in it's signature. So, in Player, I'd propose a change to `public abstract void add(Weapon weapon) throws IllegalWeaponException`. Of course, if this is D&D, can't that wizard be an Elf and be able to use a Longsword? — user949300, Jun 05 '18 at 18:58
The Wizards and Warriors post is actually a series of posts. Did you read all of them? — Robert Harvey, Jun 05 '18 at 20:02
@RobertHarvey - Sure did :D, but I still had questions, so I decided to ask. — , Jun 05 '18 at 23:10
@user949300 - Normally, in RPGs, at least the ones I've played, there is no rules about which weapons you can and cannot carry. As long as you have the necessary strength to use it, you are good. The only reason why I want to limit it here is because those are the rules specified in the blog. — , Jun 05 '18 at 23:14
The way I read that is not `verifyifwizardcancarry` should be a (static) method on `GameRules`, rather `verifyifwizardcancarry` is an *instance* of the *type* `Rule` — Caleth, Jun 06 '18 at 08:40
@Caleth Is it not `bool canCarry(Character, Item)` and `bool canUse(Character, Item)`? — Deduplicator, Jun 06 '18 at 12:58
@Caleth - I think your way is the correct way of looking at it. :D — , Jun 06 '18 at 15:57

Doc Brown · Accepted Answer · 2018-06-06T09:34:02.373

3

If a subclass violates the LSP can only be verified in context of the contract of a method. Some programming languages provide explicit language support for constracts, but even if not, one can always describe the contract in the documentation.

So lets say the abstract add method looks like this:

 // - may or may not add weapon to the list "weapons"
 // - if adding is not possible, an exception can be thrown
 public abstract void add(Weapon weapon);

as long as a subclass implements add obeying these rules, their is no LSP violation. However, if the contract reads

 // - adds weapon to the list "weapons"
 // - no exception will be thrown
 public abstract void add(Weapon weapon);

the implementation in your example will violate the LSP.

In case someone did not specify any behaviour, add has no "provable properties", so there is nothing to violate in terms of the LSP, at least not formally.

However, naming of the method, its parameters and choice of return type can implicitly lead to a certain expectation for the user of this interface, which can be interpreted as some kind of "informal" contract. For example, by naming the method TryAdd instead of just add, it would be much clearer that it does not guarantee to add the weapon to the list. By giving it a boolean return type, this would probably make the user expect the method not to throw an exception in case the weapon is not added. If a subclass then behaves differently, this can be seen as an "informal" violation of the LSP, or just a violation of the POLA.

edited Jun 06 '18 at 09:34

answered Jun 05 '18 at 19:52

Doc Brown

199,015
33
367
565

Yes, it's all a matter of the contract. Though I wouldn't say that if a contract isn't properly specified, it cannot be violated. No, instead all the existing code-base implementing and / or using it becomes that implied contract, whether you know the code, or even about the existence of that code, or not. Such an implied contract is just about impossible to verify or extend though, and thus work with. – Deduplicator Jun 05 '18 at 20:15
@Deduplicator: of course, there is more than just sticking formally to the LSP for writing error-free code. And sure, in reality, using code could make assumptions about the unspecified behaviour and it could also be implemented not as robust as it should be, which may create an implicit de-facto contract. But that's reality, not LSP in theory. – Doc Brown Jun 05 '18 at 20:49
@DocBrown - Would it be better to have `tryandadd(Weapon weapon)` as the method name? At least this way, the reader knows there is a possibility that adding might fail, in addition to the exceptions that are possible in the method signature. – Jun 05 '18 at 23:12
Good answer, but might want to mention it may be possible to prevent the exception by preventing the object from being added at compile time using the type system. – Frank Hileman Jun 05 '18 at 23:51
@Sveta: yes, I think so (actually I would call it `TryAdd`). Returning a boolean or some error object to show the success would give the caller the expection that function may not add anything, but won't throw an exception. This is the "principle of least surprise", which is also a way of describing a contract in an informal way. – Doc Brown Jun 06 '18 at 05:49
@DocBrown - Suppose if I have `boolean TryAdd(Weapon weapon)` and `Weapon` has a getter called `getWeaponType()`, which returns an enum of `WeaponType.Sword`. If I were to write an `if` statement in `TryAdd(Weapon weapon)` to reject the `weapon` if the `enum` returned was a `WeaponType.Sword, would this violate LSP? I don't think so, correct? The method contract states formally or informally that the weapon may or may not be added. – Jun 06 '18 at 16:03
@Sveta: I think you are right, wasn't that clear after what I wrote? – Doc Brown Jun 06 '18 at 17:08
@DocBrown - Just making sure :D. Thanks for the help. – Jun 06 '18 at 17:11

Ewan · Answer 2 · 2018-06-06T15:41:34.653

0

'Wizards and Warriors' is really a terrible example to choose. It opens the field to all sorts of solutions that don't really help explain the issue.

I much prefer Cats and Dogs

public abstract class Animal
{
    public abstract List<Animal> Children { get; set; }
}

public class Cat : Animal
{
    public override List<Cat> Children { get; set; }
}

public class Dog : Animal
{
    public override List<Dog> Children { get; set; }
}

This doesn't work even though it seems like it should. If you try:

public class Cat : Animal
{
    public override List<Animal> Children { get; set; }
}

then you can get

Cat.Children.Add(new Dog);

and you are back in the Wizards with Swords area. But while its conceivable for a wizard to hold a sword, no-one is going to think a cat can have puppies. That would be insane(1)

The very nature of inheritance is to allow the substitution that we are trying to prevent. You can throw exceptions, or just not add but then you break the LSP

The solution (well one of the solutions) is Generics

class Animal<T> where T : Animal<T>
{
    List<T> Childern;
}

class Cat : Animal<Cat> {}

Of course some classes can be an Ass and require more complex solutions.

(1) https://youtu.be/nC9GXlYb-ek

edited Jun 06 '18 at 15:41

answered Jun 05 '18 at 21:49

Ewan

70,664
5
76
161

Is throwing an exception from the subclass a violation of LSP? The answer above, states as long as it's specified in the method signature in the abstract class, it's not a violation. I'm not sure about C#, and how you would specify that a method throws a specific exception. – Jun 05 '18 at 23:16
yeah, unless you say it might throw. https://softwareengineering.stackexchange.com/questions/370580/ – Ewan Jun 05 '18 at 23:19
If I follow the example in the link, the `Character` and `Weapon` would have an `enum` type, and the `GamesRules` will check what character type is attempting to add what weapon type and if the character can't wield it, it will return false, correct? – Jun 05 '18 at 23:25
In the other example, the `EatenFood` and `Food` are essentially the `rules` in that they determine which Animal can eat it. correct? – Jun 05 '18 at 23:28
That's _why_ its a bad example. Of course you can redesign your classes to avoid the problem, as i do in that link, but it doesn't help demonstrate the problem – Ewan Jun 05 '18 at 23:29
Clarify something, the answer you provided in that link if one way to avoid violating LSP, correct? You removed the LSP violation and use of exceptions. – Jun 06 '18 at 00:06

Does it violate SRP to reject subclass method parameter

2 Answers2

Linked