Adding actions to Permit() and PermitIf(), and adding an OnExitFrom()

[betty-crokker]

I've got some trial code in my fork: https://github.com/betty-crokker/stateless/tree/Permit

The best way to see the new APIs in action is in Tests/StateMachineFixture.cs.

There's a new (optional) triggerAction parameter on Permit(), looks like this:

sm.Configure(State.A)
    .Permit(Trigger.X, State.B, () => fired = true);

That's quite easy to use, and there's also an OnExitFrom() that gives the same functionality in a different way:

sm.Configure(State.A)
    .Permit(Trigger.X, State.B).OnExitFrom(Trigger.X, () => fired = true);

I also added the triggerAction parameter to OnPermitIf():

sm.Configure(State.A)
    .PermitIf(Trigger.X, State.B, () => true, () => value = 1)
    .PermitIf(Trigger.X, State.C, () => false, () => value = 2);

which I fear is a little hard to read, possible confusion as to which function is the guard and which is the action. But OnExitFrom() doesn't help in this situation:

// Both trigger functions get called, regardless of the guard function
sm.Configure(State.A)
    .PermitIf(Trigger.X, State.B, () => true).OnExitFrom(Trigger.X, () => value = 1)
    .PermitIf(Trigger.X, State.C, () => false).OnExitFrom(Trigger.X,  () => value = 2);

Having an "OnExitFromIf" sounds really awkward.

If folks think this looks good I'll create the async versions ...

[betty-crokker]

Per @nblumhardt 's suggestion, renamed OnExitFrom() to OnExitBy()

[jsobell]

I suppose the question is whether this functionality belongs in the definition of the guards or at a central point. Your solution addresses two issues; it allows a function to be specified, and it allows you to specify the data to pass to that function. If I were using this I'd probably end up with every PermitIf calling handleTransition(n) where n was a constant for each expression. Since n is simply an identifier for the appropriate transition, does it make more sense to have an ID with each transition, and allow an engine action passing that in as a parameter? Your way is potentially less wasteful, as you can add or remove the 'event action' from each transition, but it's also far more verbose. Also, if there are any other features required around transitions, we have the same issue again, in that we have no way to identify a transition. For example, we can currently query the trigger conditions, but not differentiate between two triggers with different guard conditions. Perhaps it makes more sense to allow an optional Id or 'tag object on each transition definition, and pass that to appropriate handlers.

sm.Configure(State.A)
    .PermitIf(Trigger.X, State.B, () => true, 1)
    .PermitIf(Trigger.X, State.C, () => false, 2);
    .Permit(Trigger.X, State.A, -1)
    .PermitReentry(Trigger.Loopback, 4);

Now OnTransitionedEvent can pass a parameter sequence of 'source, destination, trigger, tag'.

[betty-crokker]

I see a couple of problems with adding an "id" or "tag" object to each transition. 1) It's less flexible than the method I originally proposed 2) The author ends up with a big switch statement to decide what to do about the id value. The upside of the id/tag proposal, as you pointed out, is somewhat less code.

So we have one vote "for" and one vote "against".

@nblumhardt @HenningNT want to weigh in?

[jsobell]

Unfortunately you can't generalise it like that. A formula based action, such as logging, will only require a single statement in an Action based solution, so we don't know if a switch statement will be needed or not. The key thing here is that this mode of operation already exists in the project. The only thing missing is the ability to know which transition the trigger belonged to.

Plus, as I said, there are many other situations where the transition needs to be known and individually identifiable, such as highlighting the path a change took when debugging. We also do this, as every state change is recorded and the user can visualise the path as an 'audit' feature. There's no way to do that at present, and adding actions to the statements won't help there either.

If the 'tag' was an object the user could provide it as an action, meaning your OnTransitionedEvent could be handled like this:

        phoneCall.OnTransitioned(OnTransitionAction);
:
:
        private static void OnTransitionAction(StateMachine<State, Trigger>.Transition obj, object tag)
        {
            (tag as Action)?.Invoke();
        }

This provides identical features to your suggestion, but with the option of allowing any type of handling, including (for example) tuples with an ID and an Action, just an integer, or just a name. If 'tag' was implemented as a type on the public partial class StateMachine<TState, TTrigger, TTag> it would be even more 'correct' from a typesafe viewpoint, and if the parameter were optional we'd also have backwards compatibility.

Thoughts?

[HenningNT]

I would prefer the first option, as it is generic. I would use it like this, since I'm not comfortable with lambdas yet ;-)

sm.Configure(State.A)
    .Permit(Trigger.X, State.B, StateBOnTriggerX);

With regards to PermitIf, adding an action is strictly not necessary, as the guard can double as the action. But I think it should be added, so that we can expect the same configuration options.

sm.Configure(State.A)
    .PermitIf(Trigger.X, State.B, StateBEvaluateTriggerX, StateBOnTriggerXGoesToB)
    .PermitIf(Trigger.X, State.C, StateBEvaluateTriggerX, StateBOnTriggerXGoesToC);

