views:

116

answers:

4
public interface ITaskProvider
{
    T GetTask<T>();
}

In the implementation of ITaskprovider below, as you see the IUserTask and IIdentityTask is being injected from property instead of constructor. The reason is that Windsor automatically instantiates the injected properties on runtime when accessed so that i dont have to put all the must injected dependencies in the constructor.

public class TaskProvider : ITaskProvider
    {
        public IUserTasks UserTasks { get; set; }

        public IIdentityTasks IdentityTasks { get; set; }

        public T GetTask<T>()
        {
            Type type = typeof(T);
            if (type == typeof(IUserTasks)) return (T)this.UserTasks;
            if (type == typeof(IIdentityTasks)) return (T)this.IdentityTasks;

            return default(T);
        }
    }

In the controller I am injecting the ITaskProvider in the constructor.

public ITaskProvider TaskProvider { get; set; }

public AuctionsController(ITaskProvider taskProvider)
        {
            TaskProvider = taskProvider;
        }

And here i call the taskprovider and its methods fine.

public ActionResult Index()
{
 var userTasks = TaskProvider.GetTask<IUserTasks>();
 var user = userTasks.FindbyId(guid);

}

Up to here, everything works fine.

I have been told that this is more like a service locator pattern and is violating dependency injection pattern and i want to know what is violating here.

+1  A: 

If the controller requires an IUserTasks instance, then it would be simpler if it received one from the container directly. Essentially, the TaskProvider is just a wrapper around the container anyway, since that is where it gets the UserTasks and IdentityTasks instances from.

Lee
Yes that is just a wrapper. Is it still violating the dependency injection pattern?
Murat
@Murat - It's not really a violation if it is injected into its dependents, it just doesn't provide any value.
Lee
+1  A: 

You are using Dependency Injection to inject what is effectively the "Service Locator" into the controller rather than injecting an implementation of IUserTasks and IIdentityTasks.

Your actual controller really has dependencies on IUserTasks and IIdentityTasks but you are not directly injecting those into your controller instead deciding to use the "Service Locator" or Task Provider and therefore you have injected a dependency on the service locator which in the example seems to provide nothing that could not be done by just injecting the true dependencies directly.

Ian Johnson
+1  A: 

You should inject IUserTask and IIdentityTask into the controller's constructor, as there is no gain in using the TaskProvider. Besides that, in the way you did it, you miss some compile time checks. For example, you could call TaskProvider.GetTask() and wait to explode at runtime. You should, at least, put some constraint on that generic parameter (if it's possible to both interfaces inherit from common parent).

Regarding the "violation", you should note that you are not injecting the dependencies into the controller. You're providing a way to retrieve them.

Fernando
+2  A: 

For me, There is no violation against DI in your code, regarding wikipedia:

core principal to separate behavior from dependency resolution

But the bad side your controller has too much knowledge, in some cases (if you don't programs carefully) you may violate the Law Of Demeter

take a look at your code:

public ActionResult Index()
{
 var userTasks = TaskProvider.GetTask<IUserTasks>();
 var user = userTasks.FindbyId(guid);
}
ktutnik
Thanks for this useful stuff.
Murat