Is my design still a state design pattern, or some abomination?

I was trying to review state design pattern, and tried to create flexible implementation of it so it could reuse it in future.

So I came out with this:

public interface FsmState,INPUT> {
    Result action(INPUT input);
}

and this:

public class Result> {

    public boolean isCompleted() {
        return isCompleted;
    }

    public S getNextState() {
        return nextState;
    }

    public Result(boolean isCompleted, S state) {
        super();
        this.isCompleted = isCompleted;
        this.nextState = state;
    }

    private final boolean isCompleted;

    private final S nextState;
}

and for now this is way how you can use it:

```
public class Fsm {
static enum SomeEnum {
A, B, C
}

static class aState implements FsmState {

@Override
public Result action(String input) {

if (input.equals("B")) {
return new Result(false, SomeEnum.B);
}
return new Result(false, SomeEnum.A);
}
}

static class bState implements FsmState {

@Override
public Result action(String input) {
if (input.equals("B")) {
return new Result(false, SomeEnum.B);
}
if (input.equals("C")) {
return new Result(false, SomeEnum.C);
}
throw new RuntimeException("Invalit input");
}
}

static class cState implements FsmState {

@Override
public Result action(String input) {
if (input.equals("A")) {
return new Result(true, SomeEnum.C);
}
return new Result(false, SomeEnum.C);

}
}

private Map> statesMap = new HashMap>();

public Fsm() {
statesMap.put(SomeEnum.A, new aState());
statesMap.put(SomeEnum.B, new bState());
statesMap.put(SomeEnum.C, new cState());
}

boolean run(String input) {
FsmState action = statesMap.get(Some

First of all all design patterns are well defined. Every deviation of it will distort your implementation and negate advantages into disadvantages. That is because all known design patterns in general were NOT MADE UP. They were IDENTIFIED to be the best model structures to represent some of the real world structures.

The state pattern is a behavioural pattern. Behaviour is not existent for types only objects can behaves. The state pattern consists of a holder object and an association to a state object that is known abstract. Every single state decides on his own what is the following state dependent on the state of the holder object. So far your implementation as well.

But currently you are only hopping from one state to the other without doing something. You are currently not manipulating any object local variables of the holder object what will be the target of the state pattern. The manipulation is done in the for-loop. The holder object provides the information to the action() method of each state object. So here you distribute responsibilities between the holder object and the state object. My recommendation is to let the state objects decide on their own, when to get the next character. Because the "input" variable is not in object scope you should put it there. Doing this in the run() method will lead to an error prone and not thread enabled solution. So here I would pass the "input" through the constructor and make it object local available. No Parameter for the run() method anymore. For each Thread instantiate a fresh Fsm object with new input.

Furthermore you have at least one other "state" representation: isCompleted(). This should be represented as a state as well and not with a separate construct in the result.

What your implementation explicitly defines is the reuse of the state objects (caching).

In a special case you will not face any problem. That is if only behaviour is encapsulated in the state object no "state". So the state objects must not have object local variables to be modified (and of course no class variables too). BTW this is another pattern called flyweight. Its primary feature is immutability. So in your implementation you should ensure that your state objects are immutable.

If you have state within your state objects then you have to consider an initialization mechanism for your stateful state objects or always create new state objects. If you don't do that you introduce distributed states because the execution of the next state does not only depend on the state of the holder object. It also depends on the state of the following state object that the predecessor is not aware of. But every state object must be aware of ANY state to decide to what next state is to delegate. So if you have stateful state objects you must newly instantiate or initialize them or make them a flyweight. As your state objects hold no state you currently have no problem.

But one other thing is allowed. You may parametrize a successor with a result of a predecessor. I am not sure about any use cases but may pass information around because the current state object can decide on its own what to do. But in your example your are not passing anything around.

OK, what about the enum? Under the condition your state objects remain flyweights the enum is an unnecessary code fragment. You can remove the enum and introduce constants for each state object. If you have the intention to have stateful state objects I recommend to avoid reusing state objects. Of course you can initialize them but this is error prone. In this case it's also not necessary to have an enum.

Currently you are returning a complex object that consists of a boolean and the enum (the TYPE!!!) of the new state. I recommend to return nothing (void). The boolean as already stated can be seen as a separate state construct beside the existing state pattern which should be represented as a separate state object. The enum is obsolete when you use state object constants or newly instantiate state objects. The return value is obsolete if you provide a setState() method on the holder object. I didn't recognize until now that you haven't got a currentState variable held by the holder object. That has to be introduced.

Finally to make your Fsm (whatever this will be) generic you should provide a set of start states (public Set getStartStates()) on the holder object. They will be accessible to the client object that uses an instance of Fsm. The client may pass one start state into the constructor.

As well you should provide a set of end states. Your loop should then only look like this:

while (!getEndStates().contains(currentState)){ currentState.action() }.

One thing I won't go into: To make Fsm generic I assumed to have flyweight state objects. If you do not have flyweights you will go deep into meta modelling. Then you would introduce a type level. BTW your enum was somehow something like this b

StackExchange Code Review Q#114283, answer score: 3