Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Access Modifiers are very restrictive #54

Closed
p-m-j opened this issue Mar 14, 2016 · 5 comments
Closed

Access Modifiers are very restrictive #54

p-m-j opened this issue Mar 14, 2016 · 5 comments

Comments

@p-m-j
Copy link
Contributor

p-m-j commented Mar 14, 2016

I'm subclassing StateMachine and was hoping I would be able to find a nice way to set up OnUnhandledTrigger so that if a state transition exists for given state and transition but guard conditions are not met that the exception message could contain the text from PermitIf->guardDescription.

Sadly everything I could possibly use is either internal or private.

Would there be any interest in making more use of protected access modifier?

@nblumhardt
Copy link
Collaborator

Hi @rustybox - thanks for the note. Instead of using subclassing for this, would it be better for us to just improve the default exception message in Stateless?

@p-m-j
Copy link
Contributor Author

p-m-j commented Mar 16, 2016

I think the best possible answer is both.


I'm sure there are lots of valid reasons people might want to get into the configuration properties etc, having them as protected keeps the public API the same but gives options to those who wish to extend.


The exception message at the moment when a guard condition is not met is a little confusing

No valid leaving transitions are permitted from state 'S' for trigger 'T'. 
Consider ignoring the trigger

It would be nice for this to mention that the trigger is valid but only in some situations.

I'll fork and throw something together

@nblumhardt
Copy link
Collaborator

👍 sounds good, thanks!

Not ruling out a change to access in the future, but while we can avoid it and handle more scenarios better (like this one) the status quo is probably preferable. StateMachine<,> isn't really designed for inheritance at all at the moment, so it'd need a bit of thought.

Cheers!

@p-m-j
Copy link
Contributor Author

p-m-j commented Mar 16, 2016

Interesting, it feels like it already works quite well

    public class PropertyStateMachine : StateMachine<PropertyState, PropertyTrigger>
    {
        public Property Property { get; private set; }

        public static Action<PropertyState> GetStatusHandler(Property property)
        {
            return status =>
            {
                Console.WriteLine("PropertyState was changed from {0} to {1}", property.CurrentPropertyState, status);
                property.Audit.Add(Tuple.Create(status, DateTime.Now));
            };
        }

        public PropertyStateMachine(Property property) 
            : base(() => property.CurrentPropertyState, GetStatusHandler(property))
        {
            this.Property = property;

            Configure(PropertyState.NotSet).Permit(PropertyTrigger.Brief, PropertyState.Briefed);

            Configure(PropertyState.Briefed)
                .PermitIf(PropertyTrigger.Submit, PropertyState.Submitted, () => Property.AnySubmitted())
                .Permit(PropertyTrigger.DeBrief, PropertyState.NotSet);

            Configure(PropertyState.Submitted)
                .PermitIf(PropertyTrigger.Audit, PropertyState.Audited, () => Property.AnyAudited())
                .PermitIf(PropertyTrigger.UnSubmit, PropertyState.Briefed, () => !Property.AnySubmitted());

            Configure(PropertyState.Audited)
                .Permit(PropertyTrigger.Activate, PropertyState.Activated)
                .PermitIf(PropertyTrigger.RemoveAudit, PropertyState.Submitted, () => !Property.AnyAudited());

            Configure(PropertyState.Activated)
                .Permit(PropertyTrigger.RemoveActivation, PropertyState.Audited);
        }
    }

    // usage
    var property = new Property();
    var machine = new PropertyStateMachine(property);
    machine.Fire(PropertyTrigger.Brief);

@nblumhardt
Copy link
Collaborator

Thanks for all of the detailed scenario information; I don't think we'll move forward on this one in the near future however. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants