C#: Simplfy code using Interfaces + business rules / validation

By Posted on

Problem

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();
    }

}

Solution

  • 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.

Leave a Reply

Your email address will not be published. Required fields are marked *