7

I have a class Party that has a constructor that takes a Collection<Foo>. I plan to have two subclasses NpcParty and PlayerParty. All Party instances have an upper limit on the size of the input collection (6). However, an NpcParty has a lower bound of 1, while a PlayerParty has a lower bound of 0 (or rather, there is an implied minimum bound because a List cannot have a negative size). This is a violation of LSP because the NpcParty strengthens the preconditions in terms of the size of the input collection.

An NpcParty is intended to be immutable; meaning that the only Foos it will ever have, are those that are specified in the constructor. A PlayerParty's Foos can be changed at runtime, either in order, or reference/value.

public class Party {

    // collection must not be null
    // collection size must not exceed PARTY_LIMIT
    public Party(List<Foo> foos) {
        Objects.requireNonNull(foos);

        if (foos.size() > PARTY_LIMIT) {
            throw new IllegalArgumentException("foos exceeds party limit of " + PARTY_LIMIT);
        }
    }
}

public class NpcParty extends Party {

    // additional precondition that there must be at least 1 Foo in the collection
    public NpcParty(List<Foo> foos) {
        super(foos);

        if (foos.size() < 1) {
            throw new IllegalArgumentException("foos must contain at least 1 Foo");
        }
    }
}

public class PlayerParty extends Party {

    // no additional preconditions
    public PlayerParty(List<Foo> foos) {
        super(foos);
    }
}

In what ways can I resolve this violation, such that an NpcParty is allowed to have a minimum bound?

EDIT: I'm going to give an example of how I might test this.

Suppose I had a class AbstractPartyUnitTests that tested the minimum contract of all Party implementations.

public class AbstractPartyUnitTests {

    @Test(expected = NullPointerException.class)
    public void testNullConstructor() {
        createParty(null);
    }

    @Test
    public void testConstructorWithEmptyList() {
        party = createParty(new ArrayList<Foo>());

        assertTrue(party != null);
    }

    @Test(expected = IllegalArgumentException.class)
    public void testConstructorThatExceedsMaximumSize() {
        party = createParty(Stream.generate(Foo::new)
                                  .limit(PARTY_LIMIT + 1)
                                  .collect(Collectors.toList()));
    }

    protected abstract Party createParty(List<Foo> foos);

    private Party party;
}

With subclasses for PlayerParty and NpcParty

public class PlayerPartyUnitTests extends AbstractPartyUnitTests {

    @Override
    protected Party createParty(List<Foo> foos) {
        return new PlayerParty(foos);
    }
}

and

public class NpcPartyUnitTests extends AbstractPartyUnitTests {

