simpleinjector / SimpleInjector

An easy, flexible, and fast Dependency Injection library that promotes best practice to steer developers towards the pit of success.
https://simpleinjector.org
MIT License
1.21k stars 154 forks source link

Simplify registering types that contain configuration values in their constructor #246

Open dotnetjunkie opened 8 years ago

dotnetjunkie commented 8 years ago

When dealing with types that mix primitive types and dependencies in their constructor, there are currently three ways to deal with this; they are:

All three options have their downsides though:

A feature should be added that simplifies these use cases, without causing one of the downsides that the current options have.

dotnetjunkie commented 7 years ago

Here is a list of use cases with a suggested WithConstructorArgument method that can be used to apply the constructor argument.

// Use cases

// Use case 1: Simple
container.Register<IRepo, MyRepo>().WithConstructorArgument("constr");

// Use case 2: Named argument
container.Register<IRepo, MyRepo>().WithConstructorArgument("parameterName", "value");

// Use case 3: Generic type
container.Register(typeof(IRepo<>), typeof(MyRepo<>))
    .WithConstructorArgument("value");

// Use case 4: Decorator
container.RegisterDecorator(typeof(IX<>), typeof(LoggingXDecorator<>))
    .WithConstructorArgument("value");

// Use case 5: Registration
Lifestyle.Transient.CreateRegistration<MyRepo>(container)
    .WithConstructorArgument("value");

// Use case 6: InstanceProducer
Lifestyle.Transient.CreateProducer<IRepo, MyRepo>(container)
    .WithConstructorArgument("value");

// Use case 7: Multiple registrations
var repo1 = Lifestyle.Singleton.CreateRegistration<MyRepo>(container)
    .WithConstructorArgument("connectionString1");

// NOTE: This is problematic, because CreateRegistration caches registrations.
var repo2 = Lifestyle.Singleton.CreateRegistration<MyRepo>(container)
    .WithConstructorArgument("connectionString2");

container.RegisterConditional(typeof(IRepo), repo1, c => c.Consumer.Namespace.Contains("Module1"));
container.RegisterConditional(typeof(IRepo), repo2, c => c.Consumer.Namespace.Contains("Module2"));
dotnetjunkie commented 7 years ago

@qujck , @TheBigRic, any feedback on this feature?

TheBigRic commented 7 years ago

I can see the downside of the three current possible solutions. I think however you forgot one simple solution, which is the solution I always use, which is creating a fit-for-purpose DTO which contains the primitive type(s). I register these DTO's as Singleton. My ctor's never contain any primitive types. I think of this way as the solution.

Third party components are covered by using a proxy or adapter implementations.

But if we want or must be able to support this feature I think the proposed solution .WithConstructorArgument seems usable and definitively makes the code readable.

What I don't see in the given examples is how this will really work. Which value will the container supply when we call WithConstructorArgument("value"). Does this mean the constructor has a single ctor parameter of type string, which will be supplied with "value". What if there are other primitive types in the ctor?

What about this?

container.RegisterPrimitiveType("MyConnectionString", "data source=...");
container.RegisterPrimitiveType("MyServiceTimeout", 100);

// void WithConstructorArgument(string parameterName, string primitiveDictionaryKey)
container.Register<IRepo, MyRepo>().WithConstructorArgument("constr", "MyConnectionString");

It would be far nicer if we could supply the user with a possibility (e.g. container.SetPrimitiveTypeDictionary())in which he would supply his own Dictionary<SomeEnumType, valueType> filled with primitive type values and will get compile time support (intellisense showing his own SomeEnumType) when he makes a call to .WithConstructorArgument(). My open generic knowledge isn't quite good enough to design this, but I think this will be next to impossible.

I would nevertheless make an entry point for registering the primitive types because as far as I can see this will give us the opportunity to do diagnostics if primitive types are missing with the nice exception messages we are all accustomed to. Registering primitive types makes the process consistent with the complete API and will make it possible to reuse primitive type values because you can supply a self determined key to the WithConstructorArgument method.

dotnetjunkie commented 7 years ago

What if there are other primitive types in the ctor?

Every part of Simple Injector's API must follow the same design principles, which also means it has to fail fast. So the call to WithConstructorArgument should preferably check immediately whether or not there is exactly one constructor argument for that type. In case there is none, or more than one, Simple Injector must throw an exception. It should never inject both arguments with the same value.

