Tiered Design With Analytical Widgets - Is This Code Smell?

Question

The idea I'm playing with right now is having a multi-leveled "tier" system of analytical objects which perform a certain computation on a common object and then create a new set of analytical objects depending on their outcome. The newly created analytical objects will then get their own turn to run and optionally create more analytical objects, and so on and so on. The point being that the child analytical objects will always execute after the objects that created them, which is relatively important. The whole apparatus will be called by a single thread so I'm not concerned with thread safety at the moment. As long as a certain base condition is met, I don't see this being an unstable design but I'm still a little bit queasy about it.

Is this some serious code smell or should I go ahead and implement it this way? Is there a better way?

Here is a sample implementation:

namespace WidgetTier
{
    public class Widget
    {
        private string _name;

        public string Name
        {
            get { return _name; }
        }

        private TierManager _tm;
        private static readonly Random random = new Random();

        static Widget()
        {
        }

        public Widget(string name, TierManager tm)
        {
            _name = name;
            _tm = tm;
        }

        public void DoMyThing()
        {
            if (random.Next(1000) > 1)
            {
                _tm.Add();
            }
        }
    }

    //NOT thread-safe!
    public class TierManager
    {
        private Dictionary<int, List<Widget>> _tiers;
        private int _tierCount = 0;
        private int _currentTier = -1;
        private int _childCount = 0;

        public TierManager()
        {
            _tiers = new Dictionary<int, List<Widget>>();
        }

        public void Add()
        {
            if (_currentTier + 1 >= _tierCount)
            {
                _tierCount++;
                _tiers.Add(_currentTier + 1, new List<Widget>());
            }
            _tiers[_currentTier + 1].Add(new Widget(string.Format("({0})", _childCount), this));
            _childCount++;
        }

        //Dangerous?
        public void Sweep()
        {
            _currentTier = 0;
            while (_currentTier < _tierCount)  //_tierCount will start at 1 but keep increasing because child objects will keep adding more tiers.
            {
                foreach (Widget w in _tiers[_currentTier])
                {
                    w.DoMyThing();
                }
                _currentTier++;
            }
        }

        public void PrintAll()
        {
            for (int t = 0; t < _tierCount; t++)
            {
                Console.Write("Tier #{0}: ", t);
                foreach (Widget w in _tiers[t])
                {
                    Console.Write(w.Name + "  ");
                }
                Console.WriteLine();
            }
        }
    }

    class Program
    {
        static void Main(string[] args)
        {
            TierManager tm = new TierManager();

            for (int c = 0; c < 10; c++)
            {
                tm.Add();   //create base widgets;
            }

            tm.Sweep();
            tm.PrintAll();

            Console.ReadLine();
        }
    }
}

Answer 1

The biggest potential issue here is that the Sweep method is iterating over a collection (_tiers) that could potentially change in a call to Widget.DoMyThing().

The .NET BCL classes do not allow collection to change while they are being iterated. The code the way it's structured, exposes a risk that this may happen.

Beyond that, the other problem is that the structure of the program makes it difficult to understand what happens in what order. Perhaps you could separate the phase of the program that recursively assembles the model from the portion that visits the model and performs computations.

Answer 2

Yes, I call the following code smell:

        _currentTier = 0; 
        while (_currentTier < _tierCount)  //_tierCount will start at 1 but keep increasing because child objects will keep adding more tiers. 
        { 
            foreach (Widget w in _tiers[_currentTier]) 
            { 
                w.DoMyThing(); 
            } 
            _currentTier++; 
        } 

You are iterating over a collection as it is changing. I mean the first iteration, not the second. You're obviously accounting for the change (hence the < _tierCount rather than a standard foreach) but it's still a smell, IMO.

Would I let it into production code? Possibly. Depends on the scenario. But I'd feel dirty about it.

Also: Your _tiers member could just as easily be a List<List<Widget>>.

Answer 3

+1 to both Randolpho and LBushkin.

However, I gave it some thought and I think I know why this smells. The pattern as I have implemented seems to be some kind of perversion of the Builder pattern. What would work better is creating a Composite out of a series of analytical steps which, taken as a whole, represents some kind of meaningful state. Each step of the analytical process (behavior) should be distinct from the output Composite (state). What I've implemented above meshes both the state and the behavior together. Since the state-holders and state-analyzers are one and the same object, this also violates the Single Responsibility Principle. The approach of having a composite "build itself" opens up the possibility of creating a vicious loop, even though my prototype above has a deterministic completion.

Links:

Builder Pattern

Composite Pattern