20

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

}
Improve this question

5 Answers 5

Reset to default
19

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.

Improve this answer
3
  • Thanks! This helps. Hypothetically, the purpose of the doSomething() method would be type conversion to SpecialSomething: if it receives SpecialSomething it would just return the object unmodified, whereas if it receives a generic SomethingInterface object it'd run an algorithm to convert it to a SpecialSomething object. Since the preconditions and postconditions remain the same, I don't believe the contract has been violated.
    – Evan
    Commented Dec 30, 2015 at 11:51
  • 1
    @Evan Oh wow...that is an interesting case. That might actually be completely unproblematic. The only thing I can think of is that if you're returning the existing object instead of constructing a new object, maybe somebody depends on this method returning a brand new object...but whether that sort of thing can break people might depend on the language. Is it possible for someone to call y = doSomething(x), then x.setFoo(3), and then find that y.getFoo() returns 3?
    – Ixrec
    Commented Dec 30, 2015 at 11:56
  • That is problematic depending on the language, though it should be easily worked around by just returning a copy of the SpecialSomething object instead. Though for purity's sake, I could also see forgoing the special-case optimization when the object passed is SpecialSomething and just running it through the larger conversion algorithm since it should still work on account of it also being SomethingInterface object.
    – Evan
    Commented Dec 30, 2015 at 12:23
1

It's not a violation of the LSP. However, it's still a violation of the "don't do type checks" rule. It would be better to design the code in such a way that the special arises naturally; maybe SomethingInterface needs another member that could accomplish this, or maybe you need to inject an abstract factory somewhere.

However, this isn't a hard and fast rule, so you need to decide whether the trade-off is worth it. Right now you have a code smell, and a possible obstacle to future enhancements. Getting rid of it might mean a considerably more convoluted architecture. Without more information I can't tell which one is better.

Improve this answer
1

No, taking advantage of the fact that a given argument doesn't just provide the interface A, but also A2, does not violate the LSP.

Just make sure that the special path does not have any stronger pre-conditions (aside from those tested in deciding to take it), nor any weaker post-conditions.

C++ templates often do so to provide better performance, for example by requiring InputIterators, but giving additional guarantees if called with RandomAccessIterators.

If you have to decide at runtime instead (for example using dynamic casting), beware of the decision which path to take consuming all your potential gains or even more.

Taking advantage of the special case often goes against DRY (don't repeat yourself), as you might have to duplicate code, and against KISS (Keep it Simple), as it's more complex.

Improve this answer
0

There is a trade-off between the principle of "don't do type checks" and "segregate your interfaces". If many classes provide a workable but possibly-inefficient means of performing some tasks, and a few of those can also offer a better means, and there is a need for code which can accept any of the broader category of items that can perform the task (perhaps inefficiently) but then perform the task as efficiently as possible, it will be necessary to either have all objects implement an interface that includes a member to say whether the more efficient method is supported and another to use it if it is, or else have the code which receives the object check whether it supports an extended interface and cast it if so.

Personally, I favor the former approach, though I wish object-oriented frameworks like .NET would allow interfaces to specify default methods (making larger interfaces less painful to work with). If the common interface includes optional methods, then a single wrapper class can handle objects with many different combinations of abilities while promising for consumers only those abilities present in the original wrapped object. If many functions are split into different interfaces, then a different wrapper object will be needed for every different combination of interfaces that wrapped objects might need to support.

Improve this answer
0

Liskov substitution principle is about subtypes acting in accordance with a contract of their supertype. So, as Ixrec wrote, there is not enough information to answer whether it's an LSP violation.

What is violated here though is an Open closed principle. If you have a new requirement -- Do SpecialSomething magic -- and you have to modify existing code, then you're definitely violating OCP.

Improve this answer

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.