6
\$\begingroup\$

While I should probably be using Dependency Injection via Ninject or a similar project, I have been attempting to implement an abstract factory design that would provide me with the following easy to read and use syntax:

UniversalFactory.Factory<IFoo>.Create<FooImpl>(... parameters ...);

I'm throwing this experiment on here in order to solicit some constructive feedback both with implementation logic and design reasoning.

Here is my current implementation:

public class UniversalFactory
{
    private readonly IFactory<IFoo> FooFactoryEx;

    public UniversalFactory()
    {
        FooFactoryEx = new FooFactory();
    }

    public IFactory<T> Factory<T>()
    {
        if (typeof (T) == typeof (IFoo))
            return (IFactory<T>)FooFactoryEx;
        return null;
    }
}

public interface IFactory<in TF>
{
    T Create<T>(params object[] p) where T : TF;
}

public class FooFactory : IFactory<IFoo>
{
    public T Create<T>(params object[] p) where T : IFoo
    {
        if (typeof(T) == typeof(FooImpl))
        {
            var id = Convert.ToInt32(p[0]);
            return (T)FooImpl.Create(id);
        }
        return default(T);
    }
}

public interface IFoo
{
    // Some interface
}

public class FooImpl : IFoo
{
    public int Id { get; set; }

    private FooImpl(int id)
    {
        Id = id;
    }

    public static IFoo Create(int id)
    {
        var foo =  new FooImpl(id);
        return foo;
    }
}

This code allows a UniversalFactory to be instantiated and the following to be invoked successfully:

UniversalFactory.Factory<IFoo>().Create<FooImpl>(1);

My reasoning for this approach:

  1. All construction is easily identifiable via the call to UniversalFactory.
  2. Factories are invoked based on the interface implementation being constructed, hiding all details about the specific factory being used from the new object request.
  3. The object type and its construction parameters are clearly identifiable in the request.
  4. Object construction stays in each class, and any additional updates to other objects at construction time can take place in the factory method.

My current implementation has (at a minimum) the following flaws:

  1. There is no compile-time safety that ensures IFoo is implemented by an IFactory. Only that FooImpl implements IFoo is ensured.
  2. UniversalFactory.Factory has to add a check for every interface supported by a factory
  3. Individual factories have to invoke static Create() methods within each class. It would be better if the static create could be invoked in a generic way, and Create(params object[] p) somehow mapped to Create(int a, string b, ...) as needed.

Thoughts? Feedback? Is this approach unnecessary or too complex? Hopefully this experiment is, at the least, interesting to a couple of other SO folk out there.

\$\endgroup\$
  • \$\begingroup\$ obligatory: discuss.joelonsoftware.com/default.asp?joel.3.219431.12 \$\endgroup\$ – Jesse C. Slicer Feb 6 '14 at 4:31
  • \$\begingroup\$ I do love his blog, and I agree with the sentiment. However this is just an experiment with generics and factories and not a serious implementation. Trying to learn by seeing how much I can do with these tools, before actually using a DI framework. \$\endgroup\$ – jmsb Feb 6 '14 at 18:59
  • \$\begingroup\$ Soo, how is UniversalFactory.Factory<IFoo>().Create<FooImpl>(1) any better than new FooImpl(1) (assuming it was public)? \$\endgroup\$ – svick Feb 14 '14 at 20:05
2
\$\begingroup\$

Well what you are trying to do is not strictly speaking an abstract factory. For it to be an abstract factory the use case should look like this:

UniversalFactory.Factory<IFoo>  // client has no idea about IFactory<IFoo> implementation
                .Create(... parameters ...); // client has no idea about IFoo implementation, returns IFoo

Ideally, you should not even need to pass parameters. That is why you are building factories after all. To delegate object creation.

Other minor remarks:

  1. where T : TF - those are not very descriptive names for generic arguments. Perhaps you can come up with better naming.
  2. var id = Convert.ToInt32(p[0]); - i'm not sure i like Convert here. It might hide bugs and misusages. I think that strong cast is better in that case.
  3. public static IFoo Create(int id) - i don't see any reason to use factory method.

    public class FooImpl : IFoo
    {
        public int Id { get; private set; }
    
        public FooImpl(int id)
        {
            Id = id;
        }
    }
    

    this looks a lot cleaner. Also i think Id setter should be always private.

Overall, i think unless you are doing it for studying purposes, you should just grab an existing DI library, as you've metioned yourself :)

\$\endgroup\$
  • \$\begingroup\$ I agree with the remarks, so thanks. I would like to hear more reasoning on why passing in parameters is a bad idea in this case? What if there is more then ID assignment and tracking, and there are required configuration options that need to be known at creation-time? Is that a flaw in the object construction logic, or is the assumption that such a scenario would not arise? \$\endgroup\$ – jmsb Feb 6 '14 at 19:03
  • \$\begingroup\$ @jmblack, well, using params object[] p nullifies the abstraction (you are passing concrete parameters for concrete implementation of IFoo) and does not simplify object creation (calling new FooImpl(...) is just as easy if not easier). And those are two main reason people use factories (abstraction and dependency injection). However, if, for example, IFoo whould contain an Id property which should be set by client, i think it is ok to pass strongly typed id parameter to Create method, as every implementation will need it. \$\endgroup\$ – Nikita B Feb 7 '14 at 5:53
  • \$\begingroup\$ I ended up reworking a lot of my test code based on this feedback. I ran into a new error while doing such though, and have posted that question to SO here (in case you're interested or want to weigh in there or here): stackoverflow.com/questions/21651411 \$\endgroup\$ – jmsb Feb 8 '14 at 20:26
2
\$\begingroup\$

The code is really well written but that is a LOT of power to give one class. It has essentially created a dependency bottleneck.

Most of the advantages of factories is the ability to swap/change/update implementations without extraneous recompiling or changing.

Although here you are calling the Universal Factory (which as an implementation can't be changed without recompiling every class using it)

Second, you are calling the direct implementation in the call itself. this means that you are also dependent on that.

You correctly asserted the correct approach is probably a DI system. Alternatively you could stick with the factory and go with something like

IGlobalFactory factory;//Possibly constructor initialized

IFoo bar = factory.Create<IFoo>();

Whereby the GlobalFactoryImpl is Injected and provides a factory with implementation of your IFoo.

It is usually a bad idea to let your main class be aware there is a FooImpl in this way the GlobalFactoryImplcan return FooImpl when called but a GlobalFooBlargFactoryImpl can return a FooBlargImpl , you still get the ability to swap out implementation but everything is a little more obfuscated.

\$\endgroup\$
  • \$\begingroup\$ UniversalFactory was intended to be a singleton or static, and it would only contain generic logic that would never change. This would allow it to act as the universal entry point for all object creation. You bring up some good points. I may rework this some more and add a V2 edit to the OP for further comments. Thanks! \$\endgroup\$ – jmsb Feb 6 '14 at 19:04
  • \$\begingroup\$ The other issue that I didn't even realize I have, that you pointed out, is the dependency on FooImpl. What I was trying to do here was to have IFoo implemented by FooImpl, FooImpl2, and FooImpl3 and be able to request any of these (or any other implementation of IFoo) from arbitrary locations. \$\endgroup\$ – jmsb Feb 6 '14 at 19:08

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.