3

Assuming I have the following code

IDLE = 0
STARTED = 1
STOPPED = 2
ERRORED = 3
# additional states as needed

class StateMachine:
    def __init__(self)
        self.state = IDLE

    def start(self):
        self.state = STARTED
        # do something

    def stop(self):
        self.state = STOPPED
        # do something

    def reset(self):
        self.state = IDLE
        # do something

Our current interface allows a client to change the state of an instance by stating the desired target state, at which point we run certain validation checks and then the appropriate method. I would ideally like to keep a dictionary mapping of the desired target state to the correct method to avoid massive and pointless if-statement blocks. i.e.

if target_state = STARTED:
    instance.start()
elif target_state = STOPPED:
    instance.stop()
...

But I'm uncertain as to whether or not the following solution is considered good practice or not (it feels a bit whacky calling methods from the class using an instance as arg).

state_mapping = {
    IDLE: StateMachine.reset,
    STARTED: StateMachine.start,
    ....
}

And then calling using:

action = state_mapping[target_state]
action(instance)
....

Any thoughts?

Andy
  • 33
  • 4
  • Have you tried using a `lambda` expression? – Alon Alexander Jun 20 '17 at 13:50
  • I think the solution is OK. Perhaps you should consider bench marking to see what is more time efficient. If efficiency isn't a concern, readability should be. I personally think the `if` `elif` `elif` `else` solution is more readable - even if it is a little boring. – ChickenFeet Jun 20 '17 at 14:00
  • there is another approach, which is to have an overall StateMachine class, then a subclass for each state, like StateMachineStopped, StateMachineStarted. you switch back and forth with a `self.__class__ = StateMachineStopped` kinda approach. See https://stackoverflow.com/questions/13280680/how-dangerous-is-setting-self-class-to-something-else/24463654#24463654 for example. – JL Peyret Jun 20 '17 at 17:36

2 Answers2

2

Not so whacky.

However, the only thing one has to bear in mind is that action is an unbound method, which may not be very obvious at a first glance of the method call; except I know first-hand how that dictionary is defined.

I think a more readable alternative is to call the method from the instance:

state_mapping = {
    IDLE: "reset",
    STARTED: "start",
    ....
}

action = state_mapping[target_state]
getattr(instance, action)()

This will equally improve readability in the case when the method takes more than one argument.

Moses Koledoye
  • 77,341
  • 8
  • 133
  • 139
  • Thanks for the feedback, I had considered doing this as well but was a bit torn between the two approaches. Now that you mentioned readability it makes sense do use `getattr` instead. – Andy Jun 20 '17 at 14:02
1

One other alternative.

As your class is called "StateMachine", perhaps it should have a method to execute the state change?

In which case, you can use bound methods in your map

class StateMachine:
    ...

    def ChangeState(self, target):
        state_mapping = { IDLE: self.reset, STARTED: self.start, ... }
        state_mapping[target]()

You may want to deal with invalid target states, or just let it raise a KEY_ERROR exception.

kdopen
  • 8,032
  • 7
  • 44
  • 52