Correctly use dependency injection

Question

Me and two other colleagues are trying to understand how to best design a program. For example, I have an interface ISoda and multiple classes that implement that interface like Coke, Pepsi, DrPepper, etc....

My colleague is saying that it's best to put these items into a database like a key/value pair. For example:

Key       |  Name
--------------------------------------
Coke      |  my.namespace.Coke, MyAssembly
Pepsi     |  my.namespace.Pepsi, MyAssembly
DrPepper  |  my.namespace.DrPepper, MyAssembly

... then have XML configuration files that map the input to the correct key, query the database for the key, then create the object.

I don't have any specific reasons, but I just feel that this is a bad design, but I don't know what to say or how to correctly argue against it.

My second colleague is suggesting that we micro-manage each of these classes. So basically the input would go through a switch statement, something similiar to this:

ISoda soda;

switch (input)
{
   case "Coke":
      soda = new Coke();
      break;       
   case "Pepsi":
      soda = new Pepsi();
      break;
   case "DrPepper":
      soda = new DrPepper();
      break;
}

This seems a little better to me, but I still think there is a better way to do it. I've been reading up on IoC containers the last few days and it seems like a good solution. However, I'm still very new to dependency injection and IoC containers, so I don't know how to correctly argue for it. Or maybe I'm the wrong one and there's a better way to do it? If so, can someone suggest a better method?

What kind of arguments can I bring to the table to convince my colleagues to try another method? What are the pros/cons? Why should we do it one way?

Unfortunately, my colleagues are very resistant to change so I'm trying to figure out how I can convince them.

Edit:

Seems like my question is a little bit hard to understand. What we're trying to figure out is how to best design an application that utilizes multiple interfaces and contains many concrete types that implement those interfaces.

Yes, I know that storing this stuff in the database is a bad idea, but we simply do not know of a better way. This is why I'm asking how do we do it better.

public void DrinkSoda(string input)
{
   ISoda soda = GetSoda(input);
   soda.Drink();
}

How do we correctly implement GetSoda? Should we rethink the entire design? If it's a bad idea to pass around magic strings like this, then how should we do it?

User inputs such as "Diet Coke", "Coke", "Coke Lime", or whatever would instantiate a type of Coke, so multiple keywords map to a single type.

A lot of people have said that the posted code above is bad. I know it's bad. I'm looking for reasons to present to my colleagues and argue why it's bad and why we need to change.

I apologize if this explanation is still difficult to understand. It's hard for me to describe the situation because I don't really understand how to put it in words.

Answer 1

I think the problem isn't even a factory or whatever; it's your class structure.

Does DrPepper behave differently from Pepsi? And Coke? To me, it sounds like you're using interfaces improperly.

Depending on the situation, I would make a Soda class with a property BrandName or similar. The BrandName would be set to DrPepper or Coke, either through an enum or a string (the former seems like a better solution for your case).

Solving the factory problem is easy. In fact, you don't need a factory at all. Just use the constructor of the Soda class.

Answer 2

This is the pattern I generally use if I need to instantiate the correct class based on a name or other serialized data:

public interface ISodaCreator
{
    string Name { get; }
    ISoda CreateSoda();
}

public class SodaFactory
{
    private readonly IEnumerable<ISodaCreator> sodaCreators;

    public SodaFactory(IEnumerable<ISodaCreator> sodaCreators)
    {
        this.sodaCreators = sodaCreators;
    }

    public ISoda CreateSodaFromName(string name)
    {
        var creator = this.sodaCreators.FirstOrDefault(x => x.Name == name);
        // TODO: throw "unknown soda" exception if creator null here

        return creator.CreateSoda();
    }
}

Note that you will still need to make a lot of ISodaCreator implementations, and somehow collect all the implementations to pass to the SodaFactory constructor. To reduce the work involved in that, you could create a single generic soda creator based on reflection:

public ReflectionSodaCreator : ISodaCreator
{
    private readonly ConstructorInfo constructor;

    public string Name { get; private set; }

    public ReflectionSodaCreator(string name, ConstructorInfo constructor)
    {
        this.Name = name;
        this.constructor = constructor;
    }

    public ISoda CreateSoda()
    {
        return (ISoda)this.constructor.Invoke(new object[0])
    }
}

and then just scan the executing assembly (or some other collection of assemblies) to find all soda types and build a SodaFactory like this:

IEnumerable<ISodaCreator> sodaCreators =
    from type in Assembly.GetExecutingAssembly().GetTypes()
    where type.GetInterface(typeof(ISoda).FullName) != null
    from constructor in type.GetConstructors()
    where constructor.GetParameters().Length == 0
    select new ReflectionSodaCreator(type.Name, constructor);
sodaCreators = new List<ISodaCreator>(sodaCreators); // evaluate only once
var sodaFactory = new SodaFactory(sodaCreators);

Note that my answer complements that of Mark Seemann: he is telling you to let clients use an abstract ISodaFactory, so that they don't have to care about how soda factory works. My answer is an example implementation of such a soda factory.

Answer 3

As a general rule, it's more object-oriented to pass around objects that encapsulate concepts than passing around strings. That said, there are many cases (particularly when it comes to UI) when you need to translate sparse data (such as strings) to proper Domain Objects.

