Preventing Liskov Substitution violation

Question

I am reimplementing some component and noticed that the original version has a Liskov Substitution violation. It's not all that critical, though I'd like to get rid of it in the new implementation . It is however not clear to me how to do this.

I have a component defining very simple value classes used by the system. This component has a DataValue interface and a dozen implementations such as NumberValue and GeoCoordinateValue.

The component I'm reworking is a storage layer meant for indexing data so it can easily be queried against. This component contains a set of classes that provide storage information for a particular DataValue implementing class. This information are things such as which fields a table needs to have to contain the DataValue in question, which field should be used for sorting, and what indexes should be placed. These classes all implement some DataValueHandler interface.

The LSP violation occurs for two particular methods in this DataValueHandler interface:

getWhereConditions( DataValue $dataValue )

getInsertValues( DataValue $dataValue )

The interface defines these methods take a DataValue. The implementations however expect the DataValue for which they provide information. For instance, the NumberValueHandler expects a NumberValue, and will throw an exception if it gets a GeoCoordinateValue.

So how can I get rid of this? (Before anyone suggests it: putting info of some specific storage backend in DataValue is not going to happen, as this would be worse design wise then the current LSP violation.)

Answer 1

The specific DataValue class should be a parameter for the DataValueHandler constructor and stored as a member variable, and the get methods should not include a DataValue object as a parameter.

(Some java-ish pseudocode below to illustrate)

class NumberDataValueHandler implements DataValueHandler {
    private NumberDataValue myDataValue;

    NumberDataValueHandler (NumberDataValue newNumberDataValue) {
       myDataValue = newNumberDataValue;
    }

    String getWhereConditions () {
        myDataValue.someNumberSpecificMethod();
        // do some NumberDataValue specific operations...
    }

}

Answer 2

Sorry for the very late answer, but this is a scenario which is valid to PHP to this day (v5.6/7.1), and is confounding to a lot of programmers. I've done a lot of research on the topic of LSP in regards to contractual obligations and violations, as I am developing a framework that is fully SOLID compliant, and I can say there are a lot of varied opinions on the matter.

Given that PHP does not currently have DbC (Design by Contract), and at best has invariant type hinting for method parameters and return types, we have to consider the underlying sentiment of the Liskov Substitution Principle. While it is possible to enforce parameter and return types in both interfaces and abstract functions, this is just one tool to ensure that Liskov is not violated, but it does not cover DbC. As Robert Martin states in his article The Liskov Substitution Principle (bold/italics added by me):

The LSP makes clear that in OOD the ISA relationship pertains to behavior. Not intrinsic private behavior, but extrinsic public behavior; behavior that clients depend upon

Under DbC, the validity of a DataType in relation to a consuming class is something that is conceptually identical across all concrete DataTypeHandlers - if you pass a DataType to a DataTypeHandler, it is either valid or invalid. We'll revisit this later.

Now, to the human mind it seems logical to implement that rule as a typed parameter in the method, like so:

public function getWhereConditions(DataValue $dataValue) {...}

// Override in a child class...
public function getWhereConditions(NumberDataValue $dataValue) {...}

But this violates the LSP as it involves a covariance of method parameters (i.e. NumberDataValue is a more refined type of DataValue), and changes the extrinsic public behaviour.

If, instead of applying this rule as a (public) method parameter type, we instead abstract it out to a (private) validity check against the passed argument, then we are shifting the behaviour from "public" to "private". As stated by Robert Martin, this is not a violation of LSP. (Read the last few pages at the end of his paper to see an example using PersistentSets that don't have an ISA relationship to Sets, which backs up this claim).

Here's an example in PHP which demonstrates what I mean. Note that the abstract methods have invariant parameters (i.e. DataType), but allow for variance in private behaviour. Also, note that the getWhereConditions() method is inherited by all concrete implementations, ensuring the public behaviour remains consistent (throw a DataTypeException, or returns a WhereConditionResultInterface):

abstract class DataHandler()
{
    public function getWhereConditions(DataValue $dataValue)
    {
        if(!$this->isDataValid($dataValid) {
            throw new DataTypeException('Invalid data given');
        }
        $this->processGetWhereConditions($dataValue);
    }
    
    abstract protected function isDataValid(DataValue $dataValue);
    /** @return WhereConditionResultInterface */
    abstract protected function processGetWhereConditions(DataValue $dataValue);
}

final class NumberDataHandler extends DataHandler
{
    protected function isDataValid(DataValue $dataValue)
    {
        return is_a($dataValue, NumberDataValueInterface::class);
    }
    
    protected function processGetWhereConditions(DataValue $dataValue)
    {
        /** @var NumberDataValueInterface $dataValue - Type hinting for modern IDEs */
        // Do stuff specific to NumberDataValueInterface here
    }
}

final class GeoCoordinateDataHandler extends DataHandler
{
    protected function isDataValid(DataValue $dataValue)
    {
        return is_a($dataValue, GeoCoordinateDataValueInterface::class);
    }
    
    protected function processGetWhereConditions(DataValue $dataValue)
    {
        /** @var GeoCoordinateDataValueInterface $dataValue - Type hinting for modern IDEs */
        // Do stuff specific to GeoCoordinateDataValueInterface here
    }
}

Answer 3

Do the DataValue classes all implement IDataValue or inherit from the same abstract DataValue class?

I'd expect all classes to have a common set of public methods, and for the GetWhereConditions only to act on these common methods (thus facilitating the substitution).

Otherwise they're just different classes, in which case you'd expect to see...

GetWhereConditions(NumberValue){...}
GetWhereConditions(GeoCoordinateValue){...}

...to reflect this.