Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

This image is taken from Applying Domain-Driven Design and Patterns: With Examples in C# and .NET

enter image description here

This is the class diagram for the State Pattern where a SalesOrder can have different states during its life time. Only certain transitions are allowed between the different states.

Now the OrderState class is an abstract class and all its methods are inherited to its subclasses. If we consider the subclass Cancelled which is a final state that doesn't allow any transitions to any other states, we'll have to override all its methods in this class to throw exceptions.

Now doesn't that violate Liskov's substitution principle since a sublcass shouldn't alter the behavior of the parent? Does changing the abstract class into an interface fixes this?
How can this be fixed?

share|improve this question
    
Change OrderState to be a concrete class that throws "NotSupported" exceptions in all its methods by default? –  James Jan 8 '13 at 18:34
    
I haven't read this book, but it seems weird to me that OrderState contains Cancel() and then there is Cancelled state, that is different subtype. –  Euphoric Jan 8 '13 at 19:53
    
@Euphoric why is it weird? calling cancel() changes the state to cancelled. –  Songo Jan 8 '13 at 19:56
    
How? How can you call Cancel on OrderState, when instance of state in SalesOrder is supposed to change. I think the whole model is wrong. I would like to see full implementation. –  Euphoric Jan 8 '13 at 19:58
add comment

3 Answers

up vote 2 down vote accepted

This particular implementation, yes. If you make the states concrete classes rather than abstract implementors then you will get away from this.

However the state pattern you're referring to which is effectively a state machine design is in general something I disagree with from the way I've seen it grow. I think there is sufficient grounds to decry this as a violation of single responsibility principle because these state management patterns end up being central repositories for knowledge of the current state of many other parts of a system. This state management piece being centralized will more often than not require business rules relevant to many different parts of the system to reasonably coordinate them.

Imagine if all the parts of the system which care about state were in different services, different processes on different machines, a central status manager that details the status for each of those places effectively bottlenecks this whole distributed system and I think that bottleneck is a sign of a SRP violation as well as generally bad design.

In contrast, I would suggest one make objects more intelligent as in the Model object in the MVC pattern where the model knows how to handle itself, it doesn't need an outside orchestrator to manage it's internal workings or reason for it.

Even putting a state pattern like this inside of an object so it is only managing itself, it feels like you would be making that object too large. Workflows should be done through composition of various self-responsible objects I would say, rather than with a single orchestrated state which manages the flow of other objects, or the flow of intelligence within itself.

But at that point it's more art than engineering and so it is definitely subjective your approach to some of those things, that said the principles are a good guide and yes the implementation you list is an LSP violation, but could be corrected not to be. Just be very careful about the SRP when using any pattern of this nature and you're likely to be safe.

share|improve this answer
    
Basically, the big problem is that object orientation is good for adding new classes but makes it hard to add new methods. And in the case of state, as you said, it is not likely that you will need to extend the code with new states very often. –  hugomg Jan 8 '13 at 20:37
2  
@Jimmy Hoffa Sorry, but what do u mean exactly by "If you make the states concrete classes rather than abstract implementors then you will get away from this." ? –  Songo Jan 8 '13 at 22:35
add comment

a sublcass shouldn't alter the behavior of the parent?

That's a common misinterpretation of LSP. A subclass can alter the behavior of the parent, as long as it remains true to the parent type.

There is a good long explanation on Wikipedia, suggesting the things that will breach LSP:

... there are a number of behavioral conditions that the subtype must meet. These are detailed in a terminology resembling that of design by contract methodology, leading to some restrictions on how contracts can interact with inheritance:

  • Preconditions cannot be strengthened in a subtype.
  • Postconditions cannot be weakened in a subtype.
  • Invariants of the supertype must be preserved in a subtype.
  • History constraint (the "history rule"). Objects are regarded as being modifiable only through their methods (encapsulation). Since subtypes may introduce methods that are not present in the supertype, the introduction of these methods may allow state changes in the subtype that are not permissible in the supertype. The history constraint prohibits this. It was the novel element introduced by Liskov and Wing. A violation of this constraint can be exemplified by defining a MutablePoint as a subtype of an ImmutablePoint. This is a violation of the history constraint, because in the history of the Immutable point, the state is always the same after creation, so it cannot include the history of a MutablePoint in general. Fields added to the subtype may however be safely modified because they are not observable through the supertype methods. One may derive a CircleWithFixedCenterButMutableRadius from ImmutablePoint without violating LSP.