In case of there are multiple arguments of the same type, that's when the overload comes into play that accepts the parameter name (it's in my first example):

.WithConstructorArgument("parameterName", "value");

I expect the definition of this method to be:

.WithConstructorArgument<T>(string argumentName, T value);

It would be far nicer if we could supply the user with a possibility (e.g. container.SetPrimitiveTypeDictionary())in which he would supply his own Dictionary<SomeEnumType, valueType> filled with primitive type values and will get compile time support (intellisense showing his own SomeEnumType)

I'm not sure I completely understand the use case, but it sounds a bit like a far fetched solution. What problem does that try to solve?

this will give us the opportunity to do diagnostics if primitive types are missing with the nice exception messages we are all accustomed to.

Absolutely. Simple Injector should guide the user using clear exception messages.

dotnetjunkie commented 7 years ago

Moved to backlog. Will not add this to v4.

TheBigRic commented 7 years ago

Good decision!

lessismore1 commented 6 years ago

Any news about this feature?

dotnetjunkie commented 6 years ago

This feature requires a major API change which is something we can only implement in a next major release. So for now, the status pf this feature is unchanged. Don't expect any related changes in this area soon.

Do note that the solution described by @TheBigRic above (using DTOs) is the typically your preferred solution and solves the problem elegantly.

Since the DTO solution is in most cases the most elegant solution, this feature is not high on my priority list.

gitcob commented 6 years ago

I hope this feature does make it into the next major release. I'm currently trying to introduce DI at my company where that hasn't been on anyone's radar so far. Since I'm dealing with a big legacy project, any feature that makes refactoring less painful is very welcome and would help answer questions like "is this really worth the overhead?".

Having just read your (fantastic) book on DI, the DTO solution feels like an unnecessary restriction, much like a service locator is an unnecessary dependency. I can definitely see the value of putting your volatile dependencies into the constructor and creating them somewhere else. But I'm not going to convince anyone new to DI that they should also bundle up all their primitive constructor parameters into an extra class, especially when they are already out of their comfort zone writing more interfaces and wrestling with the idea of a "composition root".

dotnetjunkie commented 6 years ago

Hi @gitcob,

Great to hear you've been reading the book by Mark and I (for as far as it's finished of course). I hope it's doing you and your organization a lot of good. You might be in for a treat when you read chapter 10, which will hopefully be added to the MEAP content by Manning soon. In that chapter, we describe that the number of classes a system has by itself is a bad metric for measuring maintainability. Many developers think that more classes means more code to maintain, resulting in a less maintainable system. This premise is false though, and increasing the number of classes can actually have an enormous positive impact on the maintainability of a software system—but, it has to be done according to certain rules. Chapter 10 will go into more details on that subject.

So from that perspective, you shouldn't worry about creating new classes by grouping configuration values together—a practice which is a common refactoring pattern called the Parameter Object refactoring.

From my own experience, I do know that it can be daunting to persuade developers into DI, especially if that's the opposite they being working for years. It takes a whole book to explain why DI is better than strongly coupled code. So I do respect the effort you are taking. It’s not easy.

On the other hand, I hope you can understand my stance. I cannot introduce features just for the sake of making it easier for you to persuade your colleagues. If that's what you are looking for, Simple Injector might actually not be the best place to start, because from the very start of this project, the other contributors and I have been very strict with the features we wish to add to Simple Injector. We always ask ourselves: does feature X promote best-practices.

That’s not said that the feature in question is a bad thing per-see, it’s simply that I don’t think the feature to be that important, because there are workarounds available, and simple design changes solve the problem as well.

But as stated above, there are several ways to register classes with primitive dependencies, and that's not limited to extracting them to DTOs. Extracting to DTOs however, would be our advised way of working (and not surprisingly, you'll see Microsoft give similar advice with their new ASP.NET Core framework). But as stated at the start of this post, there are several ways to skin a cat so you're absolutely not forced into the DTO approach. Fot instance, the following example shows how to mix configuration values with dependencies:

container.Register<IMessageSender>(() => new MailSender(
    mailServerUrl,
    container.GetInstance<ILogger>()));
gitcob commented 6 years ago

Hi @dotnetjunkie,

thanks for the detailed reply. I'll try initializers at work and the parameter object route at home. And I'll be closely watching the MEAP updates!