Dependency injection (ninject) using strings, anti-pattern?

Question

I have some code that is using ninject to inject dependencies, these dependencies are actual strings. Is this an anti-pattern to inject strings rather than creating a new object for example.

I.e. I wanted to inject Username and Password, would it actually be better to create a small class called credentials with 2 properies of Usernamd and Password and inject this ?

Injecting strings into constructors can be done via

kernel.Bind<IUser>().To<User>()
      .WithConstructorArgument(@"username", configuration.Username)
      .WithConstructorArgument(@"password", configuration.Password);

Is this code smell ?

Any ideas or improvement on what I am doing ?

Mikol

Well using a verbatim string literal for no reason is unsightly for one thing :) – Lobate

Thanks Jon, totally correct! So ideally it would be better to create a small class with 2 properties, create an interface and inject the implementation into the interface on the constructor ? – Mikol

I was only commenting on the verbatim part of it (the @ prefix). I only use verbatim string literals if I want to use backslashes or multiple lines. – Lobate

My take on this is that injecting primitives is fine, unless that same value has to be injected into multiple components, because in that case you are missing an abstraction. – Kristopher

Answer 1

I would prefer to use ToMethod() here:

kernel.Bind<IUser>()
      .ToMethod(ctx => new User(configuration.Username, configuration.Password));

If the User constructor has other dependencies, then I would defer to @jgauffin's answer.

You could still use ToMethod() with Kernel:

kernel.Bind<IUser>()
      .ToMethod(ctx => new User(configuration.Username,
                                configuration.Password,
                                ctx.Kernel.Get<Foo>()));

Poulin

I agree, using a registration that uses a factory delegate is much less fragile than using WithConstructorArgument. – Kristopher

Damb! I didn't realize that existed! Thanks. – Mikol

Answer 2

Is this code smell ?

Yes. Either create a ConfigurationRepository or create a factory/builder (two different design patterns) which creates the different services and then register that factory/builder in the container instead.

I got an issue with this code too:

kernel.Bind<IUser>().To<User>()
      .WithConstructorArgument(@"username", configuration.Username)
      .WithConstructorArgument(@"password", configuration.Password);

A IoC container is primarly not used to create domain entities but to create services/repositories/controllers etc. i.e. to create the objects which control the flow in your application.

Substation

Could you elaborate a little ? ConfigurationRepository, you mean a class that holds a few properties so that i can pass this in part of the constructor? And a factory/builder pattern, i presume nothing exists in ninject at the moment to support this ? Thanks once again – Mikol

-1, dependencies are dependencies, whether complex or primitive. They should be managed consistently. Adding a ConfigRepo is totally unnecessary complexity. I do, however, agree that entities should not be injected. – Darren

Answer 3

I go to great lengths trying to avoid injecting primitive types.

Looking at the code you posted, my first instinct would be to create an "IConfigurationService" and inject that as required. That service would contain properties for username and password.

Humphreys

twisting your code in unnatural ways simply to satisfy the limitations of a particular container is the real code smell. Say you want to inject a Foo initialized as Foo("a", "b", "c") in multiple places with different arguments for each parameter. They may not even be constant across invocations. You want to inject an instance, then initialize. OOPS! Your injected service probably needs a code change (assuming you own the code) to support that. The DI benefits far out weigh this problem but it's definitely a problem. – Crossway

Answer 4

Seems there are two questions here:

1. Is it a code-smell/anti-pattern to inject primitives?
2. Should some kind of type be created to compose username and password strings?

These are totally separate issues. First, a dependency is a dependency. It can be complex or primitive, but management of dependencies should be consistent. If you are using an IoC Container, it is absolutely not a code-smell to inject primitives. The IoC Container and Composition Root are privileged parts of the code that understand the needs of all services and how to satisfy them. Conceptually, this holds for complex and primitive types. Practically however, there are different mechanisms for registering complex dependencies vs primitive dependencies. The argument name approach you listed is totally valid. Other approaches depend on argument index. Choosing between the two is splitting hairs, but comes down to whether you want freedom to rename constructor arguments or re-order constructor arguments without altering wire-up code.

