Pass previous and current state to the callback

[y8]

Pretty much the same as #27.

[soveran]

Hello @y8, do you have a use case for this change? In principle I don't like adding that much code for this feature, so if the use case justifies the modification we will have to find a better way to solve it. Let me know what the use case is and let's see what we can do. Thanks!

(I closed #27 to move the discussion here)

[y8]

That's not that much of the code :)

When you have couple transitions to the same state with one event, for example:

1. Pending -> Queued, Pending -> Failed
2. Queued -> Started, Queued -> Failed,
3. Started -> Completed, Started -> Failed

In the case of fail event there no way to know what state was before event happened, because state transition occurs before event callback is called.

We can drop current_state from the callback arguments, since the current state is available from the MicroMachine instance, but if method is used as callback (fsm.on(:event, other.method(:on_event))), it might be not reachable from that scope.

[soveran]

That's not that much of the code :)

I the big scheme of things, I agree that it's not a lot, but for a library with 41 lines of code, 13 new lines is significant :-)

About the use case: are you running into this issue in a real world application?

[y8]

I can reduce number of lines, but that comes with the price of reducing readability, which is not good ;)

About the use case: are you running into this issue in a real world application?

Yeah. I need to catch the previous state when event happens.

[soveran]

I can reduce number of lines, but that comes with the price of reducing readability, which is not good ;)

Yes, I mentioned that as a proxy to the complexity involved. Ideally we wouldn't add any more complexity to the library unless it's extremely necessary.

Yeah. I need to catch the previous state when event happens.

What if you keep the previous value of the state? Can you show me how you are implementing the solution?

[halostatue]

Do we need the temporary variable?

[y8]

Yep.

To reduce the number of array allocations during the each invocations. But it just came to me, that take will allocate new array for result, so yeah, this is no use.

Alternative is to expand the case to explicitly match each number of arguments, in this case we can avoid array allocation, but at the cost of the additional branching.

[halostatue]

Yeah; I’d done exactly that for #27 (increased branching). You’re guaranteeing at least n+1 Array allocations where n is the number of callbacks. This is probably small, but depending on how the state machine is used…it may be important to ensure optimization here. And no, I don’t actually have a clue as to whether the branching or allocations are more expensive in performance; that’s what benchmarks are for.

[soveran]

What I'm thinking is if the feature could be solved by the application using this library by keeping the value of the previous state. The impression I have is that the use case is not very common, but of course I may be wrong! In any case, if the application side can solve it, maybe that's a good start and we can communicate that use case in the documentation.

[halostatue]

When I was looking at this, @soveran, is that one couldn’t necessarily count on it being solvable in the application. If you are injecting micromachine in a Sequel model, you have #changed? on the model; with an ActiveRecord model, you have a similar method. I injected micromachine in something that just does PORO and just wants micromachine to keep track of the problem.

That said, I think I was able to work around the problem…but it left me somewhat unsatisfied. Note that both #27 and this PR fix the problem I described in #26 in that the callback mechanism was broken for folks who upgraded.

[y8]

Okay, I've updated with new method notify that call callbacks and with a new previous_state accessor.

Why the previous_state in fsm itself?

1. Keeping the previous state in the "outer" class is harder and more have an hard to debug errors, because you need to keep an eye at all the transitions. Miss one transition and you are done.
2. From the SOLID point of view: moving the previous state tracking to the "outer" class, forces "outer" class to have new responsibility: tracking of all transitions of the state machine. So it's adds unnecessary complexity to the "outer" class. Moreover, you will be forced to carry this tracking logic to the other classes that need information about the direction of the transition.

About my case, it's pretty simple:

self.transitions = MicroMachine.new(state)

transitions.when :start,      pending:   :started
transitions.when :withdraw,   started:   :withdrawn
transitions.when :deposit,    withdrawn: :completed

transitions.when :failure,    started:   :failure,
                              withdrawn: :failure,
                              deposited: :failure

transitions.on(:any) do |event|
  transition_to event
end

def transition_to(event)
  send(:"on_#{event")
rescue MyApp::Exceptions
  transitions.trigger(:failure)
ensure
  self.state = transitions.state
  save!  
end

# ... def on_start, def on_withdraw and def on_deposit ...

def on_failure
  self.failed_transition = transitions.previous_state
end

Yeah, I can it like this:

def on_failure
    self.previous_state, self.state = self.state, transitions.state
end

But what if I'm not tracking all the events, but only two?

How common this issue is? 50% of open, and 8.33% of all pull requests are about this issue :B

[soveran]

Keeping the previous state in the "outer" class is harder and more have an hard to debug errors, because you need to keep an eye at all the transitions. Miss one transition and you are done.

I'm not sure about that, I'm providing below an example of how you can keep track of the previous state.

From the SOLID point of view: moving the previous state tracking to the "outer" class, forces "outer" class to have new responsibility: tracking of all transitions of the state machine. So it's adds unnecessary complexity to the "outer" class. Moreover, you will be forced to carry this tracking logic to the other classes that need information about the direction of the transition

It's true that the use case is valid, but it's also true that it can be solved already without having to modify the internals. Another use case, for example, could be to keep an array of all the past states. It is also a valid use case, but it can also be acomplished with the primitives provided, check this example:

machine = MicroMachine.new(:s1)

machine.when :start, :s1 => :s2
machine.when :reset, :s1 => :s1,
                     :s2 => :s1

curr = machine.state
prev = nil

