Sponsored By Aspose - File Format APIs for .NET

Aspose are the market leader of .NET APIs for file business formats – natively work with DOCX, XLSX, PPT, PDF, MSG, MPP, images formats and many more!

State Pattern Misuse

Just a quick run by posting from some discussions yesterday. I see many people using the State Pattern in their domains. More often than not though they are making a mistake that can be seen by analyzing the OO aspects of their code. Let’s go through a quick example of a Loan Application.

 

When its “In process” you can edit data on it or submit for approval.

When its “awaiting approval” you can no longer edit details data but you can approve or deny the application.

When its “completed” it has become immutable and can no longer be edited but may have some getters on it for viewing of historical data

 

I went through and made a quick little example of this

public interface ILoanApplicationState
{
    void Approve();
    void Deny();
    InterestRate GetApprovedInterestRate();
    InterestRate CalculateLikelyInterestRate();
    void SubmitForApproval();
    void ChangeDetails(LoanDetails details);
}

public class LoanApplicationState : ILoanApplicationState
{
    private LoanApplication parent;
    public LoanApplicationState(LoanApplication Parent)
    {
        parent = Parent;
    }

    public virtual void Approve() {}
    public virtual void Deny(){}
    public virtual InterestRate GetApprovedInterestRate() {}
    public virtual InterestRate CalculateLikelyInterestRate() {}
    public virtual void SubmitForApproval(){}
    public virtual void ChangeDetails(LoanDetails details) {}

}

public class InProcessState : LoanApplicationState
{
    public InProcessState(LoanApplication Parent) : base(Parent) { }

    public override void Approve()
    {
        throw new InvalidOperationException("Cannot approve an inprocess application");
    }

    public override void Deny()
    {
        throw new InvalidOperationException("Cannot deny an inprocess application");
    }

    public override InterestRate GetApprovedInterestRate()
    {
        throw new InvalidOperationException("No approved interest rate exists on an in process application");
    }

    public override InterestRate CalculateLikelyInterestRate()
    {
        return InterestRate.Default;
    }

    public override void SubmitForApproval()
    {
        //change parent state to submitted
    }

    public override void ChangeDetails(LoanDetails details)
    {
        //change parent details
    }
}

public class AwaitingApprovalState : LoanApplicationState
{
    public InProcessState(LoanApplication Parent) : base(Parent) { }

    public override void Approve()
    {
        //change parent state to approved and issue some behavior
    }

    public override void Deny()
    {
        //change parent state to denied and issue some behavior
    }

    public override InterestRate GetApprovedInterestRate()
    {
        throw new InvalidOperationException("No approved interest rate exists on an awaiting approval application");
    }

    public override InterestRate CalculateLikelyInterestRate()
    {
        return InterestRate.Default;
    }

    public override void SubmitForApproval()
    {
        throw new InvalidOperationException("Already awaiting approval");
    }

    public override void ChangeDetails(LoanDetails details)
    {
        throw new InvalidOperationException("No approved interest rate exists on an in process application");
    }
}

public class CompletedApplicationState : LoanApplicationState
{
    public InProcessState(LoanApplication Parent) : base(Parent) { }

    public override void Approve()
    {
        throw new InvalidOperationException("Application has already been completed");
    }

    public override void Deny()
    {
        throw new InvalidOperationException("Application has already been completed");
    }

    public override InterestRate GetApprovedInterestRate()
    {
        //return parents approved interest rate
    }

    public override InterestRate CalculateLikelyInterestRate()
    {
        //return parents approved interest rate
    }

    public override void SubmitForApproval()
    {
        throw new InvalidOperationException("Already completed");
    }

    public override void ChangeDetails(LoanDetails details)
    {
        throw new InvalidOperationException("Application is already completed");
    }
}

 

 

OK enough yucky code you guys get the idea (the state pattern is also very verbose). The idea here is we plug in these state objects to our object and they handle the behavior changes from the original object.

