Interface based design problem

Question

My question is related class design. It is really hard to explain everything here so I am taking an example which is similar to my problem.

Requirements: Customer can execute a single action or he can combine multiple actions.

Based on the above requirements, the classes were structured as below:

interface ICustAction
{
    void Execute();
}
class CustAction:ICustAction
{
    // this class contains a single action to be performed
    public void Execute()
    {
       //should execute customer action
    }
}

class CustActionCollction:ICustAction
{
   // list of actions to be performed
    private List<CustAction> _custActions;



    public void Execute()
    {
        //should execute all the customer action in the list
    }
}

class ExecutionEngine
{
    public void Execute(ICustAction action)
    {
        //executes customer action/ customer action collection based on reference
        action.Execute();
    }
}

New Requirement: Customer should be able to add CustActionCollection inside CustActionCollection.

Example: Customer action collection: customer action 1: customer action 2: customer action collection 1: (this action collection might have multiple customer actions) customer action 3:

Based on these requirements, the updated CustActionCollection class looks like this

 class CustActionCollction:ICustAction
{

   // list of actions to be performed
    private List<ICustAction> _custActions;



    public void Execute()
    {
        //should execute all the customer action in the list
    }
}

Now here is the new requirement where I need expert opinion.

I need to check whether a particular action is defined more than once in a customer action collection??

Customer action collection: customer action 1: customer action 2: customer action collection 1: (this action collection might have multiple customer actions) customer action 3:

SO, the check should be recursive including all child collection.

Question:

Should I add this check method inside the interface or it should be exclusively for the customer action collection class??

A)If the method is defined in the interface then what will be the signature of the method then implementation for both classes?

B)If the method is not in the interface then what will be the implementation of the method?

I'm Sorry, if I could not explain it properly.

Answer 1

