1

I've have an animal class

class Animal
{
    public function eat(Food $food);
}

the subclass who inherit it actually cannot support all kinds of Food (Cat can only eat meat):

class Cat extends Animal
{
    public function eat(Food $food)
    {
        if (!$food instanceof Meat) throw new InvalidArgumentException();
    }
}

of course, Meat is a subclass of Food

So is this code violate LSP (I think it does)? and how to re-design it?

====================================

PS. The description above is an edited version. the original version is like below:

I've defined a data transformer interface

interface TransformerInterface
{
    /**
     * @return mixed
     */
    public function transform($origin); 
}

As you could see, $origin and the return type could be any type of data (I use PHP), however, the class who implements it actually cannot support all kinds of data, (I think it should be OK if it returns certain type of data, it doesn't violate LSP):

class TagTransformer implements TransformerInterface
{
    public function transform($origin)
    {
        if (!is_string($origin)) throw new InvalidArgumentException();
        ...
    }
}

So is this code violate LSP (I think it does)? and how to re-design it?

  • What constraint do you think it violates? – candied_orange May 8 '18 at 0:28
  • 'TagTransformer' is a base class? – HungDL May 8 '18 at 2:34
  • thanks for the reply, as the LSP is related to "inherit" so I think the example I listed is no good. I'll change the description with a more suitable example. – chrisyue May 8 '18 at 8:31
  • 1
    As with every question about the LSP here, I need to point out that the LSP is about the documented contract of a class. There is no documented contract for your eat or transform method in your post. Is throwing an InvalidArgumentException an allowed action under the contract of the base class? If yes, it's not a violation, if no, it is a violation. – Sebastian Redl May 9 '18 at 9:04
  • 1
    Possible duplicate of Is this a violation of the Liskov Substitution Principle? – Sebastian Redl May 9 '18 at 9:05
5

The LSP:

Let φ(x) be a property provable about objects x of type T. Then φ(y) should be true for objects y of type S where S is a subtype of T.

What is provable about Animal? you don't provide code or return type, Presumably something like:

(Animal.Hungry = false AND Food.State = Consumed) OR InvalidArgumentException thrown

Is it still true for Cat? Yes. (assuming you add the state changes)

Now what about Food and Meat? You don't prove any code, so we can assume the only difference is the Type. No violation is possible.

What about that type checking statement in Cat though? this looks bad, its not a LSP violation, but it does raise questions about whether Food has a violation in it. If it doesn't then I shouldn't need to check the Type right?

Really the code smell is pointing out that Food should have some sort of FoodType property or CanBeEatenBy(Animal animal) method, rather than using SubTypes to identify the type.

Edit: it seems the correct design is in scope...

I'm going to use c#, which has a "flaw" in that you cant specify what exceptions might be thrown in the class defintion. That being the case, and respecting the normal rule about not using exceptions for logic, I will try to avoid all exceptions.

Obviously this isnt the only way to do this, its just a way to avoid your code smell and problems with exceptions.

public enum AnimalType
{
    Omnivore,
    Herbivore,
    Carnivore
}

public class Food
{
    public bool IsEdibleBy(Animal a)
    {
        return true;
    }
}

public class EatenFood
{
    public override bool IsEdibleBy(Animal a)
    {
        return false;
    }
}

public class Animal
{
    public readonly AnimalType Type = AnimalType.Omnivore;
    public Food Eat(Food f)
    {
         if(f.IsEdibleBy(this))
         {
              return new EatenFood(f);
         }
         else
         {
              return f;
         }
    }
}

What is proveable?

Eat always returns a type of Food 
IsEdibleBy always returns true or false
AnimalType is one of the enum values

Now we add Cat:

public class Cat : Animal
{
    public Cat() { this.Type = AnimalType.Carnivore; }
}

Public class Meat : Food
{
   public override bool IsEdibleBy(Animal a)
   {
       return a.Type == AnimalType.Omnivore || 
              a.Type == AnimalType.Carnivore;
   }
}

Now you have avoided all exceptions and LSP questions, you can extend you food types indefinitely, although you may have issues if you add more AnimalType enums. It would be important to make sure that that list is complete at the start

  • Note that in OP's example, your quote of the LSP does fail. Provable property "cat eats food" is violated for Meat, which is a subtype of Food. – Flater May 8 '18 at 10:02
  • @Flater I include the possibly of the invalidarg exception in the provable statement for animal to avoid this violation. The problem of course with exceptions is that in fact there is a massive list of technically possible exceptions for any function and that "does it terminate?" is... hard to prove – Ewan May 8 '18 at 10:10
  • @Ewan as you are the only answerer who answers the second question with details, I decide to mark your answer as the right answer :) – chrisyue May 9 '18 at 12:35
