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.
- 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.
- 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
Adding Complexity to
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:
- Zero or negative
Count on one or more
- Custom equality comparer which will incorrectly identify unequal
CustomKeys as equal, or vice versa
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
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.