Injecting collections as constructor dependencies

Question

We are using constructor dependency injection in our application. Following that approach we inject everything using an injection container so we are able to replace any dependency with a Mock.

Some colleagues complain about this procedure as they say that injecting collections of any type is going too far with injection and they prefer to call a new() inside the constructor.

What do you think about this? What procedure do you follow if you are using dependency injection?

---

One example of a real use case. We have a Dictionary with an special behavior, it allows to add the same key several times and it just increments an internal counter and it decreases the counter when removing elements from it only removing the element from memory when the counter is 0 (the class has been simplified a bit but I think you can get the idea):

public class ExtendedDictionary<TValue> : IExtendedDictionary<TValue>
{
    private readonly IDictionary<CustomKey, TValue> dictionary;

    public ExtendedDictionary(IDictionary<CustomKey, TValue> dictionary)
    {
        this.dictionary = dictionary;
    }

    public void Add(ParameterBase parameter, DriveInformation driveInformation, TValue data)
    {
        CustomKey key = new CustomKey (driveInformation, parameter.Index);

        if (this.dictionary.ContainsKey(key))
        {
            this.dictionary.Single(p => p.Key == key).Key.Count++;
        }
        else
        {
            this.dictionary.Add(key, data);
        }
    }

    public void Remove(ParameterBase parameter, DriveInformation driveInformation, TValue data)
    {
        CustomKey key = new CustomKey (driveInformation, parameter.Index);

        if (this.dictionary.ContainsKey(key))
        {
            this.dictionary.Single(p => p.Key == key).Key.Count--;
        }
        else
        {
            this.dictionary.Remove(key);
        }
    }
}

Answer 1

Pros and Cons

As with any situation where there's a judgement call to be made, it's worth weighing up the main pros and cons. I'll summarize them as I see them, then go into some aspects a little deeper to try to show why- although there are some situational aspects- in the large majority of cases the factors pretty heavily favor a particular answer.

Pros

• Ease of writing the "arrange" phase of tests. When you are trying to test a class with heavily encapsulated state, it can be frustrating that arranging a particular situation often can't be done directly and instead involves multiple calls on the class. Being able to inject in state directly removes this hassle.

Cons

• You are introducing a crack in your encapsulation- now, instead of external code never being able to access or mutate your contained state, it can mutate it once, when the object is created
• You are increasing, by a great deal, the cognitive complexity of working out potential routes through your code. Instead of the single, simple, degenerative case of an empty collection, you now have to think about every possible way that the collection could be initialized and think about what error or corner cases this may introduce
• Any new checks you need to introduce to deal with potentially invalid collections being passed in have to be runtime checks. By creating the collection yourself, you have a compile-time guarantee that certain error cases will never happen
• You weaken your ability for your class to guarantee invariants- facts about its internal state which it can take responsibility for ensuring are always true.
• It isn't really dependency injection. Ultimately, the Dictionary in this case is not conceptually a dependency. Rather than being a collaborator that your class uses to get access to behavior it doesn't want to take responsibility for itself, it's just a data structure which your class should be fully responsible for managing. (This isn't a con per se, but an indication that you shouldn't just shove this under the umbrella of DI and take that as justification). Note this won't always be the case for, e.g. all IEnumerable<T>s, but for a collection which exists purely to represent your class's internal state and not provide any special behaviour, it probably will be.

That's quite a few more cons than pros, but it's not fair to just count them up, since there's quite a lot of overlap- for example, not being able to guarantee invariants is very closely related to gaps in encapsulation. So it's worth going a bit deeper with reference to your ExtendedDictionary<TValue> class.

Adding Complexity to ExtendedDictionary<TValue>

Disclaimer: Your example isn't actually complete- some of the logic is wrong and some of the types aren't defined. So I may be making wrong assumptions in a couple of places. I'll try to be as clear about that as possible.

I looked at your example, and tried to think of ways I could break your class by injecting a bad starting state. Here are the ones I came up with:

• Null Dictionary.
• Zero or negative Count on one or more CustomKeys.
• Custom equality comparer which will incorrectly identify unequal CustomKeys as equal, or vice versa

Possibly Count is a uint, which would remove one part of one of those possibilities, but otherwise I think those are all error cases. In fact, since you're using an IDictionary rather than a Dictionary it's really much worse- you could have it do more or less anything! But it would be easy enough to change it to a concrete class so I won't worry too much about that.

For the first two, these could be protected against relatively easily by adding guard clauses as the beginning of your constructor. But this isn't free:

• You've replaced compile-time guarantees that none of these will happen with potential runtime failures
• You have to work out all these possibilities. I know it took me a non-negligible amount of time to come up with those three, and there may well be more that I've missed altogether. If the class changes, new ones may arise, or old ones may become redundant. You've basically added a huge amount of cognitive complexity to reasoning about how this class works.
• You have to write and maintain more tests, to cover these cases. Since the motivation behind this was testability, this is very counterproductive; the easiest test to write is no test at all!

