Using a class in its constructor C# - Does it smell?

Question

Does the code below smell? I'm refactoring some code and have discovered this circular relationship where foo needs a class which needs an interface which foo itself implements.

In the real code, foo is a Silverlight UserControl and ifoo has methods to do UI type things like raise a dialog box (eg ShowMessage). The needsAnIfoo class is a (kind of) controller which uses the ifoo interface whenever it want's to do anything with the UI. I have different "themed" UI's which implement iFoo and have the same boiler plate code in their constructor. The needsAnIfoo has various properties which are databound to the UI (so it's kind of a model too.)

It compiles and runs fine, but I'm wondering if there's a better way.

So, does it smell?

    interface ifoo
    {
        void bar();
    }

    class foo : ifoo
    {
        readonly needsAnIfoo _needsAnIfoo;
        internal foo()
        { 
            _needsAnIfoo = new needsAnIfoo(this);
        }

        #region ifoo Members
        public void bar()
        {
            throw new NotImplementedException();
        }
        #endregion
    }

    class needsAnIfoo
    {
        readonly ifoo _myfoo;
        public needsAnIfoo(ifoo foo)
        {
            _myfoo = foo;
        }
    }

    static void Main(string[] args)
    {
        foo foo = new foo();
    }

Perhaps I should new up the needsAnIfoo without passing the iFoo in the constructor and then give it the iFoo in an Initialize method. But this looks very odd:

    foo foo = new foo();
    needsAnIfoo needsAnIfoo = new needsAnIfoo(foo);
    foo.Initialise(needsAnIfoo);

Answer 1

It doesn't look right to me. Smells fragile.

Have you considered looking at either a builder or factory pattern to create the relevant objects and establish the relationships between them? It might provide a safer way forward.

Answer 2

Sounds like a great place to institute a pattern, I'd say Factory Method.

class Factory
{
public:
    virtual needsAnIfoo* Create(ProductId);
};

needsAnIfoo* Factory::Create(ProductId id)
{
    if (id == TYPE1) return new needsAnIfoo(ifooType1());
    if (id == TYPE2) return new needsAnIfoo(ifooType2());
    ...
    return 0;
}

Then you would use it like so:

Factory f = new Factory();
theme1 = f.Create(TYPE1);
theme2 = f.Create(TYPE2);

Patterns are your friend!

Answer 3

I agree that a Builder or Factory pattern, or similar, would be better. The provided code is not very testable and is, as mentioned, kind of fragile, so some form of dependency injection would be good.

The pattern to use will depend how foo and needsAnIFoo use each other. You might need to consider the Observer pattern as well, if needsAnIFoo is a subject, foo is an observer, and bar() is an update method.

Answer 4

It sounds like this may be overly complicated, and that you are making your theme be a controller and have a controller (by having both classes implement ifoo)

You may get better results if you separate the concepts of theme and controller, so that the controller has a theme. Then, for example, when the controller does something, like pop up a dialog, it looks into its theme to find out what font to use.

Like this:

interface itheme {} // to describe properties of the theme
class theme : itheme {}// a bunch of different themes, this previously would have been the "foo" 
class theme2 :itheme{} //etc.

abstract class icontroller
{
    protected icontroller(itheme ptheme) {theme = ptheme;}

    protected itheme theme;

    //function declarations
    // ....

}
class control : icontroller {} // implements the icontrol functions.
//not sure if you need more than one control implementation... 
//  if not, i'd get rid of the icontrol interface.


//use it by passing a theme into the controller constructor:
icontroller myUIController = new control(new ClassicTheme());

Answer 5

I'm trying to understand the question a little bit but if each needsAnIfoo is tied to only one type of Class and the needsAnIfoo does nothing to itself seems that you can make a static class of needsAnIfoo with extension methods no need to pass this as the constructor arg.

extension method programming guide

Answer 6

Here's the factory with an observer - seems to do the trick and avoids newing the "controller" inside my themed UI.

   interface ifoo
    {
        void bar();
    }

    class foo : ifoo
    {
        public void bar() { Console.Write("do a foo type thing"); }
    }

    class foo2 : ifoo
    {
        public void bar() { Console.Write("do a foo2 type thing"); }
    }

    class needsAnIfoo
    {
        public event EventHandler SomethingIFooCanDealWith;
        System.Threading.Timer _timer;
        public needsAnIfoo()
        {
            _timer = new System.Threading.Timer(MakeFooDoSomething, null, 0, 1000);
        }

        void MakeFooDoSomething(Object state)
        {
            if (SomethingIFooCanDealWith != null) 
            {
                SomethingIFooCanDealWith(this,EventArgs.Empty);
            };
        }
    }

    class fooFactory
    {
        needsAnIfoo _needsAnIfoo = new needsAnIfoo();
        Dictionary<String, ifoo> _themedFoos = new Dictionary<string,ifoo>();
        ifoo _lastFoo = null;

        public void RegisterFoo(String themeName, ifoo foo)
        {
            _themedFoos.Add(themeName, foo);
        }

        public ifoo GetThemedFoo(String theme)
        {
            if (_lastFoo != null) { _needsAnIfoo.SomethingIFooCanDealWith -= (sender, e) => _lastFoo.bar(); };
            ifoo newFoo = _themedFoos[theme];
            _needsAnIfoo.SomethingIFooCanDealWith += (sender, e) => newFoo.bar();
            _lastFoo = newFoo;
            return newFoo;
        }
    }

    static void Main(string[] args)
    {
        fooFactory factory = new fooFactory();
        factory.RegisterFoo("CompanyA", new foo());
        factory.RegisterFoo("CompanyB", new foo2());

        ifoo foo = factory.GetThemedFoo("CompanyA");
        Console.Write("Press key to switch theme");
        Console.ReadKey();

        foo = factory.GetThemedFoo("CompanyB");
        Console.ReadKey();
    }