I would prefer to NOT add a OnExitFrom/OnExitBy, as it then requires two configuration step to set up a trigger. Adding an action as a tag or object, and then relying on the OnTransitioned event to execute the action seems like a adding extra work to accomplish the same thing.

[jsobell]

A guard can't double as an action. A guard is an evaluation expression, and should not modify any data. What you're suggesting is that we don't need the existing OnTransitioned feature because we could do the same thing by adding a custom StateBOnTriggerXGoesToB defined call to every defined transition option? Plus we still don't have any way of identifying the transition for display, logging, or debugging purposes.

[HenningNT]

No, that's not what I was saying. Regarding adding an action to PermitIf, I said "But I think it should be added, so that we can expect the same configuration options." So I really think it's a useful addition, and I hope this goes trough.

If one needs display, logging, or debugging one can use the OnTransitioned event.

[jsobell]

If one needs display, logging, or debugging one can use the OnTransitioned event.

Sorry, I must be being a bit dense. Do you mean you can do this if you make the changes I suggested, or as-is?

[HenningNT]

Well, if it is for tracing the transition and triggers, then the OnTransition event can be used as the framework is at this stage. I use it in one application to let other objects know that something has happened, and I also write this to a log file.

[jsobell]

I think you may be missing my point. The OnTransition doesn't tell you which transition was followed, simply that one of the defined transitions from A to B was followed, with T being the trigger that caused it. In a situation where you have guard conditions you can't detect which transition it was.

sm.Configure(State.A)
    .PermitIf(Trigger.X, State.B, () => feespaid >= 200)
    .PermitIf(Trigger.X, State.B, () => feespaid < 200);

There are two possible transition paths between A and B, both triggered by event X. When the OnTransition fires how do I know which it was? This isn't an academic question, this is a common requirement for state machine based workflows. On a DOT graph generated from Stateless it will show both paths, because they are different transition rules, hence you must have a way of distinguishing between them.

[betty-crokker]

As Tech Sergeant Chen once said, ""I just had this really interesting idea."

The problem that I see, is that as we add more & more arguments to Permit(), it gets harder to tell what each argument is. It's already showing up in the code I wrote:

.PermitIf(Trigger.X, State.C, AFunction, AnotherFunction);

Which function is which? One is probably a guard, and the other an action ...

So what if we made it so Permit() took a variable number of possible named objects:

.Permit(Trigger.X, State.C, If(AFunction), Action(AnotherFunction))

then @jsobell could do things his way:

.Permit(Trigger.X, State.C, If(AFunction), Tag(3))

That solves both the problem of argument proliferation, and gives @jsobell the tag he wants.

Thoughts?

[jsobell]

This is an alternative syntax for setting properties, and we might as well implement it in fluent syntax, as that's more readable and flavour of the year :) To use your example: .Permit(Trigger.X, State.C).If(e => AFunction(e)).Tag(3).Description('Only if AFunction is true...')

Another option is to use a configuration syntax: .Permit(Trigger.X, State.C, cfg => cfg.If(e => AFunction(e)).Tag(3).Description('Only if AFunction is true...'))

This has the advantage of being significantly simpler than an effective fluent syntax (which itself requires a state machine), and clearly delineates the "Permits", but both options require significant code changes.

@nblumhardt any suggestions?

[betty-crokker]

I would avoid the "significant code changes" since they would also imply loss of backward compatibility. I'm imagining adding the fluent or configuration syntax as a new form of Permit() while leaving all the existing Permit() functions intact. I don't think it would be a huge increase in code ...

[betty-crokker]

Here's another possible syntax, not too hard to implement:

public class TriggerConfiguration : StateConfiguration
{
    TransitioningTriggerBehaviour Trigger;

    internal TriggerConfiguration(StateConfiguration config, TransitioningTriggerBehaviour trigger)
        : base(config.Machine, config.Representation, config.Lookup)
    {
        Trigger = trigger;
    }

    public TriggerConfiguration If(Func<bool> guard, string guardDescription = null)
    {
        Trigger.Guard = new TransitionGuard(guard, guardDescription);
        return this;
    }

    public TriggerConfiguration Do(Action action, string actionDescription = null)
    {
        Trigger.SetAction(action, Reflection.InvocationInfo.Create(action, actionDescription));
        return this;
    }
}

and

partial class StateConfiguration
{
   public TriggerConfiguration Permit(TTrigger trigger, TState destinationState)
   {
       EnforceNotIdentityTransition(destinationState);
       TransitioningTriggerBehaviour ttb = new TransitioningTriggerBehaviour(trigger, destinationState, null);
       Representation.AddTriggerBehaviour(ttb);
       return new TriggerConfiguration(this, ttb);
   }
}

which lets you easily say

sm.Configure(State.A)
    .Permit(Trigger.X, State.B).If(() => true).Do(() => value = 1)
    .Permit(Trigger.X, State.C).If(() => false).Do(() => value = 2);

[HenningNT]

@jsobell Your example:

sm.Configure(State.A)
    .PermitIf(Trigger.X, State.B, () => feespaid >= 200)
    .PermitIf(Trigger.X, State.B, () => feespaid < 200);

Will throw an exception as there are multiple exit transitions. Also it is functionally equivalent to