I would do it in this way: (I don't know if you only want to check for duplicates or execute without duplicates, so both are here).

Two interfaces

interface ICustAction
{
    void Execute();
}

interface ICustActionCollection : ICustAction
{
    List<ICustAction> Actions { get; }

    List<ICustAction> GetAllActions();

    void ExecuteDistinct();

    bool HasDuplicates();
}

ICustActionCollection gonna handle the whole problem.

Lets see the implementations:

    class CustAction : ICustAction
    {
        public void Execute()
        {
        }
    }

    class CustActionCollection : ICustActionCollection
    {
        private List<ICustAction> _actions;

        public List<ICustAction> Actions
        {
            get
            {
                if (_actions == null)
                {
                    _actions = new List<ICustAction>();
                }

                return _actions;
            }
        }

        public void Execute()
        {
            foreach (var action in _actions)
            {
                action.Execute();
            }
        }

        public void ExecuteDistinct()
        {
            List<ICustAction> executionList = GetAllActions();
            foreach (var action in distinctActions(executionList))
            {
                action.Execute();
            }
        }

        public List<ICustAction> GetAllActions()
        {
            Stack<ICustAction> actions = new Stack<ICustAction>();
            List<ICustAction> executionList = new List<ICustAction>();
            actions.Push(this);

            while (actions.Count > 0)
            {
                ICustAction action = actions.Pop();

                if (action is ICustActionCollection)
                {
                    foreach (var i in ((ICustActionCollection)action).Actions)
                    {
                        actions.Push(i);
                    }
                }
                else
                {
                    executionList.Add(action);
                }
            }

            return executionList;
        }


        public bool HasDuplicates()
        {
            List<ICustAction> actions = GetAllActions();
            return distinctActions(actions).Count() < actions.Count;
        }

        private IEnumerable<ICustAction> distinctActions(IEnumerable<ICustAction> list)
        {
            return list.Distinct();
        }
}

The ICustAction implementation is simple.

At class CustActionCollection, you implement both interface.

Methods:

distinctActions your custom implementation on what is distinct? I use the default here, but you can override the Equals and GetHashCode methods in class CustAction to define what is a duplicate for example.

GetAllActions will return the whole ICustAction list 'recursively'.

ExecuteDistinct and HasDuplicates are using GetAllActions and distinctActions to achieve their goals (I think these methods are clear, let me know if not).

You can test it if you change the CustAction class to:

class CustAction : ICustAction
{
    public int ID { get; private set; }

    public CustAction(int id)
    {
        ID = id;
    }

    public void Execute()
    {
        Console.WriteLine("Execute: {0}", ID);
    }
}

and you run this:

static void Main(string[] args)
{
    CustAction a = new CustAction(1);
    CustAction b = new CustAction(2);
    CustAction c = new CustAction(3);
    CustActionCollection coll = new CustActionCollection();
    CustActionCollection coll2 = new CustActionCollection();

    coll.Actions.Add(a);
    coll.Actions.Add(b);
    coll.Actions.Add(coll2);
    coll2.Actions.Add(a);
    coll2.Actions.Add(c);

    Console.WriteLine("Without distinct");
    coll.Execute();

    Console.WriteLine("With distinct");
    Console.WriteLine("Has duplicates: {0}", coll.HasDuplicates());
    coll.ExecuteDistinct();

    coll2.Actions.Remove(a);
    Console.WriteLine("Duplicate removed, simple Execute");
    Console.WriteLine("Has duplicates: {0}", coll.HasDuplicates());
    coll.Execute();
}

The output gonna be this:

Without distinct
Execute: 1
Execute: 2
Execute: 1
Execute: 3
With distinct
Has duplicates: True
Execute: 3
Execute: 1
Execute: 2
Duplicate removed, simple Execute
Has duplicates: False
Execute: 1
Execute: 2
Execute: 3

EDIT: I changed the ICustActionCollection interface to

ICustactionCollection : ICustAction

So if a class implements ICustActionCollection you can be sure that it gonna implement ICustAction too.

negra

Answer 2

If I understand your question it seems that checking for duplicates will only need to happen in the case of a CustActionCollection. As a result I think you best implementation is to:

1. Rely on the Equals implementation to check for CustAction equality. (Doesn't need to be but makes the most sense in my opinion).

2. In the CustCollection.evaluate() method, generate a master List of all CustActions and check for duplicates. It will look something like this (this in the class CustActionCollection):

   public void Execute()
   {
       //should execute all the customer action in the list
       if (DuplicatesExist())
       {
           // Handle error
       }
   }
   
   private bool DuplicatesExist()
   {
       List<CustAction> actions = ProcessCollection(this);
   
       for (int x = 0; x < actions.Count; x++)
       {
           for (int y = x + 1; y < actions.Count; y++)
           {
               if(actions[x].Equals(actions[y])
               {
                   return true;
               }
           }
       }
   
       return false;
    }
   
   // Recursively get all customer actions
   private List<CustAction> ProcessCollection(CustActionCollction actions)
   {
       List<CustAction> ret = new List<CustAction>();
   
       foreach (ICustAction action in actions._custActions)
       {
           if (action is CustActionCollction)
           {
               ret.AddRange(ProcessCollection(action as CustActionCollction);
           }
           else
           {
               ret.Add(action as CustAction);
           }
        }
   
        return ret;
    }
   

Answer 3

I can think of few ways to do this:

1. Define the check method in collection. In this method, iterator over the individual elements of the collection, building a list of CustAction. A runtime type checking will be required to iterate over the contained actions when an element is CustActionCollection
2. Create method containsAction in ICustAction. CustAction implements it by comparing it to this

Answer 4

It all depends on what other changes this structure will need (if any) and how the comparing of Actions will be done. Supposing you can already compare any ICustAction to any other: I dont suggest putting the duplication checker method into the interface, since it will do completly the same in all implementations in case you do so, later on if you want to do some minor change in the inner workings of the dupliaction checking you will need to code a lot (we most likely try to avoid these siutations in oop, not just because it is more comfortable, but because what is more comfortable will have less mistakes in it too).

If i were you i would put some helper property into ICustAction which provides all needed information (unique identifier if you like) for the duplication checker method to compare one with another. At this point you are able to write the checker once, right inside CustActionCollction.

Regards, Peter