My contention here is that you should not use the State Pattern like this. Where is the polymorphism? What if we think about the gross violation of LSP that we just created? This does however happen way too often.

The solution is that we really have three classes here InProcessApplication, AwaitingApprovalApplication, and CompletedApplication.

public class InProcessApplication {

    InProcessApplication SubmitForApproval(ILoanProcessingService processor) { …  }

}

this will force us to end up with smaller and easier to understand classes… and I mean hey, its not like polymorphism would have worked before anyways. It also makes our Ubiquitous Language more concise.

There is a particular code smell here that will help us distinguish whether we want the State Pattern or separate classes. Do some of the data/behavior only make sense in certain states? If so use three separate classes. Going along with this, if we find “throws” in our state implementors we should realize through LSP that we are doing something bad.

This entry was posted in Uncategorized. Bookmark the permalink. Follow any comments here with the RSS feed for this post.

20 Responses to State Pattern Misuse

  1. Jim says:

    Hi Greg,
    Can you please be more elaborate and post a working sample ?

  2. Jason says:

    Apparently what I was describing in my last post comes closer to a PetriNet than a Finite State Machine is better modeled here

    http://aabs.wordpress.com/2010/03/10/programming-with-petri-nets/

  3. Jason says:

    I’ve found that the traditional GoF pattern is limited and almost dangerous when used directly. I’ve implemented directly as you did above, and not been able to make heads or tails of my own code when I came back to it later. That’s very bad.

    If you enhance it a bit by returning an IEnumerable
    from your state operations, you get a lot of benefits:

    – Encapsulate the transitions
    – Increase readability: named transitions are easier to understand than the in-state code. Examples: CalculateInterestRate, Approve, etc
    – Can reuse and compose the transitions
    – Never have to throw unless you really want to .. and can still return error messages as needed. Think InvalidTransition(“ErrorMessage”)
    – Clearly separate commands and queries: think CommandTransition, QueryTransition
    – can add a command stack and undo functionality quite easily

    I’ve found this to be very successful in maintaining states even across distributed, partially-connected systems. I just send the transitions around and if some fail, I roll back. Purely deterministic.

  4. DM says:

    Thanks for the article Greg. I would also like to see the full solution if you get a chance to post it.

  5. Remi Despres-Smyth says:

    Greg,
    Out of curiosity, can you give a scenario or two where you *would* use the state pattern then? Maybe it’s just the type of problem I’ve had to work on, but I can’t see many cases where you wouldn’t need to throw in some states.

    Isn’t that the whole point of the pattern? Use state to help define the valid transitions between states – and if the invalid transitions don’t throw, what do they do?

    Regards,
    Remi.

  6. Tim says:

    “its not like polymorphism would have worked before __anyway__.”

    There is no such word as ‘anyways’.

  7. Sebastian says:

    Hey Greg,

    that’s exactly what I’ve been asked the other day for a job interview. I was asked how to implement a machine of throwing out a chewing gum each time I put a coin in. The traditional response was to implement the State pattern exactly as you mention it (WithCoin) (WithouCoin) and you are right, there are behaviour that doesn’t make sense at all for some state impl.
    What does the method “throwOutChewingGum()” means in a “WithoutCoin” State? Obviously this perfect suites for throwing a runtime “NotImplementedYetException” other than business exception.
    Well, in conclusion I gave roles to actors as ThrowOutMachine.throwOut(ThrowOutMachineContent, Money) where there’re State validations in the “ThrowOutMachine < -> ThrowOutMachineContent” collaboration that replaces the State pattern purpose.
    So I ask myself given your post, is State pattern really a good solution? Should be first look for a collaboration pattern such as Container – Content
    Yeah, ppl always tend to open the GoF book… I don’t know why, there’s always a better solution using tradicional OO.
    Cheers,
    Sebastian.

  8. Calum says:

    hi,

    Great article.

    Could you post the full code for the solution? I would like to compare the bad code with the good code as I understand the theory but would like to see how it is done practically.

    Thanks,

    Calum

  9. Sebastian says:

    Hey Greg,

    that’s exactly what I’ve been asked the other day for a job interview. I was asked how to implement a machine of throwing out a chewing gum each time I put a coin in. The traditional response was to implement the State pattern exactly as you mention it (WithCoin) (WithouCoin) and you are right, there are behaviour that doesn’t make sense at all for some state impl.
    What does the method “throwOutChewingGum()” means in a “WithoutCoin” State? Obviously this perfect suites for throwing a runtime “NotImplementedYetException” other than business exception.
    Well, in conclusion I gave roles to actors as ThrowOutMachine.throwOut(ThrowOutMachineContent, Money) where there’re State validations in the “ThrowOutMachine < -> ThrowOutMachineContent” collaboration that replaces the State pattern purpose.
    So I ask myself given your post, is State pattern really a good solution? Should be first look for a collaboration pattern such as Container – Content
    Yeah, ppl always tend to open the GoF book… I don’t know why, there’s always a better solution using tradicional OO.
    Cheers,
    Sebastian.

  10. Nick says:

    So the Ubiquitous Language now speaks of 3 different Application classes as if they are separate? Conceptually, it’s always a single Application, with a unique Id, no? I think it might be clearer to think of it as a single stateful object.

    I agree State is wrongly applied here because in this case, all the implementations that just throw an exception aren’t really valid operations, they should probably never be called.

    In this case, I might rather just keep it to one class and do a check at the top of certain methods, throwing an exception if the method isn’t allowed.

    That way, you could keep it to
    1. retrieve the application by id
    2. perform the operation
    3. save the changes

    If the UI was implemented correctly, those exception throwing operations should never even be called, right?

  11. Balaji says:

    Canyou finish the example so that we can get the full taste of what you are saying

  12. Phil B says:

    In the past I’ve handled this by adding properties such as CanApprove, CanDeny, etc. It changes the client usage pattern so that they must check if an action can be done before attempting to execute it. This takes care of the LSP violation because the client code should expect an exception when it calls a method whose corresponding Can* property is false. I have always gotten a bit of a code smell from this solution though. You make some good points here.

  13. Peter says:

    I’m glad you included the code in all its gory LSP-violating detail. But I’m not sure I see what you’re proposing as the alternative. Are the three classes derived from a common parent? If not, how are they consumed? What about common data for the application? Do you see an accompanying information holder for that stuff, e.g., LoanApplicationData?

  14. Colin Jack says:

    “Do some of the data/behavior only make sense in certain states? If so use three separate classes. Going along with this, if we find “throws” in our state implementors we should realize through LSP that we are doing something bad. ”

    Definitely agree it can be useful, but I do think its easily overused. Where there are a lot of states I’ve had success using separate classes each of which has 1 or more states.

    On LSP, my view is its a LSP violation if a subclass breaks the contract by throwing an exception, if the exception is part of the contract of the base class then it isn’t.

  15. Paul Eaton says:

    Why not a LoanApplication base class in your final example?

  16. Andy Rose says:

    Greg, I think you have a copy/paste issue with the constructors in your second and third state classes. Possibly another issue with this kind of pattern ;- )

  17. Carsten says:

    Isn’t this about the “Tell, don’t ask” principle? If you have to call Approve() on something and get a response like “Already approved” it smells to me like asking the object for a state instead of telling the object to transition to another state.

    I hope I get a chance to implement (or at least see) this pattern on some code soon. I’ll keep your post in my mind until then. :-)

  18. St says:

    I think the state-pattern makes sense in order to implement state transition logic.

  19. Perhaps the three classes even belong to different contexts…

  20. Kasper Frank says:

    Wouldn’t that be…

    public class InProcessApplication {

    AwaitingApprovalApplication SubmitForApproval(ILoanProcessingService processor) { … }

    }

    ?

Leave a Reply

Your email address will not be published. Required fields are marked *

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <s> <strike> <strong>