Add PermitDynamicIf and CanHandleTrigger methods.

[keithballantyne]

Thanks for doing the PermitDynamic implementation.

Here are the PermitDynamicIf and the CanHandleTrigger additions.

Thanks,
Keith

[prasannavl]

Hi. I haven't had the time to update the documentation yet. But PermitDynamic already does the conditionals.

I didn't do PermitDynamicIf by design. The PermitDynamic, unlike stateless takes a Func<TState> that can return any state. So the idea is if you use PermitDynamic, all the conditional logic is taken care by the same func.

Second, CanHandleTrigger was removed from the previous designs, since it isn't reliable. Say for example, if your result was true - But by the time you use that result it may not be true (Since another thread could have changed the state). That's why I created the issue #13 for Try* methods around the same time as the release of v6 - It requires quite a bit of refactoring the self contained ExecutionHelper to accomplish this without code duplication, since Try* should never encounter any exceptions in its path.

However, for scenarios where you may just want to check, the CurrentPermittedTriggers are still available, which can be enumerated manually or with an extension method.

I wish you had quickly just created an issue first before introducing a large commit. I feel bad that I may not be able to merge your significant effort again. :(

I hope the above described design will addresses your scenarios. Please let me know if not, and I'd be happy to figure out the best path to a resolution.

[keithballantyne]

The guard predicates of the PermitIf etc. are there to determine if the trigger is valid (will be accepted) for the current state. I don't understand how you could use the NextStateRepresentationPredicate to determine if the trigger is valid for a current state.

I understand the concern of CanHandleTrigger not being completely reliable in the scenario you present, and I think the Try* methods are a good solution to the issue. However In many scenarios the CanHandleTrigger test is very desirable. The CurrentPermittedTriggers collection does not test the guard predicates and thus does not provide the complete answer if a trigger will be honoured by the state machine.

I am using (want to!) this state machine for a machine control application. I have state such as Idle, Executing, Paused etc. and triggers such as start, stop, pause, abort. The UI has buttons for Start, Stop, Pause, and Abort. The command handlers for the buttons call the CanHandleTrigger function for the appropriate trigger in the CanExecute method, and Fire the appropriate trigger in the Execute method. The guard predicates include checks for external conditions, such as doors closed, to prevent the user from trying to trigger a state transition if the machine is not able to honour it. The buttons are only enabled when the trigger is valid.

I hope this clarifies why I think the the PermitDynamicIf and CanHandleTrigger methods are needed.

[prasannavl]

I don't understand how you could use the NextStateRepresentationPredicate to determine if the trigger is valid for a current state.

This is an internal implementation detail that is never exposed outside. You really don't have to worry about how its handled for the purposes of doing PermitDynamicIf. (On that note, I also think the Predicate suffix has to be changed, which I will in the subsequent commits since it was based off an older logic).

From the usage perspective, adding conditions is easy:

config.ForState(State.Stop)
                .PermitDynamic(Trigger.Start, () => {
                     if (x == 0)
                         return State.Executing;
                     if (x == 1)
                         return State.Paused;
                 });

However, the CanHandleTrigger currently doesn't exist at all. But I see your point - I was a little skeptical if I had to add it, but I think it does have the benefits that you mentioned. I do see the value in bringing it back again.

So, with respect to how invalid triggers are handled are in the case of Dynamic triggers - Invalid states there currently uses a hook for throwing the exceptions. The hook can easily be intercepted, for these (since the default behavior is really just a static method, which can be manually called). So, a few changes should be able to take care of it.

And if you want PermitDynamic to not change at all, then you just need to create a State.Invalid. And regarding how this has to be handled is something I still have to give a little bit of thought to. You can read about one variant here in this discussion here: #7 (comment)

If you can adapt your commit for the same, that would be fantastic! Or I'll do these changes shortly.

[keithballantyne]

We will need CanHandleTrigger to be applicable to the PermitDynamic methods as well. That is why I added the PermitDynamicIf versions. The processing of the NextStateRepresentationPredicate in the CanHandleTrigger test would not be a desirable option (possible side effects). It would also make the API asymmetric, which is not desirable.

[prasannavl]

Hi, sorry for the delay. Been extremely busy the last couple of weeks. Let me look into this this weekend, and will work out if this is the best way to achieve this.

[prasannavl]

Hi, CanHandleTrigger methods have now been added. There are 3 overloads which allow to fine-tune the testing including or excluding the generic parameter variants.

Regarding PermitDynamicIf - While you do have a point with the side-effect, I think it better to add a guideline or best practices, which enforces not doing anything with side-effects in the PermitDynamic logic. A PermitDynamicIf would be redundant, since you can easily to it right inside Func itself. There are already quite a lot of Permit* methods, and I don't want to add anymore unless it provides real value. I hope you understand.

In the future, it would be great if you could open an issue, before modifying something that changes the core implementations. I understand that you probably, modified the code for your own operations, but I still feel bad that I was not able to merge your commit.

[prasannavl]

The latest commit improves on the existing implementation of PermitDynamic, but changes its signature. All PermitDynamic methods now requires a func that returns a struct DynamicState<TState> (which can be created easily from the DynamicState.Create helper methods.

CanHandleTrigger* also evaluates Dynamic scenarios seamlessly. Now, this can be considered feature complete. Let me know if this, along with the previous comment, resolves all your scenarios. You can try the v7-pre versions for the same.

[keithballantyne]

Thanks,

I have refactored one of my statemachines to use this pre-release version.
So far everything is working as expected.

I still think that having the PermitDynamicIf(...) would be desirable. In
my refactoring I have put the guard statement that would be in the
predicate as an if at the beginning of the DynamicState Func and return the
NoTransition when the trigger is not valid for the state. It works, but it
is not consistent with the other PermitIf() etc. methods.

I think that the symmetry and orthogonality of the API would be improved
with the PermitDynamicIf methods.

Just my thoughts.

Thank you for your efforts with this project. I do appreciate it.
Keith

On Wed, Jun 17, 2015 at 6:23 AM, Prasanna V. Loganathar <
[email protected]> wrote:

The latest commit improves on the existing implementation of
PermitDynamic, but changes its signature. All PermitDynamic methods now
requires a func that returns a struct DynamicState (which can be
created easily from the DynamicState.Create helper methods.

CanHandleTrigger* also evaluates Dynamic scenarios seamlessly. Now, this
can be considered feature complete. Let me know if this, along with the
previous comment, resolves all your scenarios. You can try the v7-pre
versions for the same.

—
Reply to this email directly or view it on GitHub
#20 (comment)
.

[prasannavl]

I think that the symmetry and orthogonality of the API would be improved
with the PermitDynamicIf methods.

This is true. In fact, I'm rather of the opinion that its best that PermitDynamic is asymmetric to the others. Here's why:

All the other Permit* methods deal with one state to one trigger mapping. So, its perfectly fine to have many of them. Think about the complications of having the PermitDynamicIf - Developers are now free to map each conditionals into different lambdas. And each of these would likely be mappings to many states. It would become extremely confusing to figure out which state is actually chosen. Things become very obscure, and unreadable.

Whereas, this way, all the conditional logic is clearly in one PermitDynamic lambda, which makes it very easy to understand the selection of states. Since, PermitDynamic at its very nature is different, I think an asymmetric API which clearly demarcates the API use would be more productive.