You may argue that, given that the constructor is only meant to be used by your IOC container and tests, this is all irrelevant, because anyone injecting a non-empty, non-default-comparer Dictionary from anywhere outside the tests is something you're not allowed to do. But to me that's very similar to making all getters and setters public and saying it's just a rule that you're not actually supposed to use them outside of tests.

What About Testability?

Okay, so what about the pro? If you can't make sure a class actually works through testing it, what use is making sure it's all nicely encapsulated?

Well, first I'd say there are two aspects to "testability": the ease of writing tests, and the ease of maintaining tests.

One great thing about unit tests is it puts your api first- if a class is hard to use by its consumers, you learn that early on. But if you write a class that- for example- lacks encapsulation, you find that code which uses that class is easy to write, but hard to maintain. If you want to- for example- add some new business logic associated with a poorly-encapsulated class, that logic may not just require a change to that class, it may spray out across all the code that uses it.

Unfortunately, tests don't really let you detect maintainability (as opposed to writeability) issues early. More likely, when you make a change that requires lots of fixes to production code, you'll probably also have to make lots of fixes to test code too. The upshot is that giving yourself a backdoor into the class when you first start writing tests may make that stage quicker, but just like with production code, you may be creating a maintenance headache for yourself later.

Having said that, let's look again at ExtendedDictionary<TValue>. If we remove that constructor parameter, is it really that hard to test? Again, I can't say for certain since your code is missing parts, but wouldn't it be relatively easy to write a helper method:

private ExtendedDictionary<object> _extendedDictionary = new ExtendedDictionary<object>();
private void ArrangeState(Dictionary<CustomKey, object> state)
{
    foreach(var kvp in state)
    {
        for(int i=0; i<kvp.Key.Count;i++)
        {
            _extendedDictionary.Add(something, something, kvp.Value); //Not sure exactly what should go to those other parameters
        }
    }
}

This doesn't seem all that painful, and I think usually for well-designed classes that aren't doing too much, setting up a particular state shouldn't be too much of a headache

But...

So all that said, I'm pretty heavily in favor of not doing this. However, I appreciate that the ExtendedDictionary<TValue> was chosen as a simple example, and it's probably not impossible that you may hit some situation where, for whatever reason, tests would be prohibitively difficult to write without using a technique like this.

If you are in that situation, I think the first course of action should be to consider that a design smell and give serious thought to refactoring your way out of the problem. If that fails, then injecting your collection may, ultimately be warranted.

So as a solution to a specific, rare problem, I would treat it with caution but not reject it outright. But I would certainly not do it proactively, as your question seems to indicate. You'll ultimately get a lot more benefit- in both tests and production code- from sealing up that encapsulation as tight as you can and writing a few extra test helper methods.

Answer 2

Dependency injection is very easy to take too far. You should use is whenever it decreases your overall effort, not because someone says it's a "good practice".

That said, I don't see any fundamental difference between collections and other data used in constructors. If they are liable to change often, or are hard to mock for testing otherwise, go ahead and inject them, and if you have so many of them that doing it by hand would lead to too much repeated code, go ahead and use a DI framework. Otherwise, don't. But deciding whether or not to do it based on the type of the injected thing is probably the wrong thing to do.

Answer 3

This strikes me as a bit extreme, and overkill. I think it's a case of taking the common charge to never use new() in a class a bit too literally.

Injection is useful for having classes consume dependencies whose implementations can change or are (or need to be) replaceable. For unit testing, it's useful to get rid of your own classes that are already under test (and whose bugs should not impact testing of other code), or abstractions over items that make it difficult to isolate your code (such as the file system, databases, and things of that nature). A standard collection is (normally) neither of these. If a class needs to maintain a list of items, that is an implementation detail, and one that shouldn't cause you trouble. To whatever degree the contents of that list impact the output should be the focus of your testing efforts.

public class Foo
{
    // limit, not eliminate, new()
    Frobber frobber = new Frobber(); // your enemy, makes isolation harder
    List<Bar> bars = new List<Bar>(); // usually a benign implementation detail
}

Answer 4

Resolve behavior, not data (with some exceptions, eg. constants).

If your business logic needs this data structure, then the business logic is responsible for initializing it, by using new or a factory. Then it can store this data structure in a cache or pass it around to other class through a method call. The container shouldn't have any responsibility in this case.

Answer 5

Since you always pass an empty dictionary to the constructor, in my opinions this is bit too extreme.

Only advantage I can see is that when you unit test, you can pass a mock dictionary and verify add/remove methods are called.

However same can be achieved by simply using count on the custom collection.

So I believe this is an overkill.