3
\$\begingroup\$

I have a workflow; within the workflow I can have many stages. I want to open and close a stage based on some business rules. E.g., stage 7 cannot be opened, because stage 5 + 6 need to be closed first; or, check whether key data x is filled out: if so, you can close stage 4.

How can I optimize or improve my code, below? Are there any guidelines or help?

public interface IHigherLevelWorkflow
{
    int InstructionID { get; }
    int DashboardID { get; }
    string Name { get; }
    string Description { get; }
    int CurrentStage { get; }
    IList<DashboardStageDetailDTO> Stages { get; }
    InstructionStageProgress CloseStage(int progressId);
    InstructionStageProgress OpenStage(int progressId);
}

public class Workflow : IHigherLevelWorkflow
{
    private readonly IReadOnlySession _readOnlySession;
    private readonly IUserSession _userSession;
    public Workflow (Instruction instruction, IReadOnlySession readOnlySession, IUserSession userSession)
    {
        Guard.ArgumentNotNull(instruction, "instruction");
        InstructionID = instruction.InstructionID;
        MyInstruction = instruction;

        Guard.ArgumentNotNull(readOnlySession, "readOnlySession");
        _readOnlySession = readOnlySession;

        _userSession = userSession;
        Guard.ArgumentNotNull(userSession, "usersession");

        MyWorkflow = ResolveDashboard();
    }

    public Instruction MyInstruction { get; private set; }
    public int InstructionID { get; private set; }
    public int DashboardID
    {
        get { return MyWorkflow.DashboardID; }
    }

    public string Name
    {
        get { return MyWorkflow.Name; }
    }

    public string Description
    {
        get { return MyWorkflow.Description; }
    }

    public int CurrentStage
    {
        get { return MyWorkflow.CurrentStage; }
    }

    public IList<DashboardStageDetailDTO> Stages
    {
        get {return MyWorkflow.Stages; }
    }

    public InstructionStageProgress CloseStage(int progressId)
    {
        //user wants to close a stage...
        //is stage valid?
        var stage = Stages.FirstOrDefault(x => x.ProgressID == progressId);

        if(stage == null)
            throw new NoAccessException(string.Format("stage progress #{0} does not exist",progressId));

        //what kind of stage is it? do some rules and checks...

        //if ok, save to db - do we require a update session? Should we return the object and allow caller to save?
        //where will notifications be generated?

        var mystage = _readOnlySession.Single<InstructionStageProgress>(x => x.ProgressID == progressId);

        if (mystage == null)
            throw new NoAccessException(string.Format("stage progress #{0} does not exist", progressId));

        mystage.IsCompleted = true;
        mystage.DateCompleted = DateTime.Now;
        mystage.CompletedByID = _userSession.UserID;

        return mystage;
    }

    private static void OpenStageRules(DashboardStageDetailDTO stageDetail)
    {
        //depending on what stage it is, we will allow the opening of the stage...
        var rules = new RulesException<DashboardStageDetailDTO>();

        switch (stageDetail.ViewHook)
        {
            case StageViewHook.NoView: //first and last stage can never be opened, this is down to the system
                rules.ErrorForModel("this stage cannot be opened");
                break;
            case StageViewHook.Quotes:
                rules.ErrorForModel("this stage cannot be opened");
                break;
            case StageViewHook.Configure:
                break;
            case StageViewHook.Report:
                rules.ErrorForModel("this stage cannot be opened");
                break;
            case StageViewHook.Check:
                rules.ErrorForModel("this stage cannot be opened");
                break;
        }

        if(rules.Errors.Any())
            throw rules;
    }

