New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question: how to resolve instances by key without a factory? #256

Closed
christianspecht opened this Issue Jun 21, 2016 · 10 comments

Comments

Projects
None yet
2 participants
@christianspecht
Copy link

christianspecht commented Jun 21, 2016

I created this issue because of this Stack Overflow question, and the comments under Steven's answer:

@ChristianSpechr I typically wouldn't bother writing uni tests for this, knowing that some integration test would cover this code. But more generally, I never have these kinds of factories in my applications; I don't know your specific use case, but find them being used for the wrong reasons most of the time. – Steven 20 hours ago

[...]

As already mentioned, my use case is "I have a string value from a config file and need an instance of the correct class based on that string". I'm not tied to the design I showed in my question...it's just what I came up with as a solution to my use case. How would you do this kind of stuff without a factory like this? – Christian Specht 1 hour ago

@christianspecht: I would like to answer this last question, but to be able to do so, I need more information about what you are actually trying to achieve. If you're interested in hearing this, could you create a Github issue containing specific information about what you are trying to do. This included actual class names, show how the factory is used throughout the application and possibly the relevant parts of the configuration. Alternatively, you can also contact me directly through mail and if you find this more suited. – Steven 1 hour ago


So here I am 😃

My code is actually public on Github: https://github.com/christianspecht/scm-backup/
It's a backup tool for repositories from multiple source code hosters.
(similar to Bitbucket Backup, a previous project of mine, which backups repositories from Bitbucket only)

It's still in a very early stage right now, and the little bit that already works supports GitHub only.
It has this config file.
The idea is that you can have multiple sources in there, with "hoster": "github", "hoster": "bitbucket" and so on.
The tool will connect to each hoster's API with the provided credentials (not yet in the config file), get a list of all repositories and clone all of them to your local machine.

