14

This question already has an answer here:

During a code review I have started having a bit of a dilemma as to whether use dependency injection or not. I would like to hear your thoughts, because this is kind of an ongoing theme and would help in the future code reviews as well.

Before I start I want to say that from my knowledge DI is mostly used for dependency management and better, much easier unit testing (correct me if I am wrong please).

Now here is the scenario:

This is the class that will be used as a dependency only once and probably for good. Meaning it will stay that way for a while and no other class will be using it.

Reason being is because it is a refactoring of some legacy code and there was not much to be done but to separate it into another class, at least as a first good step, for following the SRP.

Also, another thing is that the hypothetical doSomethingImportant() hits the database.

public SomeClass
{
   public Object doSomethingImportant()
   {
      ...
   }
}

Based on that information do you think it is okay to new up the dependency in the other class as opposed to using DI since:

Dependency management argument kind of falls off since it is only going to be used once.

&

Unit testing also falls off since I would rather do an integration or acceptance test to see how the method is interacting with the database in a real-life application.

public SomeOtherClass
{
   private readonly SomeClass _someClass = new SomeClass();

   public Object doSomethingImportantUsingDependency()
   {
      ...
   }
}

I was personally inclined towards doing DI because it is considered good practice, but for a second there it felt like I was blindingly following the rules and not thinking it over since there are always exceptions to the rule.

What are your thoughts on this? I would love to hear.

PS: I don't think this is a generic "when should I use DI" question because it is very specific to this particular situation when unit tests are of no use and the class is going to be used only once so there is no need to centralize it for dependency management (even though it is good practice in general).

marked as duplicate by gnat, user40980, user22815, gbjbaanb, Kilian Foth Apr 9 '15 at 9:00

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

  • 2
    wonder how it is possible to use it only once? I mean, one gets at least one use in the main code and at least one more in unit test - that makes it two (and frankly, my experience has been that second use - in tests - is often much more worthwhile than first one) – gnat Apr 8 '15 at 16:03
  • @gnat It is like a very specific helper class for the one it is being injected into. I guess you could argue as to why not put them all in one and make the method private - and that is a good point but it is frankly 2 different responsibilities which is the reason it was extracted. But currently only used once and will probably stay that way for a little while. As for testing - it would much rather be tested with integration/acceptance than unit which takes away the need for it being mocked. Hope that clarifies. – AvetisG Apr 8 '15 at 16:09
  • 1
    sorry, I did not notice that. Though, given that one of the answers already appears to be pointing to use in unit tests, you may consider making it somehow easier for readers to see – gnat Apr 8 '15 at 16:37
  • 1
    dependency injection need not be centralized at all (see other answers). And even though this class may be used only once, it still needs to be tested for its behavior. Plus, more importantly, the class using it needs to be tested. And though you could test both classes through the outer one (the one aggregating the other class), your test classes and cases will be much simpler if you focus them on a single class. Which means that the outer class needs to be tested only for its own behavior and its interaction with the inner class. And that automatically is an argument for some form of DI. – Marjan Venema Apr 8 '15 at 18:18
  • 1
    "Unit testing also falls off since I would rather do an integration or acceptance test to see how the method is interacting with the database in a real-life application.". integration or acceptance tests are more costly in terms of maintenance than a unit test, so I'm not sure why those would be your preference. If this is going to be common as you move forward with other classes, the costs of this are going to be very real. Especially if the class representing the legacy code will be rewritten to not be legacy (by further decomposition), you may want to rethink. – Andy Apr 9 '15 at 1:44
15

In C# it is trivial to provide optional dependency injection without coupling yourself to your dependency too tightly:

public class SomeOtherClass {
    private readonly ISomeClass _someClass;

    public SomeOtherClass(ISomeClass dependency = null) {
        _someClass = dependency ?? new SomeClass();
    }
}

(or you can make the constructors explicit if your company dislikes default params)

In my experience, "oh we'll never need another one" is naive. Business changes. Technology changes. Make dependencies flexible.