    public InstructionStageProgress OpenStage(int progressId)
    {
        //user wants to open a stage...
        //is stage valid?
        var stage = Stages.FirstOrDefault(x => x.ProgressID == progressId);

        if (stage == null)
            throw new NoAccessException(string.Format("stage progress #{0} does not exist", progressId));

        //what kind of stage is it? do some rules and checks...
        OpenStageRules(stage);
        //if ok, save to db - do we require a update session? Should we return the object and allow caller to save?
        //where will notifications be generated?

        var mystage = _readOnlySession.Single<InstructionStageProgress>(x => x.ProgressID == progressId);

        if (mystage == null)
            throw new NoAccessException(string.Format("stage progress #{0} does not exist", progressId));

        mystage.IsCompleted = false;
        mystage.DateCompleted = null;
        mystage.CompletedByID = null;
        mystage.DateInitiated = DateTime.Now;
        mystage.InitiatedByID = _userSession.UserID;

        return mystage;
    }

    private WorkflowDTO MyWorkflow { get; set; }

    private WorkflowDTO ResolveDashboard()
    {
        var wf = _readOnlySession.All<Dashboard>()
            .GroupJoin(_readOnlySession.All<DashboardStage>(),
                       x => x.DashboardID,
                       y => y.DashboardID,
                       (dash, ds) =>
                       new
                           {
                               Dashboard = dash,
                               DashboardStages =
                           ds.DefaultIfEmpty().Join(_readOnlySession.All<InstructionStageProgress>()
                                                        .Where(x => x.InstructionID == InstructionID),
                                                    x => x.DashboardStageID,
                                                    y => y.StageID,
                                                    (d, s) => new
                                                                  {
                                                                      InstructionStageProgress = s,
                                                                      DashboardStage = d,
                                                                      StageOwnerCompany =
                                                                  _readOnlySession.All<Company>().FirstOrDefault
                                                                  (
                                                                      c => c.CompanyID == s.StageOwnerID),
                                                                  })
                           })
            .Select(dash =>
                    new WorkflowDTO
                        {
                            DashboardID = dash.Dashboard.DashboardID,
                            Name = dash.Dashboard.Name,
                            Description = dash.Dashboard.Description,
                            CurrentStage = dash.DashboardStages
                                .OrderByDescending(o => o.DashboardStage.SortOrder)
                                .First(x => x.InstructionStageProgress.IsCompleted).DashboardStage.
                                DashboardStageID,
                            Stages = dash.DashboardStages
                                .Select(stage =>
                                        new DashboardStageDetailDTO
                                            {
                                                Name = stage.DashboardStage.Name,
                                                Description = stage.DashboardStage.Description,
                                                StageID = stage.DashboardStage.DashboardStageID,
                                                StageNumber = stage.DashboardStage.SortOrder,
                                                ViewHook =
                                                    stage.DashboardStage.StageHook.Convert<StageViewHook>(),
                                                ProgressID = stage.InstructionStageProgress.ProgressID,
                                                StageOwnerID = stage.InstructionStageProgress.StageOwnerID,
                                                StageOwnerCompanyName =
                                                    stage.StageOwnerCompany != null
                                                        ? stage.StageOwnerCompany.Identifier
                                                        : string.Empty,
                                                Progress =
                                                    new ProgressDTO
                                                        {
                                                            DateInitiated =
                                                                stage.InstructionStageProgress.DateInitiated,
                                                            DateCompleted =
                                                                stage.InstructionStageProgress.DateCompleted,
                                                            IsCompleted =
                                                                stage.InstructionStageProgress.IsCompleted,
                                                            InitiatedByUserID =
                                                                _readOnlySession.All<User>().Single(
                                                                    u =>
                                                                    u.UserID ==
                                                                    stage.InstructionStageProgress.InitiatedByID)
                                                                .
                                                                Login,
                                                            SignedOffByUserID =
                                                                _readOnlySession.All<User>().Single(
                                                                    u =>
                                                                    u.UserID ==
                                                                    stage.InstructionStageProgress.CompletedByID)
                                                                .
                                                                Login,
                                                        }
                                            })
                                .OrderBy(s => s.StageNumber)
                                .ToList()
                        })
            .FirstOrDefault();



        if (wf == null)
            throw new NoAccessException(string.Format("dashboard not found for instruction #{0}", InstructionID));

        return wf;

    }
}