The alternative is to have concrete dependencies (or worse, Service Locator pattern), which is a slippery slope to an indecipherable ball of mud.

Second, whether the two strings should be composed into a type depends on how often those values are used together and how DRY you want to be. At some point, there are diminishing returns to pursuing DRYness. Also, if the values are always injected together, you might consider simply refactoring the Ninject configuration calls. Whatever you choose, make sure the rationale is consistent across the codebase.

Last point, using IoC Containers to manage entities is considered a code smell. Entities typically are responsible for maintaining domain invariants while performing domain actions. (Not sure what the context/intention of the example code snippet is so can't provide an alternative.)

Darren

Answer 5

This is a more general answer but instead of injecting a string directly, it is generally a better idea to create an interface, a provider, and inject the provider itself. So in the future, if you need to change the way how you read and create the string, it would be easier. (consider you are reading it from app.config today but then you decide to store it in DB, or read it from the registry or pass it from a web request.) A sample application:

 public interface IMachineIdProvider
    {
//Assume we are returning a machine Id.
        String ResolveMachineId();
    }

And implementation. Assume we are reading it from webconfig

 public class MachineIdProvider : IMachineIdProvider
    {
        private string _machineId;
        public string ResolveMachineId()
        {
            if (String.IsNullOrEmpty(_machineId))
            {
                this._machineId=  ConfigurationManager.AppSettings["MachineId"];
            }

            return this._machineId;
        }
    }

And inject it that way:

kernel.Bind<IMachineIdProvider>().To<MachineIdProvider>().InSingletonScope();

Declare class with the interface

public class MyWonderfulController  : BaseController
{
    private IMachineIdProvider _machineIdProvider;

    public MyWonderfulController(IMachineIdProvider machineIdProvider)
    {
       this._machineIdProvider = machineIdProvider;
    }

}

So, wherever you need to use it, you can call

var id = _machineIdProvider.ResolveMachineId()

In this way, if you change the way that you obtain the string, you only change the method itself or create another one to inject.

Aron

Answer 6

Consider an assembly devoted to holding all your interfaces for the injection. In my current working solution (prism, using MEF), I have a class that declares constant strings for the use you specified.

Why would you use constants over literals? Because it is a symbol; it is refactorable, discoverable and the actual content of the string does not matter. I always use a GUID as a suffix on my strings so that I can "guarentee" uniqueness. A constant is also avaliable for use in attribute decorations.

/// <summary>
/// Exposes top level application region names for use when registering views.
/// </summary>
public static class Regions
{
    public const string WORKSPACE_REGION = "WorkspaceRegion {4CCDA460-D1A8-4BCE-863A-593021079F02}"; //names include a GUID to prevent clashes.

    public const string TOOL_DIAGRAM_REGION = "ToolDiagramRegigon {AD3CED71-C49D-4BD8-86FF-57E5F35116D3}";

    public const string STATUS_REGION = "StatusRegion {4CEF7A12-1E92-4EED-BD46-F70F07E27662}";

    public const string TOOLS_REGION = "ToolsRegion {3C6F99B2-6414-4E06-ACC5-7445944FFA29}";

    public const string HARDWARE_INTERFACE_REGION = "HardwareInterfaceRegion {4F16ECD1-D3F5-4BE2-BB00-DD148BAE8A83}";
}

Note the spelling mistake in TOOL_DIAGRAM_REGION. It does not matter that I messed up, because no developer should ever need to type it again. I've only noticed this error because I scanned the strings while pasting it in here.

Belk

Thanks, i understand what you are saying and i agree but this wouldn't apply to my question - would it? Maybe I am missing something ? The question was about injecting strings..... Const String is a string... What i was trying to find out is if standard string injection is good or bad. Sorry maybe i misunderstood your answer? – Mikol