Liskov Substitution Principle – Do Special-Cases with Fallbacks Violate It?

liskov-substitutionobject-orientedsolid

Let's say I have an interface FooInterface that has the following signature:

interface FooInterface {
    public function doSomething(SomethingInterface something);
}

And a concrete class ConcreteFoo that implements that interface:

class ConcreteFoo implements FooInterface {

    public function doSomething(SomethingInterface something) {
    }

}

I'd like to ConcreteFoo::doSomething() to do something unique if it's passed a special type of SomethingInterface object (let's say it's called SpecialSomething).

It's definitely a LSP violation if I strengthen the preconditions of the method or throw a new exception, but would it still be an LSP violation if I special-cased SpecialSomething objects while providing a fallback for generic SomethingInterface objects? Something like:

class ConcreteFoo implements FooInterface {

    public function doSomething(SomethingInterface something) {
        if (something instanceof SpecialSomething) {
            // Do SpecialSomething magic
        }
        else {
            // Do generic SomethingInterface magic
        }
    }

}

Best Answer

It might be a violation of LSP depending on what else is in the contract for the doSomething method. But it's almost certainly a code smell even if it doesn't violate LSP.

For example, if part of the contract of doSomething is that it will call something.commitUpdates() at least once before returning, and for the special case it calls commitSpecialUpdates() instead, then that is a violation of LSP. Even if SpecialSomething's commitSpecialUpdates() method was consciously designed to do all of the same stuff as commitUpdates(), that's just preemptively hacking around the LSP violation, and is exactly the sort of hackery one would not have to do if one followed LSP consistently. Whether anything like this applies to your case is something you'll have to figure out by checking your contract for that method (whether explicit or implicit).

The reason this is a code smell is because checking the concrete type of one of your arguments misses the point of defining an interface/abstract type for it in the first place, and because in principle you can no longer guarantee the method even works (imagine if someone writes a subclass of SpecialSomething with the assumption that commitUpdates() will get called). First of all, try to make these special updates work within the existing SomethingInterface; that's the best possible outcome. If you're really sure you can't do that, then you need to update the interface. If you don't control the interface, then you may need to consider writing your own interface that does do what you want. If you can't even come up with an interface that works for all of them, maybe you should scrap the interface entirely and have multiple methods taking different concrete types, or maybe an even bigger refactor is in order. We'd have to know more about the magic you've commented out to tell which of these is appropriate.

Related Topic