Here are my stage and progress DTO classes:

public class DashboardStageDetailDTO
{
    public int ProgressID { get; set; }    
    public int StageID { get; set; }
    public string Name { get; set; }
    public string Description { get; set; }
    public int StageNumber { get; set; }
    public StageViewHook ViewHook { get; set; }
    public string Url { get; set; }
    public bool IsViewAvailable { get; set; }
    public ProgressDTO Progress { get; set; }
    public int? StageOwnerID { get; set; }
}

public class ProgressDTO
{
    public bool IsCompleted { get; set; }
    public DateTime? DateInitiated { get; set; }
    public string InitiatedByUserID { get; set; }
    public DateTime? DateCompleted { get; set; }
    public string SignedOffByUserID { get; set; }
}

Update to my code:

public class Workflow : IHigherLevelWorkflow
{
    public Workflow(Instruction instruction, Dashboard dashboard, IEnumerable<StageProgress> progress)
    {
        Guard.ArgumentNotNull(instruction, "instruction");
        InstructionID = instruction.InstructionID;
        MyInstruction = instruction;

        Guard.ArgumentNotNull(dashboard, "dashboard");
        Description = dashboard.Description;
        Name = dashboard.Name;
        DashboardID = dashboard.DashboardID;

        Guard.ArgumentNotNull(progress, "progress");
        Progress = progress.OrderBy(x => x.Stages.SortOrder)
            .ToDictionary(x => x.Progress.ProgressID, y => y);
    }

    public Instruction MyInstruction { get; private set; }
    public int InstructionID { get; private set; }
    public int DashboardID { get; private set; }
    public string Name { get; private set; }
    public string Description { get; private set; }
    public IDictionary<int, StageProgress> Progress { get; private set; }

    public int CurrentStage
    {
        get
        {
            //get the last stage that was completed
            return Progress.Last(x => x.Value.Progress.IsCompleted).Value.Progress.ProgressID;
        }
    }

}

public class WorkflowService
{
    private readonly IReadOnlySession _readOnlySession;
    private readonly IUserSession _userSession;
    private readonly IUpdateSession _updateSession;
    private readonly INotificationService _notificationService;
    public WorkflowService(IUserSession userSession, IReadOnlySession readOnlySession, IUpdateSession updateSession)
    {
        _userSession = userSession;
        _readOnlySession = readOnlySession;
        _updateSession = updateSession;
        _notificationService = new NotificationService();
    }

    private int InstructionID { get; set; }
    private int ProgressId { get; set; }

    private DashboardInfo GetMyDashboard()
    {
        var wf = _readOnlySession.All<Dashboard>()
            .GroupJoin(_readOnlySession.All<DashboardStage>(),
                       x => x.DashboardID,
                       y => y.DashboardID,
                       (dash, ds) =>
                       new
                           {
                               Dashboard = dash,
                               DashboardStages = ds.DefaultIfEmpty()
                           .Join(_readOnlySession.All<InstructionStageProgress>()
                                     .Where(x => x.InstructionID == InstructionID),
                                 x => x.DashboardStageID,
                                 y => y.StageID,
                                 (d, s) => new StageProgress
                                               {
                                                   Progress = s,
                                                   Stages = d,
                                               })
                           })
            .Select(dash =>
                    new DashboardInfo
                        {
                            Dashboard = dash.Dashboard,
                            Stages = dash.DashboardStages.ToList()
                        })
            .FirstOrDefault();

        if (wf == null)
            throw new NoAccessException(string.Format("dashboard not found for instruction #{0}", InstructionID));

        return wf;
    }

    /// <summary>
    /// we could determine workflow by instruction source?
    /// </summary>
    /// <param name="instruction"></param>
    /// <returns></returns>
    private IHigherLevelWorkflow DetermineWorkflow(Instruction instruction)
    {
        var dashboard = GetMyDashboard();

        var wfhandler = new Workflow (instruction, dashboard.Dashboard, dashboard.Stages);
        return wfhandler;
    }

