Search code examples
phpoopsolid-principlesdesign-by-contractliskov-substitution-principle

Liskov Substitution Principle (LSP) violated via Design By Contract (DBC)?


I'm writing a framework in PHP, and have run into a pattern which smells bad. It appears that I'm implementing a contract (q.v. Design By Contract) that violates the Liskov Substituion Principle (LSP). Since the original example is heavily abstracted, I'll put it into a real world context:

(n.b. I am not an engine/vehicle/vroom-vroom person, forgive me if it's unrealistic)


Suppose we have an anaemic abstract class for vehicles, and we furthermore have two sub-types of vehicles - those that can be refuelled and those that can't (e.g. pushbikes). For this example, we will only concentrate on the refuellable type:

abstract class AbstractVehicle {}

abstract class AbstractFuelledVehicle extends AbstractVehicle
{
    private $lastRefuelPrice;

    final public function refuelVehicle(FuelInterface $fuel)
    {
        $this->checkFuelType($fuel);
        $this->lastRefuelPrice = $fuel->getCostPerLitre;
    }

    abstract protected function checkFuelType(FuelInterface $fuel);
}

abstract class AbstractNonFuelledVehicle extends AbstractVehicle { /* ... */ }

Now, let's look at the "fuel" classes:

abstract class AbstractFuel implements FuelInterface
{
    private $costPerLitre;

    final public function __construct($costPerLitre)
    {
        $this->costPerLitre = $costPerLitre;
    }

    final public function getCostPerLitre()
    {
        return $this->costPerLitre;
    }
}

interface FuelInterface
{
    public function getCostPerLitre();
}

That is all the abstract classes done, now let's look at concrete implementations. First, two concrete implementations of fuel, including some anaemic interfaces so that we can type-hint/sniff them properly:

interface MotorVehicleFuelInterface {}

interface AviationFuelInterface {}

final class UnleadedPetrol extends AbstractFuel implements MotorVehicleFuelInterface {}

final class AvGas extends AbstractFuel implements AviationFuelInterface {}

Now finally, we have concrete implementations of vehicles, which ensure the correct fuel type (interface) is being used to refuel the particular vehicle class, throwing an Exception if it is incompatible:

class Car extends AbstractFuelledVehicle
{
    final protected function checkFuelType(FuelInterface $fuel)
    {
        if(!($fuel instanceof MotorVehicleFuelInterface))
        {
            throw new Exception('You can only refuel a car with motor vehicle fuel');
        }
    }
}

class Jet extends AbstractFuelledVehicle
{
    final protected function checkFuelType(FuelInterface $fuel)
    {
        if(!($fuel instanceof AviationFuelInterface))
        {
            throw new Exception('You can only refuel a jet with aviation fuel');
        }
    }
}

Car and Jet are both sub-types of AbstractFuelledVehicle, so according to LSP, we should be able to substitute them.

Due to the fact that checkFuelType() throws an Exception if the wrong sub-type of AbstractFuel is provided, this means that if we substitute the AbstractFuelledVehicle sub-type Car for Jet (or vice versa) without also substituting the relevant fuel sub-type, we will trigger an Exception.

Is this:

  1. A definite violation of LSP, as substitution should not cause behavioural change that results in Exceptions being thrown
  2. Not a violation at all, as the interfaces and abstract functions are all implemented correctly and can still be called without type violations
  3. A bit of a grey area, whose answer is subjective

Solution

  • Combining comments into an answer...

    I agree with the analysis of LSP: the original version is a violation, and we can always solve LSP violations by weakening the contract at the top of the hierarchy. However, I would not call this an elegant solution. Type checking is always a code smell (in OOP). In the OP's own words, "...including some anemic interfaces so that we can type-hint/sniff them..." What's being sniffed here is the stench of bad design.

    My point is that LSP is the least concern here; instanceof is an OO code smell. LSP compliance here is like fresh paint on a rotten house: it may look pretty, but the foundation is still fundamentally unsound. Eliminate type checking from the design. Only then worry about LSP.


    The SOLID principles of OO design in general, and LSP in particular, are most effective as part of a design that is, in fact, object oriented. In OOP, type checking is replaced by polymorphism.