7

Note
By the time I wrote my answer, you had changed your example from an interface implementation to a base class inheritance. My answer is still correct; since the LSP applies to interfaces and base classes as per this StackOverflow answer.

You did change more in the example than just inheritance/interface, which does influence the answer. I've addressed this at the bottom of this answer.

This is not a violation of the LSP.

It would be a violation if the method is wholly unusable, i.e. you're not supposed to ever call the method TagTransformer.transform.


Examples of the difference.

Excuse the C# syntax, it's been a while since I did PHP.

public interface ICalculation
{
    void SetFirstNumber(int num);
    void SetSecondNumber(int num);
    int CalculateOutputFromNumbers();
}

Implementation 1:

public class Addition : ICalculation
{
    public void SetFirstNumber(int num)
    {
        this.Number1 = num;
    }

    public void SetSecondNumber(int num)
    {
        this.Number2 = num;
    }

    public int CalculateOutputFromNumbers()
    {
        return this.Number1 + this.Number2;
    }
}

This is clearly using the interface as intended. No issue here.

Implementation 2:

public class SquareRoot: ICalculation
{
    public void SetFirstNumber(int num)
    {
        this.Number1 = num;
    }

    public void SetSecondNumber(int num)
    {
        throw new Exception("Square roots only take one input value!);
    }

    public int CalculateOutputFromNumbers()
    {
        return Math.Sqrt(this.Number1);
    }
}

Notice how you're never going to be allowed to call the SetSecondNumber method from SquareRoot, even though the method is part of the ICalculation interface and SquareRoot implements ICalculation.

This violates LSP. In order to calculate a square root, SquareRoot class needs to be treated differently from other ICalculation-implementing classes.

Implementation 3:

public class Division : ICalculation
{
    public void SetFirstNumber(int num)
    {
        this.Number1 = num;
    }

    public void SetSecondNumber(int num)
    {
        if(num == 0)
            throw new Exception("You can't divide by zero!"); 

        this.Number2 = num;
    }

    public int CalculateOutputFromNumbers()
    {
        return this.Number1 / this.Number2;
    }
}

Based on your question, it seems that you think this is a violation of LSP. This is essentially what's happening in your code example, a specific exception is being thrown for a given invalid value.

This is not a violation. Notice how you're allowed to call the SetSecondNumber method from Division, but you simply can't use an impossible value (0).

This isn't a matter of having to use the Division class differently from ICalculation-implementing classes; it's simply a matter of bad input, no possible output.

That is significantly different from the SquareRoot example; at least in relation to LSP.


In response to your new example

Your new example does in a way invalidate your original question.

Your old example was a PHP snippet:

public function transform($origin)
{
    if (!is_string($origin)) throw new InvalidArgumentException();
    ...
}

It's important to note here that there is no type constraint on $origin. This means that checking for a usable type is a logical consequence, and not inherently bad design (since the language allows for untyped parameters).

However, this is significantly different in your revised example:

public function eat(Food $food)
{
    if ($food instanceof Meat) throw new InvalidArgumentException();
    ...
}

It's important to note here that there is a type constraint on $food. You're no longer using a typeless parameter. You're specifying that the parameter is of type Food.

At this point, it becomes an LSP violation. Your input validation is no longer a matter of how the language works; but rather a consequence of the contract that is specified by your Animal base class.

You're trying to create a Cat which inherits from Animal but actually (partially) outright refuses to implement Eat(Food). That is a willful exclusion of functionality, which does make it an LSP violation.

I would consider this an LSP violation of Meat/Food, more than it is an LSP violation of Cat/Animal. Meat is clearly being treated differently from Food, which violates the contract that says that Meat is a Food.