    /// <summary>
    /// these rules will be based on the workflow...
    /// </summary>
    /// <param name="workflow"></param>
    /// <returns></returns>
    private static IWorkflowStageHandler DetermineWorkflowRules(IHigherLevelWorkflow workflow)
    {
        return new WorkflowStageHandler();
    }

    public void CloseWorkflowStage(int instructionid, int progressId)
    {
        InstructionID = instructionid;
        ProgressId = progressId;
        var instruction = _readOnlySession.GetData(instructionid, _userSession);

        //set up workflow + rules...
        var wfhandler = DetermineWorkflow(instruction);
        var stageHandler = DetermineWorkflowRules(wfhandler);

        StageProgress mystage;
        stageHandler.CloseStage(_readOnlySession, _userSession, wfhandler, progressId, out mystage);

        //raise notification

        //persist...
        _updateSession.Update(mystage.Progress);
        _notificationService.SaveNotifications(_updateSession);
        _updateSession.CommitChanges();
    }

    public void OpenWorkflowStage(int instructionid, int progressId)
    {
        InstructionID = instructionid;
        ProgressId = progressId;
        var instruction = _readOnlySession.GetData(instructionid, _userSession);

        //user wants to close a stage...
        //is stage valid?
        var wfhandler = DetermineWorkflow(instruction);
        var stageHandler = DetermineWorkflowRules(wfhandler);

        StageProgress mystage;
        stageHandler.OpenStage(_readOnlySession, _userSession, wfhandler, progressId, out mystage);

        //raise notification//

        //persist...
        _updateSession.Update(mystage.Progress);
        _notificationService.SaveNotifications(_updateSession);
        _updateSession.CommitChanges();
    }

}
\$\endgroup\$
1
  • \$\begingroup\$ The ResolveDashboard()-method is almost impossible to read due to heavy overusage of LINQ. I would begin with some refactoring there \$\endgroup\$
    – Mattias
    Jan 22, 2012 at 16:16

1 Answer 1

1
\$\begingroup\$
  • Conceptually you say you open and close stages. Then I'd expect to see open and close methods in a stage class, not in the workflow class.
  • Looks like stages are non-trival. I'd expect to see these created w/in some kind of factory pattern
  • Wheres the exception handling? Seeing NoAccessException thrown in the example implementation of the interface concerns me. I don't see your higherLevelworkflow code expecting exceptions at all. I like to see the highest level of my application catching exceptions so it can deal with them. Maybe you want to send an email, or put them in a database, or something else or none-of-the-above.
  • Generally, I don't see the code design implementing appropriate abstraction levels.
  • In terms of "guidelines", ResolveDashboard() is violating many: Too many nested levels, too long. It's complex, but no comments. Totally untestable!! It's essentially one massive WTF statement. It either has to all work or not at all.
  • Generally, you should pull the code in ResolveDashboard() into a separate class/layer for data fetching. Generally this ultimately makes the various code pieces easier to test also.
  • Are there other kinds of concrete workflows? It's odd that the IHigherWorkflow interface has such a generically named implementation.
  • Are there "ILowerLevelWorkflow"s? If not then consider renaming the interface.
\$\endgroup\$
3
  • \$\begingroup\$ Yes I have been told there will be different higherlevel workflows - each with its own validation + rules (potentially), hence the need for an interface (I will resolve the higherlevel interface depending on certain variable) \$\endgroup\$
    – Haroon
    Jan 23, 2012 at 11:34
  • \$\begingroup\$ point3 - handling exceptions: my asp.net mvc application will handle this, not my domain/core project, it is handled inside a filter globally, some errors will show up in UI, some errors will force a 404 - depends on what exception I am throwing, so there is no need to handle anything within these classes (unless it is relevant to the code creating/calling/depending on this class) \$\endgroup\$
    – Haroon
    Jan 23, 2012 at 11:37
  • \$\begingroup\$ I have refactored, could you please comment? \$\endgroup\$
    – Haroon
    Jan 23, 2012 at 12:24

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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