    @Test
    public void testConstructorThatMeetsMinimumSize() {
        party = createParty(Stream.generate(Foo::new)
                                  .limit(1)
                                  .collect(Collectors.toList());

        assertTrue(party != null);
    }

    @Test(expected = IllegalArgumentException.class)
    public void testConstructorThatDoesNotMeetMinimumSize() {
        party = createParty(new ArrayList<Foo>());
    }

    @Override
    protected Party createParty(List<Foo> foos) {
        return new NpcParty(foos);
    }
}

All of the test cases would pass, with the exception of testConstructorWithEmptyList while running as an NpcPartyUnitTests. The constructor would fail, and as such, the test would fail.

Now, I could remove that test from the AbstractPartyUnitTests class, as it does not really apply to all types of Party; but then anywhere I have a Party, I may not be able to have a 1:1 replacement of NpcParty.

Zymus
  • 2,403
  • 3
  • 19
  • 35
  • 2
    See [here](http://stackoverflow.com/questions/5490824/should-constructors-comply-with-the-liskov-substitution-principle) and [here](http://stackoverflow.com/questions/35816023/is-it-possible-to-violate-liskov-substitution-principle-in-a-constructor). – Euphoric Apr 22 '16 at 07:59
  • Neither `NpcParty` nor `PlayerParty` seem to extend `Party`. – Tulains Córdova Apr 22 '16 at 13:54
  • @TulainsCordova Apologies, I've updated the code now. – Zymus Apr 22 '16 at 14:19
  • Your test example just tests the different behaviour of the constructors - but as I already wrote in my answer, that is not what the LSP is about - LSP says an NpcParty **object** behaves the same as each `Party` object - it does not say the constructors behave the same. Read the links given by Euphoric for getting a better understanding of that. – Doc Brown Apr 22 '16 at 23:07

2 Answers2

8

In your example above there is actually no violation of the LSP. Each NpcParty object is still a valid Party object and can be used for it in exchange. The LSP is not about exchanging class A by class B, it is about exchanging objects of type A by objects of type B. Thus, constructors, and constraints which are only checked in there, are not subject to the LSP.

For example, lets assume Player has an additional property "Size". You try to write a test case where you inject the player from outside, and want to test if that size is always in the allowed bounds:

 void TestSize(Player p)
 {
       AssertTrue(p.Size>=0);
       AssertTrue(p.Size<=PARTY_LIMIT);

       // ??? try to write a test for "Size" which does not fail for
       // ordinary Player objects, or PlayerParty objects, but fails for NpcPLayer
       // -> not possible without explicitly checking the type
 }

As you see, whatever you pass into that test method, the test does not fail. There is nothing in here where you cannot replace ordinary Player objects by NpcPlayer Objects.

If, however, your Party class would store its input somewhere and provides methods to change the number of elements afterwards (checking a constraint that the number of elements has to greater zero), then having a derived class with a stronger constraints would violate the LSP, since then you could not use an NpcParty object as an exchange for a Party.

To solve this, implement Party more general - inject the minimum and maximum bounds for the collection by the constructor and store them somewhere. That way, each user of a Party object must not assume that these bounds are zero and PARTY_LIMIT - that makes NpcParty a valid exchange for Party.

Doc Brown
  • 199,015
  • 33
  • 367
  • 565
  • The only class that would be able to modify the input collection would be the `PlayerParty`. All `NpcParty` objects would be essentially immutable. Typical use-case would be to load the `Foo`s from a repository, and would not be reassigned during runtime. If one wanted to change a value inside one of the `Foo`s, they would have to do it in the repository. Only the `Foo`s inside `PlayerParty`s can be modified during runtime, either in order, or reference. – Zymus Apr 22 '16 at 07:05
  • @DocBrown *"having a derived class with a stronger constraints would violate the LSP"* - I'd rather say with *conflicting* constraints, e.g. ones that make it impossible to satisfy `Party` requirements and `NpcParty` requirements at the same time. If you satisfy a stronger constraint, you automatically satisfy the weaker one by definition and pass the LSP test successfully. – guillaume31 Apr 22 '16 at 07:13
  • @guillaume31 that's incorrect: LSP mandates that derived classes (subtypes) cannot strengthen preconditions. See http://programmers.stackexchange.com/a/187615/167734 – Evan Apr 22 '16 at 07:20
  • 1
    @DavidPacker your example isn't substitution to me. `new NpcParty(collection)` wouldn't work in the first place, regardless if you assign it to a `Party` or anything else. – guillaume31 Apr 22 '16 at 07:22
  • 3
    @DavidPacker it seems to me that the premise of LSP is to have both a valid instance of type `T` and a valid instance of subtype `S`, otherwise it makes no sense trying to reason about what happens when you replace one with the other. – guillaume31 Apr 22 '16 at 07:35
  • @Evan I assumed DocBrown was talking about class invariants, not method preconditions (he used the term "*class* constraints"). A stronger class invariant cannot be likened to a stronger method precondition about a class member, since the state the object would have to be in upon method call isn't even attainable. Stronger class invariant is more like stronger method *post*conditions, which doesn't violate LSP. – guillaume31 Apr 22 '16 at 09:00
  • @guillaume31 again, that's incorrect for LSP. LSP requires that all properties provable about the supertype are also true for the subtype. In practice, this means invariants of the supertype must be preserved by the subtype: you cannot strengthen an invariant in a subtype because it would mean code relying on instances of the supertype was used would not necessarily be correct for the subtype, for instance code that assumes the party size can be 0. – Evan Apr 22 '16 at 09:12
  • 1
    @Evan The provable property is not that the size can be zero, it's that it is greater than or equal to zero. The property is provable for any instances of `Party` or `NpcParty`. – guillaume31 Apr 22 '16 at 09:23
1

Your code as stated is not necessarily a violation of the Liskov Substitution Principle: you can introduce an additional constraint in the constructor as long as instantiated NpcParty objects do not assume that their foos collections always have at least one member.

It doesn't seem like that will be the case, and you'll probably want to rely on that property of NpcParty objects in your application's business logic. If I'm right, you are correct that, in principle, this is a violation of the Liskov Substitution Principle.

You can fix this violation by moving any behavior that relies on party size constraints to the subclasses: PlayerParty should define a constraint on its party size just like NpcParty does now, and Party should make no claims about party size at all (or at the very least, only make claims that are true for all of its subtypes).

Evan
  • 1,245
  • 1
  • 9
  • 18