2
\$\begingroup\$

I have a session class that needs to be setup based on different needs of the client code. Sometimes a new session needs to be created, or continued, or resumed (loaded from database after a user has saved their progress and came back).

Right now the client code has to ALWAYS execute Start(), Continue(), or Resume() after instantiating the WorkflowSession class or the instance is useless. Like this:

var workflowSession = new WorkflowSession(Session);
workflowSession.Continue(); // or .Continue() or .Resume()

Should I do a return this; in order to be able to chain?

var workflowSession = new WorkflowSession(Session).Continue();

Or make some type of factory or builder? If so, how?

/// <summary>
/// Manages workflow session persistence.
/// </summary>
public class WorkflowSession
{
    private int workflowPlanId;
    private int dataClipId;
    private HttpSessionStateBase session;
    private WorkflowRepository workflowRepository;
    private DataClipRepository dataClipRepository;
    private StateRepository stateRepository;

    public WorkflowSession(HttpSessionStateBase session)
    {
        this.session = session;
        workflowPlanId = (int)session[SessionKeys.WorkflowPlanId];
        dataClipId = (int)session[SessionKeys.DataClipId];

        dataClipRepository = new DataClipRepository();
        stateRepository = new StateRepository();
        workflowRepository = new WorkflowRepository();
    }

    public SchemaProvider Provider { get; set; }

    public SchemaRoot Root { get; set; }

    public DataClip Clip { get; set; }

    public WorkflowPlan Plan { get; set; }

    public WorkflowContext Context { get; set; }

    public void Continue()
    {
        Clip = dataClipRepository.GetById(dataClipId);
        Plan = workflowRepository.GetById(workflowPlanId);
        Context = ContextFactory.Current.Create(Clip, Plan);
    }

    public void Start(int workflowPlanId, int schemaId)
    {
        Provider = ProviderCache.GetProvider();
        Root = Provider.GetRoot(schemaId);
        Clip = DataClip.Create(Root, Provider);
        Plan = workflowRepository.GetById(workflowPlanId);
        Context = ContextFactory.Current.Create(Clip, Plan);

        Context.CurrentPosition = new WorkflowState(Plan.Id, Plan.Steps.First().Id, TaskStatus.Running);

        dataClipRepository.Save(Clip);
        stateRepository.Save(Context);

        this.workflowPlanId = workflowPlanId;
        this.dataClipId = Clip.Id;
        SetSessionValues();
    }

    public void Resume(int workflowPlanId, int dataClipId)
    {
        Clip = dataClipRepository.GetById(dataClipId);
        Plan = workflowRepository.GetById(workflowPlanId);
        Context = ContextFactory.Current.Create(Clip, Plan);

        this.workflowPlanId = workflowPlanId;
        this.dataClipId = dataClipId;
        SetSessionValues();
    }

    private void SetSessionValues()
    {
        session[SessionKeys.DataClipId] = dataClipId;
        session[SessionKeys.WorkflowPlanId] = workflowPlanId;
    }

    private static class SessionKeys
    {
        public const string WorkflowPlanId = "Workflow.WorkflowPlanId";
        public const string DataClipId = "Workflow.DataClipId";
    }
}
\$\endgroup\$
  • \$\begingroup\$ What is the difference between Continue and Resume? Aren't they actually the same? I don't mean the code but from a user perspecitive? When should I use which one? Their names are very confusing. \$\endgroup\$ – t3chb0t Sep 2 '15 at 16:37
  • \$\begingroup\$ Continue is used when it is assumed workflowPlanId and dataClipId are available in the Session. Resume is used when you know a workflow session has not been initialized yet. Basically a user can start a workflow, save it, then comeback days later and click on a workflow from a laundry list of saved workflows to resume one. At that point it is loaded from the database using the dataClipId provided by the user clicking the link. \$\endgroup\$ – programmer Sep 2 '15 at 16:42
  • \$\begingroup\$ Are these properties auto-properties with public setters because you are using property injection? \$\endgroup\$ – moarboilerplate Sep 2 '15 at 20:02
2
\$\begingroup\$

Comments:

  • The structure and naming of this class says "I am a runner" but it isn't really a runner if it is basically a mechanism to persist and retrieve state. This is especially confounding because you have a separate state class. There's nothing to start, continue, or resume, except storage and retrieval of state from the session. In fact, conceptually, this class is the context, and your WorkflowContext and WorkflowState classes appear to be more appropriate runner objects.

  • Continue() and Resume()--combine them into one method where you either always pass values, or never pass values. Either the calling class needs to be able to check the session and retrieve values to pass in, or the method needs to be changed to be able to hydrate the session if it hasn't been initialized yet. This class should probably encapsulate direct session access, so I'd go with the latter. Otherwise, calling code has to make assumptions about the session, which is brittle and an encapsulation no-no.

  • All void methods and a ton of properties. This class exposes a lot of objects, and more specifically, objects that this class internally initializes. This makes your void methods stateful and full of side effects. If you are using this class to basically initialize and hold a bag of properties that you then access in business logic, why not make your methods stateless by returning a state object from this method? The current class is not really suited to be both the state (session) AND the state-getter. It would be much better to move the properties on this class to a separate state object, and change this class to have CreateNewSession() and GetExistingSession() methods that then return a state object. You can probably combine that state object with your other classes, too. Again, do ensure nothing external can assume anything about the application session.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I didn't design the classes/objects this class contains. Right now I'm just trying to work with them and document how they all fit together and are instantiated. Thanks for the feedback, I think I'll implement all three of your comments! \$\endgroup\$ – programmer Sep 3 '15 at 2:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.