Violation of Liskov Substitution Principle?

Question

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?

Answer 1

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, it's 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 can't specify what exceptions might be thrown in the class definition. That being the case, and respecting the normal rule about not using exceptions for logic, I will try to avoid all exceptions.

Obviously, this isn't the only way to do this, it's 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 provable?

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

Answer 2

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.

Answer 3

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.

Answer 4

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.

Answer 5

I wrote a function:

Makehappy(animal)
    Shop.buy(cream cake)
    animal.eat(cream cake)

And to my surprise, Makehappy(cat) Throws an exception.

The problem is: The contract for animal wasn’t stated correctly. The contract said wrongly: When you call animal.eat(food), the animal eats the food. Correct: when you call animal.eat(food), the animal either eats the food, or throws an exception. Better: The exception can tell you whether the food can still be given to another animal or not.