3
\$\begingroup\$

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<S extends Enum<?>,INPUT> {
    Result<S> action(INPUT input);
}

and this:

public class Result<S extends Enum<?>> {

    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<SomeEnum, String> {

        @Override
        public Result<SomeEnum> action(String input) {

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

    static class bState implements FsmState<SomeEnum, String> {

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

    static class cState implements FsmState<SomeEnum, String> {

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

        }
    }

    private Map<SomeEnum, FsmState<SomeEnum, String>> statesMap = new HashMap<Fsm.SomeEnum, FsmState<SomeEnum, String>>();

    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<SomeEnum, String> action = statesMap.get(SomeEnum.A);
        for (char ch : input.toCharArray()) {
             ;
            Result<SomeEnum> state = action.action("" + ch);
            if (state.isCompleted()) {
                return true;
            }
            action = statesMap.get(state.getNextState());

        }
        return false;

    }

    public static void main(String[] args) {
        System.out.println(new Fsm().run("AAABBCA"));
    }
}

For each Enum element you create implementation of FsmState interface, where based on input you do transition to next state.

It is still a bit messy code, and I'm fully aware of that. But I was wondering if it can be called a state pattern?

\$\endgroup\$
  • 1
    \$\begingroup\$ Could you describe what model FsmState is for? I don't understand the context of your case and why the state transition takes place in the Result class. In my opinion, you should simplify it first by eliminating Generic from your design. It just makes the problem too general and difficult to get it. \$\endgroup\$ – Hung Tran Dec 17 '15 at 15:35
  • \$\begingroup\$ @HungTran my main point of this, is to create generic code which could be reused in different FSM's., hence i can't remove generic. My primary concept was we take any enum, and consider it as list of states. For each state we create implementation of FsmState where based on current input work out next state, and return it wrapped in result. FSM is taking result checks is action is completed, otherwise it change state and carries further. \$\endgroup\$ – user902383 Dec 17 '15 at 15:53
  • \$\begingroup\$ As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review. \$\endgroup\$ – Jamal Dec 18 '15 at 1:45
  • \$\begingroup\$ You can call it state pattern since you encapsulate the behavior of a state in an object. That's the important part to identify the pattern, the overall structure of objects is not that important because design patterns aren't rigid plans. \$\endgroup\$ – zapl Dec 19 '15 at 2:41
3
\$\begingroup\$

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 but under the assumption of flyweights unnecessary and under the assumption of stateful state objects insufficient.

To answer your question finally: It's not the standard way to implement the state pattern and it has its pitfalls because of that.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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