4

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);
        }
    }
}
  • What do these collections contain? – Ben Aaronson Apr 29 '15 at 15:01
  • How large is the application? – Robert Harvey Apr 29 '15 at 15:04
  • The collections are empty when they are injected. They get filled normally by the class receiving the dependency. – Ignacio Soler Garcia Apr 29 '15 at 15:13
  • @RobertHarvey: the application is big (at least for me). >300KLOC – Ignacio Soler Garcia Apr 29 '15 at 15:13
  • Can you give a code example? I'm having a hard time visualizing what exactly is at play here. – whatsisname Apr 29 '15 at 15:37
3

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.

  • 1
    Well, the first thing I should say is Thank you! It is pretty clear to anyone that you have spent some time with this and I really appreciate that. – Ignacio Soler Garcia May 1 '15 at 13:22
  • @IgnacioSolerGarcia You're welcome! It's one of those questions that helped crystalize a few things I've wondered about myself, and I usually find it useful for my own understanding to write detailed answers to those. – Ben Aaronson May 1 '15 at 13:24
  • Having said that I understand your point but I think that most of the cons do not apply. In our application all the instances are created with a DI container so we don't have to deal with things like a collection injected with state, a null collection and things like that. Its real use is that is as it is was created inside with the benefits of being injected. So as the question says this approach is to be considered only when using injection containers. Logic is not sprayed (that would be a nightmare, of course). – Ignacio Soler Garcia May 1 '15 at 13:26
  • @IgnacioSolerGarcia I do try to address that in the last paragraph of the "Adding Complexity" section. If you're happy to break encapsulation and have it just be a rule that developers follow not to use those breaks, you can go with that, but in most contexts that would widely be considered poor practice. I didn't really go into detail on that because reasons why we should use the compiler to enforce that rather than relying on developers to follow rules seems like its own question. – Ben Aaronson May 1 '15 at 13:33
  • @IgnacioSolerGarcia Also note that test code is code too! Even if those problems with maintainability only actually impact on test code, that's still one more place than necessary – Ben Aaronson May 1 '15 at 13:36
6

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.

4

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
}
  • What about classes calling bars.Contains(), bars.IndexOf() ... you cannot mock the results so have to start arranging lots of things in the tests. – Ignacio Soler Garcia Apr 29 '15 at 15:51
  • 1
    If the class owns the list, the class adds things to it, and the class wants to peek inside of it to see what it put in there, these are implementation details! If your tests need to control for these interactions, I dare say you are doing something very, very wrong. Abstracting over such simple things as collections is the path to madness. – Anthony Pegram Apr 29 '15 at 15:54
  • Maybe you are right. How do you test that the implementation details do not contain bugs? We currently test everything with a 100% code coverage. – Ignacio Soler Garcia Apr 30 '15 at 6:41
  • 100% coverage does not prove there are no implementation bugs - it only tests that all paths are executed. – Chris Cooper Apr 30 '15 at 8:14
  • 50% coverage ensures you that you are not sure, 100% could prove it (even when it does not). I'm in a company where any bug detected in client site is considered as something extremely bad and they expend a lot in testing. – Ignacio Soler Garcia Apr 30 '15 at 9:11
1

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.

  • 1
    Classes contain both data and behavior, unless you're talking about an anemic model. – Robert Harvey Apr 29 '15 at 16:52
  • aren't anemic domain models about classes that contain only data? – devnull Apr 29 '15 at 17:25
  • Yes, exactly. Collection classes contain data and behavior, though I do realize you might not classify them as such. Anemic classes contain only data. – Robert Harvey Apr 29 '15 at 19:21
  • Changed the example to a real one. – Ignacio Soler Garcia Apr 30 '15 at 6:42
  • @IgnacioSolerGarcia Are you using the container to resolve instances of ExtendedDictionary<TValue>? – devnull Apr 30 '15 at 7:44
1

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.

  • You know, it is always easier to follow rules without exceptions that considering on every case if you should or shouldn't inject the element based on the test, but I get your point. Thanks. – Ignacio Soler Garcia Apr 30 '15 at 9:07

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.