For this, it will have classes like GithubHoster, BitbucketHoster (doesn't exist yet) and so on.
And that's what I need the factory for: I will need something that takes the strings github and bitbucket from the config file, and returns GithubHoster and BitbucketHoster instances.

Here's the current version of the factory, and here's the container configuration.

Right now, the factory is similar to the first one in my Stack Overflow question.
I'm trying to change it as indicated in the question, because I want to inject the validator into the GithubHoster and I need to inject more dependencies, for example something that calls the GitHub API (which needs a dependency as well).

I started to implement the changes you suggested in your answer, but I'm not there yet. The only thing I did so far is replacing the Name property by an attribute.


Plus, I'll need a similar factory later again when I clone the repositories:
Bitbucket supports Git and Mercurial, for example. So for each repository, I need to decide whether to use a GitWrapper or a MercurialWrapper based on some string I get back from the API.

In my previous tool Bitbucket Backup I did this in a very simple way because it supports just one hoster, and only Git and Mercurial.
(and because it didn't have to be testable, as there are no tests...)

One reason I'm rewriting it now is to be able to easily add support for more hosters and more source control software.
That's why I'd like to auto-register classes which implement an interface and provide their own key, so I can give instructions in the docs like "add a new SourceForgeHoster and make it implement IHoster, and SCM Backup will pick it up automatically" 😃

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator

dotnetjunkie commented Jun 22, 2016

My statement about never using factories is a bit exaggerated, but it is true that you will see very few of those factories in the applications I write. I'll explain why I think factories are overused in general, and later on go into your specific case.

Do note that I'm not talking about any form of factory abstraction. The kind of factory that I think you should reconsider are abstractions that expose by themselves service abstractions as their output. So your IHosterFactory is a good example because it returns IHoster services from its Create method. So I'm not talking about factories that spit out anything else but application services.

When you consider the consumer of such factory, having a factory is hardly ever the right thing to do. Instead of lowering complexity for the consumer, a factory actually increases the amount of complexity, because instead of having a dependency on the service type (say IService), the consumer now has both a dependency on the service IService and its IServiceFactory factory. You might find this to be an insignificant increase of complexity, but you will notice right away that the amount of testing you'll have to do increases immediately. Not only should you test the interaction the consumer has with the service type, you'll have to test the interaction with the factory as well.

So in many cases, the use of a factory abstraction is not a design that keeps the consumer in mind. Or let's put it differently, according to the Dependency Inversion Principle, abstractions should be defined by their clients, and since a factory increases the number of dependencies a consumer needs to deal with, it might consider it to be in violation of the DIP.

But this not only holds for factories; I think as a general rule, we should say that:

Service abstractions should not return other service abstractions

Or if we take it even one step further, we might formulate this as

Service abstractions should not expose other service abstractions in their definition

So a service abstraction should not accept other service types as input, nor as output.

So instead of having an IServiceFactory abstraction returning IService, we have two choices:

  • Either we define a different abstraction (a facade) that forwards the call to the service and returns the result (if any) of the service,
  • Or we let the consumer use the IService abstraction itself, and have a special implementation (such as a composite, mediator, decorator) that forwards the call to the real implementation.

So the intermediate layer here will do the proper handling on behalf of the consumer; this simplifies this consumer.

Take for instance the following controller:

public class ShipmentController
{
    private readonly ICommandHandlerFactory factory;

    public void ShipOrder(ShipOrder command) {
        ICommandHandler<ShipOrder> handler = factory.Create<ShipOrder>();
        handler.Handle(command);
    }
}

Here the ShipmentController depends on the ICommandHandlerFactory and uses it to create an ICommandHandler<ShipOrder> after which it calls the Handle method on that handler to execute the command. In other words, it depends on both ICommandHandlerFactory and ICommandHandler<T>. Compare that to a solution using a facade:

public class ShipmentController
{
    private readonly ICommandDispatcher dispatcher;

    public void ShipOrder(ShipOrder command) => dispatcher.Dispatch(command);
}

This effectively reduced the complexity of this class by 50%.

The second option, is to let the consumer use the IService interface itself. This is useful in scenarios where no extra data is needed for the creation of the type. Say for instance that our ShipmentController is singleton, while command handlers must be transient. In that case we can't inject the ICommandHandler<ShipOrder> directly into the controller. But instead of injecting a factory, we can also create some kind of decorator/proxy implementation that will delay the creation of the command handler until the moment its Handle method is called. That would cause our ShipmentController to look as follows:

public class ShipmentController
{
    private readonly ICommandHandler<ShipOrder> shipOrderHandler;

    public void ShipOrder(ShipOrder command) {
        shipOrderHandler.Handle(command);
    }
}

We'll of course have to define a decorator that wraps around the original instance and allows delayed creation of that type, but the fact is that while doing this, we kept this implementation detail out of our consumer, and kept it as simple as possible.

Do note that the dispatchers, decorators, facades and proxies you define might internally still look much like the factory implementations, but instead of just creating instances, they actually act on the given service.

So far the general case. Now about your specific case.

The only consumer that I could find of your IHosterFactory is the ValidatingConfigReader. From perspective of this consumer, it makes sense define the following abstraction:

public interface IHosterValidator
{
    ValidationResult Validate(ConfigSource source);
}

It can be implemented as follows:

public sealed class HosterValidator : Dictionary<string, IHoster>, IHosterValidator
{
    public void Add(IHoster hoster)
    {
        var attribute = hoster.GetType().GetCustomAttribute<HosterAttribute>();
        this.Add(attribute.Name, hoster);
    }

    public ValidationResult Validate(ConfigSource source)
    {
        var hoster = this[source.Hoster].Validator.Validate(source);
    }
}

This allows you to simplify the ValidatingConfigReader to the following:

private readonly IHosterValidator validator;

public Config ReadConfig()
{
    // read config and do basic sanity checks

    var results = config.Sources.Select(validator.Validate).ToArray();

    LogMessages(results);

    return results.All(result => result.IsValid) ? config : null;
}

private void LogMessages(IEnumerable<ValidationResult> results)
{
    var messages =
        from result in results
        from message in result.Messages
        select message;

    foreach (var message in messages)
    {
        this.logger.Log(message.Error, message.Message);
    }
}

You might even go one step further and remove logging from this class and throw specific exceptions instead. The logging behavior can than be added on top of this, by creating a decorator and wrapping the original class in that decorator. But okay, that's a discussion for another time perhaps ;-)

Of course this merges the factory with the validator, which is obviously only possible when this validator is the only part in the system that needs to get hosters by their key. In your case you will likely have a different part of the system (that you propably already have, but which I missed) that needs to get the right hoster and use it to make a backup. This will cause you to again extract that part into its own class, and by doing that you'll again have your factory.

But there's a difference though. In case the HosterValidator is part of the composition root, so can be your new IHosterFactory abstraction, or you can remove that abstraction completely. You can for instance inject the HosterFactory implementation (now part of the Composition Root as well) or inject a Func<string, IHoster> delegate. Difference is that now there is no application wide abstraction anymore that allows application components to retrieve IHoster implementations from. Instead they use an abstraction that is tailored for their specific use.

You might find this difference superficial, and yes, for the system you are building this might not be a big deal, since most -if not all- of the code lives in one assembly anyway. Besides that, since you are creating a reusable library, it might even make sense to use the IHosterFactory as a hook for users to intercept the creation of hosters.

For bigger line of business applications, the distinction between infrastructural code as part of the composition root and the rest of the application code is quite important. It is the sole responsibility of the Compostion Root to wire everything together and it knows how to find and create components. The rest of the application should be oblivious to this.

So long story short, even in your case you can do without an IHosterFactory abstraction, although atmittedly, in your reusable library it can make sense to have this extension point for end users.

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator

dotnetjunkie commented Jun 22, 2016

That said, I like to raise something else.

Even though I'm always pleased to see people using Simple Injector, I can't help noticing that the use of a DI container might be a form of over-engineering. I think in your case, hand-wiring your components in the Composition Root will probably do just fine. Newing up components by hand is not some obscure practice, it even has a name: Pure DI. Remember, when applying Dependency Injection, DI libraries are optional tools. Although you can imagine that I dogfood Simple Injector in my applications, not all the applications I write actually use a DI library, but applying the Dependency Injection however is a mandatory practice. For reusable libraries, Mark Seemann's guidance can be very helpful. Not using a DI library also has some interesting advantages, such as having compile-time support and in your case, the application has a smaller footprint and will probably just contain of a single .exe, instead of dragging along some extra .dlls.

I hope this helps.

@christianspecht

This comment has been minimized.

Copy link
Author

christianspecht commented Jun 29, 2016

First of all, thank you very much for this very detailed answer 👍

Please note that my app is not a reusable library. It's a ready-to-use command line app.
Maybe that changes some of the things you said?

Then, you're right about this:

In your case you will likely have a different part of the system (that you propably already have, but which I missed) that needs to get the right hoster and use it to make a backup. This will cause you to again extract that part into its own class, and by doing that you'll again have your factory.

That's exactly what I intended. You didn't miss that part of the code though, it just doesn't exist yet.
(let's call it IBackupMaker for now, so I can use it in my answer and you know what I'm talking about)

So if I understood you correctly, you're suggesting the following:

  1. I implement the HosterFactory exactly like the FooFactory from your SO answer.
  2. In the other parts of the app, I never use the factory directly.
    Instead, I create IHosterValidator and IBackupMaker as shown in your first answer here, which:
    • are called from the other parts of the code
    • use the HosterFactory internally to create a hoster, and then call the hoster's Validate or MakeBackup methods
  3. In other words, IHoster doesn't change, except adding a second property for an IBackupMaker.

If that was correct until now, then apparently my HosterValidator won't have the public void Add(IHoster hoster) method from your example, because it will use the factory instead.

So the HosterValidator and the BackupMaker are using the factory via constructor injection, right?
Then I don't understand the next paragraph:

But there's a difference though. In case the HosterValidator is part of the composition root, so can be your newIHosterFactoryabstraction, or you can remove that abstraction completely. You can for instance inject theHosterFactoryimplementation (now part of the Composition Root as well) or inject aFunc<string, IHoster>delegate. Difference is that now there is no application wide abstraction anymore that allows application components to retrieveIHoster` implementations from. Instead they use an abstraction that is tailored for their specific use.

If HosterValidator and BackupMaker get an IHosterFactory via DI, then I do have an "application-wide abstraction that allows application components to retrieve IHoster implementations from" - I just don't use it, but hypothetically other classes could.

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator

dotnetjunkie commented Jun 29, 2016

It's a ready-to-use command line app. Maybe that changes some of the things you said?

I assume that it is your intention that others use it. In that case it can make sense to let it have as few files as possible (ideally just one). This makes it easier for others to use it; they can just copy the exe.

So if I understood you correctly, you're suggesting the following:

You understood correctly. You might even ditch the IHosterFactory abstraction completely.

In other words, IHoster doesn't change, except adding a second property for an IBackupMaker.

You might even want to split up IHoster in several smaller abstractions (conforming to the ISP). This allows you to add new features later on, without having to break existing plugins.

then apparently my HosterValidator won't have the public void Add(IHoster hoster) method from your example, because it will use the factory instead.

Correct. The only reason it has an Add method is because in my example, it internally contains the factory.

If HosterValidator and BackupMaker get an IHosterFactory via DI, then I do have an "application-wide abstraction that allows application components to retrieve IHoster implementations from" - I just don't use it, but hypothetically other classes could.

Yes, and as discussed, from perspective of SOLID it would be a problem if any of your application components take a dependency on IHosterFactory. So your code should most likely not have to use this abstraction, which means that you should remove it - in case you were building a Line Of Business application. In your case, your goal is for more general reuse and extendability. Now you're entering the rules of design for reusable libraries and frameworks. It wouldn't be weird to see users of your library to call the abstraction or replace the abstraction with a custom factory implementation.

@christianspecht

This comment has been minimized.

Copy link
Author

christianspecht commented Jun 29, 2016

I assume that it is your intention that others use it. In that case it can make sense to let it have as few files as possible (ideally just one). This makes it easier for others to use it; they can just copy the exe.

Okay, I misunderstood you. When I hear "reusably library", I think "DLL".
I'm not planning to split my own code into multiple assemblies, but of course the external libraries I'm using (like Simple Injector) will be separate files, so it's unlikely that my users can just copy the exe.
Maybe it's possible in .NET Core to really merge everything into one single exe, but I didn't evaluate that yet.

You might even ditch the IHosterFactory abstraction completely.
[...]
Yes, and as discussed, from perspective of SOLID it would be a problem if any of your application components take a dependency on IHosterFactory. So your code should most likely not have to use this abstraction, which means that you should remove it - in case you were building a Line Of Business application. In your case, your goal is for more general reuse and extendability. Now you're entering the rules of design for reusable libraries and frameworks. It wouldn't be weird to see users of your library to call the abstraction or replace the abstraction with a custom factory implementation.

How would I ditch the IHosterFactory abstraction?
As long as HosterValidator / BackupMaker are using it, I need to inject it.
This means that I need to register it in Simple Injector.
Of course I could ditch the interface and register the concrete class, but that's probably not what you meant...because any component could still take a dependency on the concrete class.

Even if this app is not a LOB app, my day job is building one. So I'd still like to hear the LOB solution.

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator

dotnetjunkie commented Jun 30, 2016

Okay, I misunderstood you. When I hear "reusably library", I think "DLL".

A .NET .EXE file is as reusable as a .DLL is. You can reference a .EXE file in your application as start using its public types.

Maybe it's possible in .NET Core to really merge everything into one single exe, but I didn't evaluate that yet.

This is possible with your plain old .NET as well using ILMerge, but not sure that this is worth it. It can be hard to remove the need of external libraries, without having to reinvent the weel, but again, in your case I'm not sure that a DI container actually makes things easier.

How would I ditch the IHosterFactory abstraction?

Ditching the IHosterFactory abstraction means that both the HosterFactory implementation, the HosterValidator implementation and the BackupMaker implementation should become part of the composition root (while their abstractions stay accessible by application code). When your HosterValidator is part of the composition root, it can depend on HosterFactory directly, instead of depending on IHosterFactory. Or you let HosterValidator depend on a Func<string, IHoster> instead. By moving it all into the composition root, you remove the need for the application to know anything about how hosters are created. Of course this probably makes little sense for your 'single layered' tool. For a LOB this can be different.

It is a common practice to have some specific logic inside your composition root. This is much like the strategy pattern, where you can change behavior based on the application's specific needs. An IUserContext implementation for a web application will likely be different from an implementation for your windows service.

Of course I could ditch the interface and register the concrete class, but that's probably not what you meant..

This is exactly what I meant. The reason we have those abstractions is to allow components to keep decoupled, and allow them to be tested independently. For the infrastructure code that is part of your composition root, both issues typically don't exist. You are testing them when triggering the complete pipeline in an integration test and you probably change them together anyway.

This doesn't mean though that the factory should be registered in your container. The following code will probably do just fine:

var factory = new HosterFactory();
container.RegisterSingleton<IHosterValidator>(new HosterValidator(factory));
container.RegisterSingleton<IBackupMaker>(new BackupMaker(factory));

Where both HosterFactory, HosterValidator, and BackupMaker could be private nested class in the bootstrap logic:

private sealed class HosterFactory { }
private sealed class HosterValidator : IHosterValidator { }
private sealed class BackupMaker : IBackupMaker { }

Or letting HosterValidator and BackupMaker depend on a Func<string, IHoster> might work just fine:

private static IHoster CreateHoster(string name) { ... }

container.RegisterSingleton<IHosterValidator>(new HosterValidator(CreateHoster));
container.RegisterSingleton<IBackupMaker>(new BackupMaker(CreateHoster));
@christianspecht

This comment has been minimized.

Copy link
Author

christianspecht commented Jul 15, 2016

Ditching the IHosterFactory abstraction means that both the HosterFactory implementation, the HosterValidator implementation and the BackupMaker implementation should become part of the composition root (while their abstractions stay accessible by application code). When your HosterValidator is part of the composition root, it can depend on HosterFactory directly, instead of depending on IHosterFactory. Or you let HosterValidator depend on a Func<string, IHoster> instead. By moving it all into the composition root, you remove the need for the application to know anything about how hosters are created. Of course this probably makes little sense for your 'single layered' tool. For a LOB this can be different.

By "moving into the composition root" you mean that in a large project, the Bootstrapper would be in its own project and that I would move the HosterValidator and BackupMaker implementations into this project? OK, makes sense.

Like you already said - my project is too small to create a separate project for the composition root.
But I liked the idea of a clearly visible separation, so at least I created a separate folder for it.


Besides that, I implemented all the changes you suggested and I think I'm almost there.
However, ditching the IHosterFactory interface introduced a new problem I don't know how to solve:
I'm not able to test the HosterValidator anymore.

Here's my current state.
In my last commit I finally changed the HosterFactory similar to what you suggested in your StackOverflow answer.

That change broke most of my integration tests.
I already fixed the HosterFactoryTests in the same commit, but I don't know what to do with the HosterValidatorTests:

I didn't commit changes to this class yet, so the one in the repository doesn't even compile right now.
At the moment, the working copy on my machine looks like this:

public class HosterValidatorTests
{
    [Fact]
    public void ValidateCallsUnderlyingValidator()
    {
        var reader = new FakeConfigReader();
        reader.SetDefaultFakeConfig();
        var config = reader.ReadConfig();
        var source = config.Sources.First();

        var factory = new HosterFactory(new SimpleInjector.Container());
        factory.Register<FakeHoster>();

        var sut = new HosterValidator(factory);
        sut.Validate(source);

        Assert.True(hoster.FakeValidator.WasValidated); // this line doesn't compile
    }
}

The problem is that with the changes to the factory, I'm not able anymore to access the IHoster instance created by the factory from the outside.
So I'm not able to test whether the hoster's validator was actually called.

The only solution that comes to my mind is to re-introduce the IHosterFactory interface I ditched three commits ago.
Then, I can create a FakeHosterFactory to the HosterValidator, where I can access the IHoster created by the factory.

Are there any better solutions I don't see?
Would you test the HosterValidator at all?

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator

dotnetjunkie commented Jul 15, 2016

By "moving into the composition root" you mean that in a large project, the Bootstrapper would be in its own project and that I would move the HosterValidator and BackupMaker implementations into this project?

No, not exactly. The composition root is a layer, just like your presentation layer and business layer. A layer is a logical artifact, while a project/assembly is a physical artifact. You can happily have multiple layers in one single assembly. As a matter of fact, it is really common to have your composition root (layer) and presentation layer exist in the same assembly. However, you only want the composition root to reference the presentation layer, but not the other way around. For more information, take a look at this answer.

so at least I created a separate folder for it.

That's all you'll usually have to do, even in big applications.

I'm not able to test the HosterValidator anymore.

Classes that are part of your composition root are infrastructural components. Those components can still be tested (and probably should). You tests can reference those components, they don't have to be private nested types. If you make those classes public (remember, a composition root is just a layer), there should be no problem in accessing them from unit tests and verifying their behavior. Or is there something that I'm missing why this doesn't work?

@christianspecht

This comment has been minimized.

Copy link
Author

christianspecht commented Jul 16, 2016

Classes that are part of your composition root are infrastructural components. Those components can still be tested (and probably should). You tests can reference those components, they don't have to be private nested types. If you make those classes public (remember, a composition root is just a layer), there should be no problem in accessing them from unit tests and verifying their behavior. Or is there something that I'm missing why this doesn't work?

I explained it in my last answer, right under "I'm not able to test the HosterValidator anymore.".

TL;DR:
Now that the IHosterFactory interface is gone, I have no choice but to test the HosterValidator by injecting the real, actual HosterFactory.

To test the HosterValidator, I would need to look at the IHoster created by the HosterFactory, and check if its Validate method was actually called.

But there's no way to get that IHoster instance from outside the HosterValidator.
If the IHosterFactory interface was still there, I'd be able to inject a FakeHosterFactory that lets me access the created IHoster instance from the outside.

So the only solution I see is to re-introduce the IHosterFactory interface again.

@dotnetjunkie

This comment has been minimized.

Copy link
Collaborator

dotnetjunkie commented Jul 16, 2016

So the only solution I see is to re-introduce the IHosterFactory interface again.

In that case, yes, it can be beneficial to introduce an abstraction for testing. Do note, that still this abstraction can be part of the composition root; the rest of the application doesn't depend on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment