Class responsibilities OOP design I'm having some questions on creating a design for simple approval workflow. Some arbitrary thing (ItemToApprove) needs approval process. Approval process begins with creating ApprovalRoute which contains several approval steps (ApprovalStep). ItemToApprove goes sequentially through all steps until gets status Approved. On each step there are several Approvers with equal priority. If one approver approves item, step becomes Approved.

public class ApprovalRoute
{
    private LinkedList&lt;ApprovalStep&gt; _approvalSteps;
    private ApprovalStep _currenStep;

    public bool? Approved { get; private set; }

    public void Approve(Approver approver)
    {
        // ... some cheks if already approved etc
        _currentStep.Approve(approver);

        if (_currentStep.Approved == true)
        {
            MoveToNextStep(); // changes _currentStep
        }

        if (AllStepsApproved()) {
           Approved = true;
        }
    }
}

public class ApprovalStep
{
    public bool? Approved { get; private set; }
    private List&lt;Approver&gt; _approvers; 

    public ApprovalStep(List&lt;Approver&gt; approvers)
    {
        _approvers = approvers;
    }

    public void Approve(Approver approver)
    {
        if (_approvers.Contains(approver))
        {
            Approved = true;
        }
    }
}

public class ItemToApprove
{
    public bool? Approved { get; private set; }
    private ApprovalRoute _approvalRoute;

    public void StartApprovalWorkflow(ApprovalRoute route)
    {
        _approvalRoute = route;
    }

    public void Approve(Approver approver)
    {
        _approvalRoute.Approve(approver);
        if (_approvalRoute.Approved == true)
        {
            Approved = true;
        }
    }
}

public class Approver
{
    public string Name { get; private set; }
    public Approver(string name)
    {
        Name = name;
    }

    // ... ovveriding Equals etc
}


