A WordSorter
is-a WordBank
, so code that works with a WordBank
should also work when a WordSorter
is used instead of a WorkBank
. On the other hand, a WordSorter
is-not-a SomeWordBank
. The compiler won't even let you use a WordSorter
in place of a SomeWordBank
, so the issue does not even begin to arise.
There might be a LSP violation, but doesn't appear to be from the minimal specification you've given. Does WordSorter
guarantee, for example, that one can add arbitrary strings to it and retrieve them all in the same order later? Then sorting the words would indeed break the that contract, and code that works for "correct" WordBank
s can be broken by substituting a WordSorter
. Whether there is such a guarantee is impossible to tell from the minimal UML diagram you've shown. If, for example, WordBank
's contract says all words which are added are included in the result of getWords
, then:
bank.add(w);
AssertContains(bank.getWords(), w);
should always work. That code would break if bank
was a WordSorter
, and it's WordSorter
's fault for breaking the contract and hence violating the LSP. But if WordBank
offers no such guarantee, then the above code is in the wrong (in the same way asser x*y == 42
will usually fail) and WordSorter
is a perfectly well-behaved subclass.
Neither "transitive dependency towards the concrete elements" nor "supertype knowing about the subtypes" leads to a conclusion of LSP being violated. The first one is harmless. The second one is merely a smoking gun.
Visitor pattern can be used for several purposes. Judging from the way this question is asked, this answer will put the focus on the case of Double Dispatch.
Visitor pattern might also be used for other purposes. The possibility of those other purposes violating OCP or LSP is practically nil; therefore they will not be discussed in this answer.
In cases where the Visitor pattern is used to implement Double Dispatch (or equivalently to get around the lack of double dispatch in OOP languages), it is often said to be incompatible with Open/Closed Principle (OCP), although generally we can't say much about whether it violates Liskov Substitution Principle (LSP) unless there is an implementation mistake.
This article on Double Dispatch explains it better than I do.
An example of double dispatch is that you have one inheritance hierarchy of Shape
, and then a different inheritance hierarchy of Display
(or ISurface
in the article linked above). The project requires an implementation of a polymorphic method, whose behavior varies with both the concrete Shape
subtype and the concrete Display
subtype. If you have 3 concrete Shape
subtypes and 5 concrete Display
subtypes, this requires 15 concrete implementations of the polymorphic method, i.e. it is a Cartesian product of implementations.
The most widely used OOP languages do not support Double Dispatch out-of-the-box. There are several ways to get around this; using the Visitor pattern is one way.
However, using the Visitor pattern to achieve Double Dispatch requires you to lock down one of the two inheritance hierarchies: you will not be able to add support for new subtypes to the locked-down inheritance hierarchy anymore (without modifying the supertype/interface). Using the above example, you have to choose either: (1) lock down the Shape
hierarchy, or (2) lock down the Display
hierarchy. This breaks the "open for extension" part of the OCP.
If the nature of one of the inheritance hierarchy is that it is "complete", i.e. there will never be a future necessity to add any new subtypes to that hierarchy, then locking it down is not an issue, so it does not violate OCP.
There is an alternative implementation of Double Dispatch that does not break OCP, i.e. that will allow adding new subtypes to both the Shape
and Display
inheritance hierarchies, without modifying existing code. However, this alternative implementation does require checking the concrete types of both arguments via instanceof
.
Some would argue that checking the concrete argument types with instanceof
is itself an antipattern, or a smell, or something undesirable. This is somewhat an exaggeration of the original opinions held by Java mentors, which was to prefer polymorphism over instanceof and downcasting, not to outright forbid it even when necessitated by software requirements.
This brings up another discussion. In light of agile methodology, it is often the case that new software requirements necessitate code changes to certain parts of the project that were once considered immutable. In other words, breaking changes are sometimes needed.
From this perspective, the Open/Closed Principle (OCP) itself often came up as being too rigid/inflexible.
In the past, violation of OCP will cause harm to compiled libraries (binaries) which are then packaged and shipped. Propagating breaking changes in upstream dependencies often has a ripple effect that result in a large number of compiled libraries to be updated.
Agile methodology intends to solve this issue with:
- Versioning (leave the old system in place, but introduce the new system as an alternative, while making both available to customers)
- Continuous integration (literally, recompile everything, every time)
- Open source (customers can see and modify the source code inside dependencies)
Agile methodology also views violations of Liskov Substitution Principle in a different way. Instead of branding a violation as a sign that the system or design is unsafe or having questionable correctness, it is merely seen as being a pain point. So, a LSP violation is not a death knell, but is a bad mark that adds up.
There are plenty of new software requirements which require the violation of LSP. In the past, such feature requests will be rejected on the grounds of correctness. Nowadays, it will be given business considerations. If a violation of LSP is accepted, then it shall be properly documented.
The additional cost of documenting an LSP violation, the displeasure of a programmer caught by the astonishment of software breakage (due to not heeding the documented warning), and the software maintenance and support overhead stemming from such breakage, are seen as business costs that are weighted against the business gains from implementing the requested feature.
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 callsomething.commitUpdates()
at least once before returning, and for the special case it callscommitSpecialUpdates()
instead, then that is a violation of LSP. Even ifSpecialSomething
'scommitSpecialUpdates()
method was consciously designed to do all of the same stuff ascommitUpdates()
, 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 thatcommitUpdates()
will get called). First of all, try to make these special updates work within the existingSomethingInterface
; 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.