4
\$\begingroup\$

I encountered with the following code in the work. The MyItemCoordinator should receive MyItem objects, process them, add them to MyItemList collection and then pass them to the listeners.

My questions are:

  1. When I saw NewMyItem method I thought that MyItemCoordinator is a factory but when I saw NewMyItem's implementation I found out that it creates, sorts, calling to listeners and it's not static. So MyItemCoordinator is not a factory and NewMyItem method does way more things that it should to do according to its name, right?
  2. If it's not a factory so why NewMyItem method receives so many params, creates an object, adds to the collection? Why not to do:

    example:

    var coordinator = new MyItemCoordinator();
    var myItem = new MyItem(...PARAMS..)
    coordinator.AddMyItemToList(myItem)
    

    Inside AddMyItemToList we can sort and call the listeners. This is more clear way to do, isn't it?

  3. So, is this implementation correct? Doesn't it have something wrong?

    public class MyItemCoordinator
    {    
        ...
    
        public IList<MyItem> MyItemList = new List<MyItem>();
    
        public void NewMyItem(int id, ...15_MORE_PARAMS)
        {
            MyItem myItem = new MyItem(id,...ONLY_5_PARAMS_WITHIN_15_ARE_USED);
            AddMyItemToList(myItem);
            SortMyItemList();
            if (MyItemDetectedHandler != null)
                MyItemDetectedHandler(this);
        }
    
        public void AddMyItemToList(MyItem myItem)
        {
            //makes some checking and adds myItem to MyItemList
        }
    
        ...
    }
    
\$\endgroup\$
1
  • 2
    \$\begingroup\$ MyItemList allows direct manipulation of the internal state, bypassing AddMyItemToList etc. \$\endgroup\$ Apr 17, 2014 at 12:05

1 Answer 1

2
\$\begingroup\$

You're right, but you should take this to its logical conclusion.

  • Creating item belongs to a factory class.
  • Tracking a list of items (coordinating?) is another responsibility.
  • Handling the new item (or list? Not clear from the code) belongs to a third class.

With this design you can reliability test each component in isolation. Merging these responsibilities into a single class makes testing difficult and blocks alternate implementations.

\$\endgroup\$
1
  • \$\begingroup\$ I agree that creating item can be moved to a factory class, but as it's implemented here with NewMyItem it's wrong. In addition, it's not a factory, right? \$\endgroup\$
    – theateist
    Apr 17, 2014 at 16:10

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.