4

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

  • Are you looking for a solution in the language you are using? If so, which language is it? Also, are you actually using the DataValueHandler interface? – Aaron Kurtzhals Feb 27 '13 at 14:16
  • 1
    I've always assumed LSP applied to single dispatch, not behaviour of another class when passed different types - i.e. it would be a breach if NumberValueHandler did not fit the contract of DataValueHandler when passed a GeoCoordinateValue, but throwing an exception if the dynamic type doesn't match that expected is not ( unless DataValueHandler's contract prohibits throwing an exception ). I would probably replace with double dispatch though, unless there is another reason to have the handlers spread across different objects. – Pete Kirkham Feb 27 '13 at 15:52
  • The language I'm using is PHP, though I'm perfectly happy with the solution specified in another language, as long as it can also be expressed (sanely) in PHP :) – Jeroen De Dauw Mar 1 '13 at 15:14
  • @PeteKirkham If you pass a subtype of DataValue to getWhereConditions and you get an exception because it's not the correct subtype, is that not a clear violation of the LSP? Not sure how double dispatch would be helpful here - can you illustrate what you are thinking of? – Jeroen De Dauw Mar 1 '13 at 15:18
  • Say you have two implementations TypeA and TypeB of interface I. For every call in the interface I, TypeA and TypeB behave in accordance to LSP. Someone then creates a free function which takes an I, tests whether its type is TypeA, and if it is throws an exception. That doesn't effect LSP with respect to TypeA or TypeB - LSP applies to the types which vary, not the behaviour of other code. If instead of a free function, it is a method of another object which throws the exception, this does not alter the LSP status of TypeA or TypeB... – Pete Kirkham Mar 1 '13 at 15:40
4

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

}
  • Thanks for your reply. What you suggested works for the problematic methods yes, though is not very sensible for the others. Those are often needed in situations where I don't have any particular instance of the DataValue in question. Guess I should split up the class into a part with info for a type of DataValue and a part for a particular instance of that type. – Jeroen De Dauw Feb 27 '13 at 15:01
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 final and 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
    }
}
0

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.

  • DataValue is an interface implemented by all datavalues. – Jeroen De Dauw Feb 27 '13 at 14:59

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.