And this sort of thing strikes the right balance between usability (yes, you'll almost always use the common/default one) and flexibility (but the ctor is there if you need it) - all with a nice simple line of code, while also providing some semblance of error correcting robustness. It's so trivial and clearly beneficial, there's no reason not to do it for the simple/straight-forward case.

  • 3
    Well... you are coupling yourself to the constructor of "SomeClass()" That's probably what the OP should be thinking about -- not so much the dependency itself, but the complexity of getting "SomeClass()" into a usable state. Why is DI good? Primarily because it decouples state management of classes. If you know for sure that "SomeClass()" is a simple class with minimal state and always will be, then using a default constructor isn't so troublesome. On the other hand, if you have to build 10 classes to create one instance of "SomeClass()" (or think you might some day)... – Calphool Apr 8 '15 at 15:57
  • @Calphool - if you need to build some more complex dependency, then you're right - it doesn't belong here and you should just have the ctor to pass that dependency in. – Telastyn Apr 8 '15 at 15:58
  • @Calphool A class's constructor is part of its public API. Knowing how to construct SomeClass is no more harmful than knowing about any of its methods that don't appear in ISomeClass. The same applies transitively to any classes required to create SomeClass. – Doval Apr 8 '15 at 16:58
  • 1
    @Doval: Think about how constructors are different from other methods, and you'll probably begin to see why this kind of coupling can become insidious over time (it's innocent in the beginning, and falls apart as a system grows, which is why comment was qualified). This is what DI frameworks like Ninject were invented to solve. – Calphool Apr 8 '15 at 17:33
  • 2
    @Calphool The point of DI isn't to reduce the complexity of instantiating something. DI makes that someone else's problem, but not because creating the dependency is hard; it does so because creating the dependency couples you to it. But if you're going to provide a default implementation, you're going to be coupled to it no matter what. If creating the instance is complex, passing the buck to someone else doesn't solve the problem; the code to do so has to be written by someone, somewhere. – Doval Apr 8 '15 at 18:05
11

There's a development principle along the lines of DRY and SOLID called YAGNI that is designed to help streamline your development efforts in getting things done and not getting paralysed with indecision over what to do.

If you later find that you need to enhance your class, then you will. YAGNI says not to worry so much over it now 'cos you probably won't need to spend that extra effort. Get it done, come back to it if you really need to.

Some say its the opposite of SOLID but really its all about trading off all the factors involved in development, you don't have infinite time or resources (and 'perfect' code never is IMHO).

So here, you say it doesn't need DI complexity... so there's your answer. Don't put it in. Move on to all the other things you have to do.

  • 1
    I think sometimes it's possible to apply YAGNI over-hastily. The reason that it's okay to not do something upfront is that if your code and test coverage is good, it'll be easy and safe enough to modify later. Dependency inversion (generally via injection) is widely considered a significant part of writing that kind of good, easy-to-modify code. So I think if you want to argue YAGNI, you need to explicitly justify why DI is something you don't need to do up-front in this case- different to, say, good naming, loose coupling, DRY, SRP, etc. – Ben Aaronson Apr 8 '15 at 15:50
  • 2
    @BenAaronson true, but then its easy to apply everything over-hastily. In this case, DI in an instance where he's never going to need it, he;s already outlined why he thinks this is the case, and I'd argue to agree that simplicity is more important. Less code means less bugs, more dev productivity and more time to spend down the pub! – gbjbaanb Apr 8 '15 at 16:01
  • Yeah, I'm not saying I disagree with your conclusion, just felt like there's maybe a chunk missing from your argument. Or at least maybe a point to be underscored so that whoever reads your answer doesn't find themselves saying "My code isn't DRY? Screw it, YAGNI!" – Ben Aaronson Apr 8 '15 at 16:03
  • @BenAaronson: just to make it clear for other readers (I am sure you know already): applying YAGNI to DI will not make the code immediately "dirty". However, trying to apply YAGNI to legitimate code duplication will sooner or later strike back on you. – Doc Brown Apr 8 '15 at 18:38
5

If you do not apply DI as long as you do not really need it (not even for unit testing), nothing bad will happen. The code does not become error prone, "overly complicated", or hard to maintain. And if you decide to refactor the dependency out later, it will most probably not be much more effort than doing it now. That's a case where the YAGNI principle applies.

However, what you should check is if you are sure you really do not want to be able to unit test SomeOtherClass in isolation from SomeClass, and if the imposed dependency on the assemblies where SomeOtherClass and SomeClass live will not become a problem. If you are 100% sure that the answer to the former questions is "yes", then you can ignore DI.

0

The answer like most things is "it depends".

If you want to unit-test the functionality in doSomethingImportantUsingDependency, then you will need to be able to inject the dependency.

However, if all that doSomethingImportantUsingDependency does is some property mapping from the result of your database call, then it would be pragmatic to not bother.

If some other class depends on SomeOtherClass then you can always inject that class instead.

Not the answer you're looking for? Browse other questions tagged or ask your own question.