So some console client would be like this

      var itemToApprove = new ItemToApprove();
      var aprovalRoute = new ApprovalRoute(new ApprovalStep(new List&lt;Approver&gt;
      {
          new Approver("One"),
          new Approver("Neo"),
      });
      itemToApprove.StartApprovalWorkflow(aprovalRoute);

      while (itemToApprove.Approved == null)
      {
          Console.Write("Your name: ");

          string approverName = Console.ReadLine();
          itemToApprove.Approve(new Approver(approverName));

      }


I thik this design has some smell because calling Approve delegates further down to ApprovalStep. The second smell I feel more naturally call Approve on Approver passing ItemToApprove

// ....
var approver = new Approver();
approver.Approve(itemToApprove);


What do you think?
Random thoughts

In this scenario, there's few implementation details making me ask "why"?


In ApprovalRoute what benefits LinkedList&lt;T&gt; gives over any other IEnumerable&lt;T&gt; ?
In ApprovalStep and ItemToApprove classes is it necessary to make Approved property    nullable ?
Won't it, at least, hurt coder's eyes if bool type is compared using equality operator == instead of someBool or !someBool ?



And to anwer myself right-away


I think it could be e.g. Queue&lt;T&gt; since



  ItemToApprove goes sequentially through all steps until gets status
  Approved



No, because default value will always be false, it is never checked if property  HasValue and there's no separate logic for nulls.
Yes, it hurts the eyes and makes you say nonconstructive things like "WTF dude". Just don't do it.



On the higher level then


  I think this design has some smell because calling Approve delegates further down to
  ApprovalStep.
  itemToApprove.Approve(new Approver(approverName));


You are right. Now it feels like the ItemToApprove is actually approving something. This method shouldn't exist.


  The second smell I feel more naturally call Approve on Approver passing ItemToApprove 
  approver.Approve(itemToApprove);


Again you are right. But in this case it would seem than approver is approving the item itself, not a step along the route.



Suggestions

I'd remove the Approve method from ItemToApprove and ApproveStep classes and make it ApprovalRoute's responsibility to dequeue ApprovalSteps as they are approved. Dequeued step is the current step and when queue is empty, item is fully approved.

Better yet I'd pass the ItemToApprove along the ApprovalRoute, not the other way around. You could also have multiple items going down the same route at the same time, all having their own "states".

Something like below.

Related dotnet Fiddle can be found here https://dotnetfiddle.net/6BlA30 (web page "console" seems little buggy though).





public class Program
{
    // console app for testing
    public static void Main()
    {
        // some items
        var item1 = new ItemToApprove("item1");
        var item2 = new ItemToApprove("item2");

        // approvers
        var approver1 = new Approver("app1");
        var approver2 = new Approver("app2");

        // steps w/ authorized approvers
        var step1 = new ApprovalStep("step1", approver1);
        var step2 = new ApprovalStep("step2", approver1, approver2);

        // create route
        var route = new ApprovalRoute("route1", new[] { step1, step2 });
        // add items to current route
        route.AddItems(item1, item2);

        // start manual testing
        Console.WriteLine("** {0} has {1} items **", route.Name, route.Items.Count());
        while (route.Items.Any())
        {
            Console.Write("&gt; ");
            string input = Console.ReadLine() ?? string.Empty;
            var parts = input.Split(' ');

            if (parts.Length == 2)
                route.ApproveStep(new ItemToApprove(parts[0]), new Approver(parts[1]));
            else if (input.Equals("?"))
                route.Status();
        }
        Console.WriteLine("No more items in route");
        Console.ReadLine();
    }
}




Implementation

For examples sake all classes are simplistic and inherit abstract ModelBase.

public abstract class ModelBase : INotifyPropertyChanged
{
    public event PropertyChangedEventHandler PropertyChanged;

    public string Name { get; set; }

    [NotifyPropertyChangedInvocator]
    protected virtual void OnPropertyChanged([CallerMemberName] string propertyName = null)
    {
        var handler = PropertyChanged;
        if (handler != null) 
            handler(this, new PropertyChangedEventArgs(propertyName));
    }

    public override bool Equals(object obj)
    {
        if (obj == null || GetType() != obj.GetType())
            return false;

        return (Name == ((ModelBase)obj).Name);
    }

    public override int GetHashCode()
    {
        return Name.GetHashCode();
    }
}


Actual "business objects" below

public class ItemToApprove : ModelBase
{
    private bool _isApproved;

    public ItemToApprove(string name)
    {
        Name = name;
    }

    public bool IsApproved
    {
        get { return _isApproved; }
        set { _isApproved = value; OnPropertyChanged(); }
    }
}

public class Approver : ModelBase
{
    public Approver(string name)
    {
        Name = name;
    }
}

public class ApprovalStep : ModelBase
{
    public ApprovalStep(string name, params Approver[] approvers)
    {
        Name = name;
        Approvers = approvers;
    }

    public IEnumerable&lt;Approver&gt; Approvers { get; private set; }
}

public class ApprovalRoute : ModelBase
{
    // sequential steps in this route
    private readonly List&lt;ApprovalStep&gt; _steps;
    // items and step indexes
    private readonly Dictionary&lt;ItemToApprove,int&gt; _itemsInRoute = 
        new Dictionary&lt;ItemToApprove,int&gt;();

    public ApprovalRoute(string name, params ApprovalStep[] steps)
    {
        Name = name;
        _steps = new List&lt;ApprovalStep&gt;(steps);
    }

    public void AddItems(params ItemToApprove[] items)
    {
        foreach (var item in items)
        {
            item.PropertyChanged += (sender, args) =&gt;
                {
                    if (item.IsApproved)
                        Console.WriteLine("Item '{0}' fully approved", item.Name);
                };
            _itemsInRoute.Add(item, 0);
        }
    }

    public IEnumerable&lt;ItemToApprove&gt; Items { get { return _itemsInRoute.Keys; } }

    public void ApproveStep(ItemToApprove item, Approver approver)
    {
        if (!_itemsInRoute.ContainsKey(item))
        {
            Console.WriteLine("There is no '{0}' on '{1}'", item.Name, Name);
            return;
        }

        int index = _itemsInRoute[item];
        var step = _steps[index];

        if (step.Approvers.Contains(approver))
        {
            Console.WriteLine("{0} for {1} approved by {2}", 
                step.Name, item.Name, approver.Name);

            index++;
            if (index &gt; _steps.Count - 1)
            {
                _itemsInRoute.Single(i =&gt; i.Key.Equals(item)).Key.IsApproved = true;
                _itemsInRoute.Remove(item);
            }
            else
            {
                _itemsInRoute[item] = index;
            }
        }
        else
        {
            Console.WriteLine("{0} for {1} can only be approved by {2}", 
                step.Name, item.Name, string.Join(",", step.Approvers.Select(a =&gt; a.Name)));
        }
    }

    public void Status()
    {
        foreach (var entry in _itemsInRoute)
            Console.WriteLine("{0} at {1}", entry.Key.Name, _steps[entry.Value].Name);
    }
}


Hope this made any sense.