  • 2
    I think both your and the question's examples are LSP violations if it is not part of the interface contract that invalid arguments throw in that way. If eat is defined as "Instance eats the specified food or throws InvalidArgumentException if it's incompatible" then we're good. If it's merely "Instance eats the specified food" then we're not good, but also the Animal interface was poorly designed. – Phoshi May 8 '18 at 8:49
  • @Phoshi: I was already drafting an update to the updated example (see my answer now). The difference is that in OP's initial example, the exception was related to the language used (which allows typeless parameters - thus opening the door to unexpected types being passed - thus making it acceptable to throw on unusable types, in my opinion). However, in the revised example, he's now working with a typed parameter (which in and of itself displays inheritance on top of that), which does significantly change the violation here. I hope you agree with my updated answer. – Flater May 8 '18 at 8:54
  • @Phoshi: Maybe a better reply after re-reading your comment: Regardless of the contract definition, if no meaningful result can be achieved; then an exception is the only reasonable outcome. LSP or not, if there's no other option than throwing an exception, throwing an exception can't be wrong or in violation of anything. – Flater May 8 '18 at 11:16
  • I think the origin example also break the LSP because the original contract TransformerInterface::transform is considered to handle "everything", like PHP's var_dump function. However the TagTransformer::transform only can handle "string". which should be more appropriate as TagTransformer::transform(string $origin). no matter there's typehint or not, I always fell it's a code smell of type checking for parameters, no matter it's "typeof" or "is_xxx" – chrisyue May 8 '18 at 11:53
  • @chrisyue: Those are valid concerns (I don't like untyped parameters for exactly this reason), but that's not really what LSP is about. Having to type check is a logical consequence of not enforcing a type to begin with. If you don't like type checking, then enforce a type. Problem solved. Type checking of typed variables (e.g. Base b = new Derived(); if(b is Derived) { HandleDerived(d); }) is more often than not an abuse of polymorphism; and I suggest avoiding this altogether to prevent future code smells and LSP violations. – Flater May 8 '18 at 11:59
2

If it is documented that not every type of object is suitable for every subclass/implementation -- and that passing an unsuitable argument may result in an InvalidArgumentException -- then you're semi-OK. You've introduced this restriction in the superclass/interface, so you're not breaking the rules by enforcing it in subclasses. You're forcing a DIP violation on the caller, but you've basically satisfied LSP.

Otherwise, the fact that $animal->eat($broccoli) may break, and you can't know that without knowing about the subtype, means you're violating LSP. Methods succeed by default. (Exceptions are called "exceptions" for a reason.) So a declaration of eat sans failure conditions is a promise that $animal->eat($food) will at least succeed. Cat is unpromising that.

One way to resolve both issues is to provide a can_eat method on Animal, and declaring that if $animal->can_eat($food) is false, then $animal->eat($food) will (probably) fail.

  • What does DIP stand for? – chrisyue May 9 '18 at 12:46
  • 1
    The Dependency Inversion Principle. (It's the "D" in SOLID. LSP is the "L".) Abstracted out (cause lol), it means that you shouldn't depend on details, but on abstractions. It's the principle behind "programming to the interface". (In the example, the caller has to care about the specifics of the Animal and Food it has, which makes things more fragile.) – cHao May 9 '18 at 13:22
1

The animal example is a violation which can be fixed easily. Do not mention food in the base class, make it so: abstract Animal -> abstract Eater -> astract MeatEater -> Cat

The transformer example is OK. It is obvious that any particular transformer will not be able to transform anything into anything else, it merely states it can transform something into something else, that it features a (polymorph) Transform method.

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.