machine.on(:any) {
  prev = curr
  curr = machine.state
}

machine.on(:any) { |event|
  printf("%s: %s -> %s\n", event, prev, curr)
}

machine.trigger(:reset)
machine.trigger(:start)
machine.trigger(:reset)

The output is:

reset: s1 -> s1
start: s1 -> s2
reset: s2 -> s1

A similar approach could be employed to keep track of the N past states (or all of them if you dare!), and the same primitives can be used for that. My point is that there are many use cases that could be pushed into this library, but whenever possible we should try to make use of what it already provides.

[halostatue]

I think that the biggest issue I see with the approach you’ve got, @soveran, is that it depends on knowledge that callbacks in micromachine are made in the order provided. One small change to the code (that seems innocuous) breaks the assumptions:

machine = MicroMachine.new(:s1)

machine.when :start, :s1 => :s2
machine.when :reset, :s1 => :s1,
                     :s2 => :s1

curr = machine.state
prev = nil

machine.on(:any) { |event|
  printf("%s: %s -> %s\n", event, prev, curr)
}

machine.on(:any) {
  prev = curr
  curr = machine.state
}

machine.trigger(:reset)
machine.trigger(:start)
machine.trigger(:reset)

This results in:

reset:  -> s1
start: s1 -> s1
reset: s1 -> s2

There are other conditions under which this would be unsafe; if I have multiple callbacks that all want to know what the real previous state was, but one of the early callbacks modifies it, then I can no longer trust the previous state variable in my object. Yes, this can be handled with careful coding, but it feels like this should be handle more automatically.

[halostatue]

The biggest change in this code is related to arity-counting callbacks, which I pointed out in #26 as being backwards incompatible—and that mattered for the 1.2 release. Not for the recent 2.0 release. In 1.1, it would have been possible to do machine.on(:any, &-> { … }) (or machine.on(:any, &method(:method_name)). In 1.2, this would have required a change to be machine.on(:any, &->(*) { … }). Version 2.0 requires the same form, but such a breaking change can be expected from v1 to v2.

This means that we could satisfy my need for consistency with a much smaller change:

diff --git i/lib/micromachine.rb w/lib/micromachine.rb
index bc09755..9367bd7 100644
--- i/lib/micromachine.rb
+++ w/lib/micromachine.rb
@@ -3,7 +3,7 @@ class MicroMachine
   InvalidState = Class.new(ArgumentError)

   attr_reader :transitions_for
-  attr_reader :state
+  attr_reader :state, :previous_state

   def initialize(initial_state)
     @state = initial_state
@@ -44,7 +44,7 @@ class MicroMachine
 private

   def change(event)
-    @state = transitions_for[event][@state]
+    @previous_state, @state = @state, transitions_for[event][@state]
     callbacks = @callbacks[@state] + @callbacks[:any]
     callbacks.each { |callback| callback.call(event) }
     true

(This will introduce a warning about @previous_state not being initialized; adding a third line change in initialize @previous_state = nil would fix that.) 

[y8]

I agree with @halostatue, there no need to pass state and previous_state to the block, if previous_state is available from the MicroMachine instance. See latest changes.

If @soveran is okay with this solutions, I'll rebase for a clean merge.

[soveran]

I like that approach better, maybe we can work on @halostatue's patch. I'm wondering if it clearer to have each attribute on its own line, initialize @previous_state to nil, and move the assignment of @state to @previous_state also on a separate line, as in this diff:

index bc09755..fa1ebc3 100644
--- a/lib/micromachine.rb
+++ b/lib/micromachine.rb
@@ -4,9 +4,11 @@ class MicroMachine

   attr_reader :transitions_for
   attr_reader :state
+  attr_reader :previous_state

   def initialize(initial_state)
     @state = initial_state
+    @previous_state = nil
     @transitions_for = Hash.new
     @callbacks = Hash.new { |hash, key| hash[key] = [] }
   end
@@ -44,6 +46,7 @@ class MicroMachine
 private

   def change(event)
+    @previous_state = @state
     @state = transitions_for[event][@state]
     callbacks = @callbacks[@state] + @callbacks[:any]
     callbacks.each { |callback| callback.call(event) }

Also, what do you guys think about the name? When I write something with previous and current state, I usually name them curr and prev, as there's a nice symmetry in that, as opposed to state and previous_state. But I guess the API should remain as state for the current value, and maybe prev is too cryptic? That's why it could be the case that state and previous_state are just the best we can do in terms of naming.

The final question regarding the behavior would be whether @previous_state should be initialized to nil, to initial_state, or maybe even raise an exception. I think nil can be a good sentinel value, but I'd like to know what you guys think.

[y8]

About the lines, yeah, rubocop is not fine with that. Fixed.

That's why it could be the case that state and previous_state are just the best we can do in terms of naming.

I'm not a fan of cryptic prev's and curr's, so state and previous_state is perfectly okay.

The final question regarding the behavior would be whether @previous_state should be initialized to nil, to initial_state, or maybe even raise an exception.

nil is fine as well. Exceptions are pricy, and you'll be required to check for exceptions each time you read the previous_state.

[halostatue]

I think that previous_state is the clearest, but I will bikeshed briefly on prior. It’s shorter and reasonably clear in its meaning (IMO) given that you are asking the state machine what its prior is. I think it’s better than prev for reasons previously stated.

[halostatue]

Otherwise, I think that this satisfies the occasional need for consistent access to prior state without expanding the API too much or the lines of code that have to be run in a tight loop.