bjavor
bjavor

Reputation: 501

Struggling with manual dependency injection

I'm working on a small(ish) support library in C# for a project (and possibly similar projects). I've read a lot about constructor injection of dependencies and while I understand the basic motivation behind it, which makes perfectly sense when dealing with certain types of dependencies, I often encounter cases where it just seems to make things harder without providing much benefits.

I would like to show two examples and would greatly appreciate any advice on how to "properly" deal with them. I would also like to note at this point, that for now I would not like to use any DI container for this as I'd like the library to be usable without one...

Example 1: Dynamically created objects

Most DI related articles stress that creation of the object graph happens at the composition root. However not all objects can be created initially. The standard answer to this is to use factories for short lived objects. This works of course but can feel really awkward in some cases...

Let's assume I want to create a series of facade classes around objects in a 3rd party library where one object may return another type of object(s) and I want to wrap both. As a (made up) example let's take a 'Directory' class that has a method 'GetFiles()' which return instances of a 'File' class. The non-DI version of the facades might look like this:

public DirectoryFacade(string path)
{
    m_directory = new Directory(path);
}
public IEnumerable<FileFacade> GetFiles()
{
    foreach(File file in m_directory.GetFiles())
    {
        yield return new FileFacade(file);
    }
}

Fairly simpe and straight forward. Now using DI the DirectoryFacade should not create any FileFacades itself (nor should it create the wrapped Directory, but that's the lesser problem...). Again, the usual proposed solution is to have a factory, e.g. 'FacadeFactory' class that can create all my wrapper objects. But that feels awkward to me for two reasons:

  1. All instances of DirectoryFacade would now need to be passed a FacadeFactory so that they can create FileFacades on the fly. This just somehow feels wrong...

  2. It may be unintuitive for the user of the library having to use a factory for creating the DirectoryFacades instead of just being able to instanciate them directly...

Now, potentially one could use either a default constructor or a static factory method to provide a default implementation, but those seem to be shunned upon, usually citing DI container concerns... Also, using static factory methods everywhere also falls somewhat in the unintuitive category in my oppinion.

Example 2: Internal helper classes

The other case I've found problematic recently is, where you would break up the implementation of a complex class into various sub-components to follow the single responsibility principle, but the user of your class should not need to know that...

For example, take a class that facilitates interacting with a complex configuration string of key-value pairs providing such methods as 'GetValueByKey()' and 'SetValueForKey()' etc.

In a normal use case the client code might want to just do something like:

Configuration config = new Configuration(configString);
config.SetValueForKey('foo','bar');
...

Internally your 'Configuration' class may parse the string into some sort of data structure. For this it may employ a class called 'ConfigurationParser'. And to take it one step further: the parser itself may output a stream of IKeyword objects. The concrete implementation of the IKeyword interface depends of the individual keywords type. So the keywords may be generated by a 'KeywordFactory'... All of this is of absolutely no interest to the client of the 'Configuration' class. In fact I should be able to change all of this to something else without impacting client code. Yet with DI you would be required to do:

KeywordFactory factory = new KeywordFactory();
ConfigurationParser parser = new ConfigurationParser(factory);
Configuration config = new Configuration(parser);
config.ConfigurationString = configString; //NOTE: this could be a constructor param...
config.SetValueForKey('foo','bar');

Now, from the client's perspective this is horrible. As mentioned before, it would also break if the normally internal implementation detail of using either parser or a keyword factory ever changes.

Of course, one could again write some additional factories and facades to provide a default implementation for the client, but does this really make sense? What benefits does it provide? Most likely there will only ever be one implementation for the parser and the keyword factory. Furthermore they will only be used in the 'Configuration' class. The only reason they are in a different class is so that each class has a clearly defined responsibility...

Upvotes: 3

Views: 4903

Answers (3)

Huander Tironi
Huander Tironi

Reputation: 516

I ended up here because I was facing the same dilema. However, there have been some changes since the question was asked, and I managed to solve this using an approach that is made for this.

So here it is. When you register Singletons, there may be scenarios where you'd like to rely on a scoped service. In order to do that, you need to create this scope.

For example, if we have this service:

public class ScopedService : IScopedService {
   
    readonly IScopedRepository _scopedRepository;

    public ScopedService(IScopedRepository scopedRepository) {
        _scopedRepository = scopedRepository;
    }

    public void Method() {
        // my implementation
    }
  
}

And I want to call it from a Singleton, here is how you need to make your singleton:

public class SingletonService : ISingletonService {
    
    readonly IServiceProvider _serviceProvider;

    public SingletonService(IServiceProvider serviceProvider) {

          _serviceProvider = serviceProvider;

    }

    public void SingletonMethod() {

        using (IServiceScope scope = _serviceProvider.CreateScope());
        
        IScopedService scopedService = scope.ServiceProvider.GetRequiredService<IScopedService>();

        scopedService.Method();

    }

}

Assuming you are working with asp.net core 5 or greater, this will do the job.

Take a look at this: https://learn.microsoft.com/en-us/dotnet/core/extensions/scoped-service

Upvotes: 0

Steven
Steven

Reputation: 172646

Now using DI the DirectoryFacade should not create any FileFacades itself

This is where you go wrong. It is never the intension of DI prevent using the new keyword anywhere in your application. DI is about loose coupling. And you shouldn't try to abstract things that don't have to be abstracted. You should make the distinction between services (application logic) and anything else (DTOs, messages, entities, commands, Exceptions, primitives, etc). This is the important distinction between newables and injectables.

In this case your FileFacades are newables. They are short lived, contain only data, no dependencies of their own, have little to no logic to test, and mainly exist as a thin proxy around the real implementation. To inject them means you are interested in them being replaced by a different implementation (for testing for instance). But if that's never the case, don't bother injecting them. In this case it seems perfectly reasonable to let the DirectoryFacade create FileFacade instances. This completely removes the need to have a FileFacadeFactory.

So the keywords may be generated by a 'KeywordFactory'...

This statement again comes from the same misunderstanding. Keywords are probably just data packages. There's no logic to abstract here, and no reason why the ConfigurationParser shouldn't new up the Keyword instances itself.

one could again write some additional factories and facades to provide a default implementation for the client, but does this really make sense? What benefits does it provide?

Look at it from the other side. If you don't structure your code like this, how do you unit test your code?

As I see it, you -as a framework writer- have two responsibilities. 1. Make code that works correctly (by unit testing the hell out of it). 2. Design an API that is easy to use for your clients.

If all those classes are implementation details and are not meant for the client to change, make sure they stay implementation details. For instance, make those classes internal and use poor man's dependency injection on the root type of your library. With poor man's DI you have one public default constructor that calls into another constructor that takes in all the dependencies. Here's an example:

public class MyPublicAPI
{
    public MyPublicAPI()
        : this(new Configuration(
            new ConfigurationParser()))
    {
    }

    internal MyPublicAPI(IConfiguration configuration)
    {
        this.configuration = configuration;
    }
}

Poor man's DI is discouraged by many people, but this advice is usually given in the context of Line of Business applications. If you write a reusable library, having a clean API is of most importance, and your users should indeed not be bothered by having to implement more than one class when running a common use case (of course, if users want to do more complex things, they might need to create more classes). Besides, your users might not use a DI container, or even when they do, you shouldn't force them to have to register all your library types. That's just not very friendly and error prone, because every time you add a new type, you will break their DI configuration.

Poor man's DI is not always an option, because you hardcode the used dependencies. If you want to give your users more flexibility, you still might have to fall back on creating factories that users can override. It all depends on how complex your framework is and which level of flexibility you need to give your users.

Another option is to ship integration DLLs with your library for integrating your library with common DI containers. So see this quite often, but you should not do this, unless users really need that kind of flexibility. You're still opening up your library and this has a few downsides. First of all, you make using your framework harder to user, and the API bigger. This also makes it harder for you to change the framework. Changing the public API of your library can cause breaking changes.

Upvotes: 2

Sam Leach
Sam Leach

Reputation: 12956

I think you are overcomplicating the design with factories and facades, this is your problem, not DI. Inject simpler objects and let them do some work for your class.

Example 1:

With a Di container you would simply

var fileFacade = // Get me a FileFacade and sort out all it's dependencies for me.

You are over using the facade and factory patterns and over-designing. Just because a pattern exists doesn't mean you have to use it everywhere (same goes for DI). What do you want to achieve here? Get some files? Inject an interface that does this. Keep it simple.

Example 2:

This is were a DI container comes in. The first three or four lines are replaced by one. The creation of a new instance of the Configuration class automatically includes its dependencies.

KeywordFactory factory = new KeywordFactory();
ConfigurationParser parser = new ConfigurationParser(factory);
Configuration config = new Configuration(parser);
config.ConfigurationString = configString; //NOTE: this could be a constructor param...
config.SetValueForKey('foo','bar');

Therefore your argument that it's horrible for the client no longer exists.

Configuration config = // Get me a new Configuration instance along with dependencies and ctor arguments.
config.SetValueForKey('foo','bar');

To answer your general question, you are correct, DI is not to be used everywhere. Simply use it when it makes sense. I mainly and completely, use it to facilitate Unit testing.

Upvotes: 1

Related Questions