Design Patterns – Does This State Pattern Implementation Make Sense?

design-patterns

I have the task to implement the state pattern in one of my classes. The assignment referenced a suggested implementation, and upon reviewing that implementation I was confused since I believed it to be nonsensical for most applications and frankly simply bad from a design point of view, since it completely dismisses some very basic design principles of software code.

This is the class diagram of the suggested implementation:

enter image description here

I wonder: What is the use of this implementation? It (imo) clearly violates the two main design principles of software engineering: Make things easily maintainable, and easily extensible.

In my eyes, this implementation has zero easy extensibility(you have to change the source of at least two classes, for example, to implement a new state that you forgot about when first implementing it). The coupling between this code and client code seems to be huge. If you remove anything in this pattern, all client code will break. If you want to add anything, you have to change all client code in order to adapt, for example, a new state, because new states can only be entered via the enter() method of the concrete state, or by directing injecting them into the Controller using the changeState() method.

As a bonus, you can only have one garage door in the entire world, since the states are practically singletons. The enter() method in the concrete states was given as this:

public static State enter(Controller c) {
if(m_instance == null) m_instance = new <<concreteStateName>>(c);
return m_instance;
}

Or you can have multiple garage doors, which all open simultaneously if you open one.

Why would you put all methods in the abstract State class? Where's the polymorpy? Why do concrete states have either empty or exception throwing versions of methods that have nothing to do with their state (for example, the resulting interface of Opened will have a "lock()" method, even though you can't directly lock the garage door when it's open. You have to close it first). Why not choose a single method and implement it differently in the concrete States, making the thing easily extensible again?

It is also (again, as far as I can see) not really maintainable without a lot of troubles. What if you, after a couple of months of using the garage, decide that one state is in fact not only obsolete but wrong? You're going to have to take it out, and the according method in State() with it, breaking the entire code base that uses this implementation. This is a garage door with four states. I wouldn't want to work on that thing if it had any more states.

The situation gets more confusing when I look around the internet: The pattern actually seems to be implemented exactly this way a lot of times (for example it is implemented like this way in our national Wikipedia (German)).

The actual question: Am I missing anything or is this simply bad? Since this is so widespread, I'm thinking that I might be missing something obvious here.

Best Answer

an easier way would be to have the state methods return the state after the operation, and combine all methods in the abstract State into a generic procesInput(data,controller): state

this way states don't need to know what controller they are linked to (allowing them to be true singletons)

and now each state then only needs to know what next states are possible according to what input

Related Solutions

State Pattern and Liskov Substitution Principle

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.

State Design Pattern – Best Implementation for C#

I agree with Hans that observer can make the maintenance more nightmare. What I can suggest is separating the concern (delegating the responsibility) of opening playing screen into another class, to follow Single Responsibility Principle.

Currently, your title class has at least 2 responsibility, that is:

rendering the title page and menus
add screen related to menu selected
(handle input such as arrow keys, etc)?

The add screen responsibility can be delegated to another class. Let's say it TitleAddScreenHandler.

public class TitleAddScreenHandler{
    public TitleAddScreenHandler(ScreenManager screenManager){
        // property assignments
    }
    private ScreenManager screenManager;

    public void AddScreen(InputState input){
        if (input.MenuSelect) {
            screenManager.AddScreen (new PlayingScreen ());
        }
    }
}

Then in your TitleScreen, you can has one instance of TitleAddScreenHandler or injected in constructor. It will limit the dependency only to one class.

Best Answer

Related Solutions

State Pattern and Liskov Substitution Principle

State Design Pattern – Best Implementation for C#

Related Topic