sm.Configure(State.A)
    .Permit(Trigger.X, State.B);

So it could be reduced to

sm.Configure(State.A)
    .Permit(Trigger.X, State.B).Do(HandleFeesPaid)

But I digress... Adding actions to all Permit variations let's @jsobell store extended state variables, and adds functionality that exists in other state machine frameworks (I'm also using the QP framework). I think that the fluent syntax is a little better, as a bonus I believe we can condense all Permit variations into a single Permit (including InternalAction):

sm.Configure(State.A)
    .Permit(Trigger.Z, State.B).Do(SomeAction)
    .Permit(Trigger.Y, State.C).If(SomeCondition, "Description").Do(SomeAction) // Or maybe...
    .Permit(Trigger.X, State.C).If(SomeCondition).Describe("Some Guard description").Do(SomeAction)
    .Permit(Trigger.W).Dynamic(StateSelector) // The action could be defined in StateSelector?
    .Permit(Trigger.V).Dynamic(StateSelector).If(SomeCondition)
    .Permit(Trigger.U).Internal().Do(SomeAction)
    .Permit(Trigger.T).Internal().If(SomeCondition).Do(SomeAction)
    .Permit(Trigger.S).Self().Do(SomeAction)
    .Permit(Trigger.R).Self().If(SomeCondition).Do(SomeAction)

The Ignore could just create an internal transaction without any associated action. I have mixed feelings about PermitDynamic. It is essentially a collection of PermitIfs. It reduces the line count, but hides some the state machine configuration...

[jsobell]

Yes, PermitIf guard clauses must be mutually exclusive. This is always the case with guarded transitions in a state-machine workflow. However, the code above won't case an exception as the guard conditions are mutually exclusive. Try it on the 'switch' example:

 onOffSwitch.Configure(off).PermitIf(space, on, () => true);
 onOffSwitch.Configure(off).PermitIf(space, on, () => false);
 onOffSwitch.Configure(on).Permit(space, off);

What's missing from this is the trigger parameters to the guard condition (discussed in issue #155) but that's another matter to be addressed.

[betty-crokker]

See PR #173 "Add options to triggers"

[jsobell]

Chris, you might want to check the PR I sent to your fork before that's merged. It allows strong-typing of the Tag(), I also added a couple of tests for your .If() syntax

[HenningNT]

Instead of Permit I would call it Transition, and supply the destination state in a .To(destinationState):

sm.Configure(State.A)
    .Transition(Trigger.Z).To(State.B).Do(SomeAction)
    .Transition(Trigger.Y).To(State.C).If(SomeCondition, "Description").Do(SomeAction)
    .Transition(Trigger.W).Dynamic(StateSelector) // The action could be defined in StateSelector?
    .Transition(Trigger.U).Internal().Do(SomeAction)
    .Transition(Trigger.S).Self().Do(SomeAction)
    .Transition(Trigger.R).Self().If(SomeCondition).Do(SomeAction)

Maybe even slap on .Or() and .And() on the .If()?

[betty-crokker]

I like the syntax, I agree that Transition() is more readable than Permit(). Just to make sure we're on the same page, we're talking about a fork that might be considered after stateless 4.0 goes out? Or are you thinking of this as a permanently separate fork?

[HenningNT]

Maybe @nblumhardt want to reply on that one? I'm not sure what we should work on after the next major release.

[jsobell]

Using the term 'Transition' makes much more sense, because (as I mention in https://github.com/dotnet-state-machine/stateless/issues/143#issuecomment-306016690) there's currently no concept of a transition being an entity, and the current system seems to try to elevate triggers to transitions, which doesn't really make sense. Re the idea of adding .Or() and .And() to the .If(). This doesn't work, because any logic operation should be within the .If() statement (which is really a guard condition, so should it be .Guard()?), and any 'alternative' guards are actually other Transitions. There are some fundamental things that need reviewing if this is to be treated as a fully fledged state-machine solution. These things are not making it any more complex, and I think it's important we differentiate between a simple solution and a naive one.

[nblumhardt]

Nice syntax, @HenningNT.

I'm a bit unsure TBH. I think there's a lot that can be improved, but getting the word out on breaking changes, an upgrade path and so-on takes additional time, which I'm currently desperately short on :-)

Post-4.0, do you think you'll have time to drive it, @HenningNT, @betty-crokker, @dotnet-state-machine/reviewers ? I mean that as a genuine question - I would be happy to open up the floor, so to speak, and would be supportive of a revised 5.0 if everyone thinks there is enough gain to be had.

At present, because my own availability is very patchy, I'm hesitant to make grand plans, lest we bite off more than we can collectively chew. If other contributors like yourselves have the time to shape, guide and document a more ambitious release that would make me super happy, though! Thoughts?

[HenningNT]

Before starting on such a big project I think we should review how stuff is handled internally. I think there are a few things we should look at, for instance the StateRepresentation class contains lists for entry and exit action, but it should only be a single action of each type. After that I could probably take on a bigger responsibility.

EDIT: I think I understand why it's Lists, one can use Any() instead of checking for null...