Reputation: 4820
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:
Upvotes: 0
Views: 573
Reputation: 17144
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.
Upvotes: 1
Reputation: 4820
On second thoughts, I believe this is a technical violation of the Liskov Substitution Principle. A way to rephrase the LSP is "a subclass should require nothing more and promise nothing less". In this case, both the Car and Jet concrete classes are requiring a specific type of fuel in order for code execution to continue (this is the violation of the LSP), and additionally the method checkFuelType() could be overridden to include all sorts of weird and wonderful behaviour. I think a better approach is this:
Alter the AbstractFuelledVehicle class to check the fuel type before committing to refuelling:
abstract class AbstractFuelledVehicle extends AbstractVehicle
{
private $lastRefuelPrice;
final public function refuelVehicle(FuelInterface $fuel)
{
if($this->isFuelCompatible($fuel))
{
$this->lastRefuelPrice = $fuel->getCostPerLitre;
} else {
/*
Trigger some kind of warning here,
whether externally via a message to the user
or internally via an Exception
*/
}
}
/** @return bool */
abstract protected function isFuelCompatible(FuelInterface $fuel);
}
To me, this is a much more elegant solution, and doesn't have any sort of code smell. We can swap the fuel from UnleadedPetrol to AvGas and the behaviour of the superclass remains the same, albeit with two possible outcomes (i.e. it's behaviour is not dictated to it by the concrete class, which could throw an Exception, log an error, dance a jig, etc)
Upvotes: 0