Personally, I find it easier simply to remember this: If I'm looking at a parameter in a method that has type A, would someone passing a subtype B cause me any surprises? If they would then there is a breach of LSP.

Is throwing an Exception a surprise? Not really. It's something that can happen at any time, whether I'm calling the Ship method on OrderState or Granted or Shipped. So I have to account for it and it's not really a breach of LSP.

That said, I do think there are better ways to handle this situation. If I were writing this in C#, I would use interfaces and check for the implementation of an interface before calling the method. For example, if the current OrderState doesn't implement IShippable, don't call a Ship method on it.

But then I also wouldn't use the State pattern for this particular situation. The State pattern is much more appropriate to the state of an application than to the state of a domain object like this.

So, in a nutshell, this is a poorly contrived example of the State Pattern and not a particularly good way to handle the state of an order. But it arguably doesn't breach LSP. And the State pattern, in and of itself, certainly doesn't.

share|improve this answer
    
It sounds to me like you're confusing LSP with Principle of least surprise/astonishment, I believe that LSP has more clear-cut boundaries where this is violating them, though you are applying the more subjective POLA which is based on expectations which are subjective; that is each person expects something different than the next. LSP is based on contractual and well defined guarantees given by the provider, not subjectively assessed expectations guessed by the consumer. –  Jimmy Hoffa Jan 8 '13 at 20:05
    
@JimmyHoffa: You're right, and that's why I spelled out the exact meaning before I stated a simpler way of remembering LSP. However, if there is a question in my mind what "surprise" means then I'll go back and check the exact rules of LSP (can you really recall them at will?). Exceptions replacing functionality (or vice versa) is a difficult one because it doesn't breach any specific clause above, which is probably a clue that it should be handled an entirely different way. –  pdr Jan 8 '13 at 20:26
1  
An exception is an expected condition defined on the contract in my mind, think of a NetworkSocket, it may have an expected exception on send if it's not open, if your implementation doesn't throw that exception for a closed socket you're violating LSP, if the contract states the opposite and your subtype does throw the exception then you are violating LSP. That's more or less how I classify exceptions in LSP. As for remembering the rules; I could be wrong on this and feel free to tell me so, but my understanding of LSP vs POLA is defined in the last sentence of my comment above. –  Jimmy Hoffa Jan 8 '13 at 21:08
1  
@JimmyHoffa: I had never considered POLA and LSP to be even remotely connected (I think of POLA in UI terms) but, now that you've pointed it out, they kind of are. I'm not sure they're in conflict though, or that one is more subjective than the other. Isn't LSP an early description of POLA in software engineering terms? In the same way as I get frustrated when Ctrl-C isn't Copy (POLA), I also get frustrated when an ImmutablePoint is mutable because it's actually a MutablePoint subclass (LSP). –  pdr Jan 8 '13 at 21:30
1  
I would consider a CircleWithFixedCenterButMutableRadius as a probable LSP violation if the consumer contract for ImmutablePoint specifies that if X.equals(Y), consumers may freely substitute X for Y, and vice versa, and must thus refrain from using the class in such a way that such substitution would cause trouble. The type might be able to legitimately define equals such that all instances would compare as distinct, but such behavior would likely limit its usefulness. –  supercat Mar 25 at 19:38
show 3 more comments

(This is written from the point of view of C#, so no checked exceptions.)

According to Wikipedia article about LSP, one of the conditions of LSP is:

No new exceptions should be thrown by methods of the subtype, except where those exceptions are themselves subtypes of exceptions thrown by the methods of the supertype.

How should you understand that? What exactly are “exceptions thrown by the methods of the supertype” when the methods of the supertype are abstract? I think those are the exceptions that are documented as the exceptions that can thrown by the methods of the supertype.

What this means is that if OrderState.Ship() is documented as something like “throws InvalidOperationException if this operations is not supported by the current state”, then I think this design does not break LSP. On the other hand, if the supertype methods are not documented this way, LSP is violated.

But this does not mean this is good design, you should not use exceptions for normal control flow, which this seems very close to. Also, in a real application, you would probably want to know whether an operation can be performed before you attempt to do it, for example to disable the “Ship” button in the UI.

share|improve this answer
    
Actually this is the point exactly that sent me thinking. If sub classes throw exceptions not defined in the parent then it must be a violation of LSP. –  Songo Jan 8 '13 at 21:32
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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