As an example, you need to translate user input in form of a string into an ISoda instance.

The standard, loosely coupled way of doing this is by employing an Abstract Factory. Define it like this:

public interface ISodaFactory
{
    ISoda Create(string input);
}

Your consumer can now take a dependency on ISodaFactory and use it, like this:

public class SodaConsumer
{
    private readonly ISodaFactory sodaFactory;

    public SodaConsumer(ISodaFactory sodaFactory)
    {
        if (sodaFactory == null)
        {
            throw new ArgumentNullException(sodaFactory);
        }

        this.sodaFactory = sodaFactory;
    }

    public void DrinkSoda(string input)   
    {   
        ISoda soda = GetSoda(input);   
        soda.Drink();   
    }

    private ISoda GetSoda(input)
    {
        return this.sodaFactory.Create(input);
    }
}

Notice how the combination of the Guard Clause and the readonly keyword guarantees the SodaConsumer class' invariants, which lets you use the dependency without worrying that it might be null.

Answer 4

Each DI library has its own syntax for this. Ninject looks like the following:

public class DrinkModule : NinjectModule
{
    public override void Load()
    {
        Bind<IDrinkable>()
            .To<Coke>()
            .Only(When.ContextVariable("Name").EqualTo("Coke"));
        Bind<IDrinkable>()
            .To<Pepsi>()
            .Only(When.ContextVariable("Name").EqualTo("Pepsi"));
        // And so on
    }
}

Of course, it gets pretty tedious to maintain the mappings, so if I have this kind of system then I usually end up putting together a reflection-based registration similar to this one:

public class DrinkReflectionModule : NinjectModule
{
    private IEnumerable<Assembly> assemblies;

    public DrinkReflectionModule() : this(null) { }

    public DrinkReflectionModule(IEnumerable<Assembly> assemblies)
    {
        this.assemblies = assemblies ??
            AppDomain.CurrentDomain.GetAssemblies();
    }

    public override void Load()
    {
        foreach (Assembly assembly in assemblies)
            RegisterDrinkables(assembly);
    }

    private void RegisterDrinkables(Assembly assembly)
    {
        foreach (Type t in assembly.GetExportedTypes())
        {
            if (!t.IsPublic || t.IsAbstract || t.IsInterface || t.IsValueType)
                continue;
            if (typeof(IDrinkable).IsAssignableFrom(t))
                RegisterDrinkable(t);
        }
    }

    private void RegisterDrinkable(Type t)
    {
        Bind<IDrinkable>().To(t)
            .Only(When.ContextVariable("Name").EqualTo(t.Name));
    }
}

Slightly more code but you never have to update it, it becomes self-registering.

Oh, and you use it like this:

var pop = kernel.Get<IDrinkable>(With.Parameters.ContextVariable("Name", "Coke"));

Obviously this is specific to a single DI library (Ninject), but all of them support the notion of variables or parameters, and there are equivalent patterns for all of them (Autofac, Windsor, Unity, etc.)

So the short answer to your question is, basically, yes, DI can do this, and it's a pretty good place to have this kind of logic, although neither of the code examples in your original question actually have anything to do with DI (the second is just a factory method, and the first is... well, enterprisey).

Answer 5

I'll be honest, and I know others disagree, but I find that abstracting class instantiation into XML or a database to be a horrible idea. I'm a fan of making code crystal clear even if it sacrifices some flexibility. The downsides I see are:

• It is hard to trace through code and find where objects are created.
• Other people working on the project are required to understand your proprietary format for instantiating objects.
• On small projects you will spend more time implementing dependency injection that you will actually switching around classes.
• You won't be able to tell what's being created until runtime. That's a debugging nightmare, and your debugger won't like you for it either.

I'm actually a bit tired of hearing about this particular programming fad. It feels and looks ugly, and it solves a problem that few projects need to have solved. Seriously, how often are you going to be adding and removing sodas from that code. Unless it's practically every time you compile then there's so little advantage with so much extra complxexity.

Just to clarify, I think that inversion of control can be a good design idea. I just don't like having code externally embedded just for instantiation, in XML or otherwise.

The switch-case solution is fine, as that's one way of implementing the factory pattern. Most programmers won't be hard pressed to understand it. It's also more clearly organized than instantiating subclasses all around your code.

Answer 6

The best introduction to dependency injection that I've come across is actually the quick series of articles that form a walkthrough of the concepts on the Ninject wiki site.

You haven't really given much background on what you're trying to accomplish, but it looks to me like you're trying to design a "plug-in" architecture. If this is the case - your feeling is correct - both those designs are in a word, horrible. The first one will be relatively slow (database and reflection?), has massive unnecessary dependencies on a database layer, and will not scale with your program, since each new class will have to be entered into the database. What's wrong with just using reflection?

The second approach is about unscalable as it comes. Every time you introduce a new class, old code has to be updated, and something has to "know" about every object that you'll be plugging in - making it impossible for third parties to develop plugins. The coupling you add, even with something like a Locator pattern, means that your code is not as flexible as it could be.

This may be a good opportunity to use dependency injection. Read the articles I linked. The key idea is that the dependency injection framework will use a configuration file to dynamically load the classes that you specify. This makes swapping out components of your program very simple and straightforward.