14

I'm trying to remove a Service Locator from an abstract base class, but I'm not sure what to replace it with. Here is a psuedo-example of what I've got:

public abstract class MyController : Controller
{
    protected IKernel kernel;
    public MyController(IKernel kernel) { this.kernel = kernel); }

    protected void DoActions(Type[] types)
    {

        MySpecialResolver resolver = new MySpecialResolver(kernel);
        foreach(var type in types)
        {
            IMyServiceInterface instance = resolver.Get(type);
            instance.DoAction();
        }
    }
}

The problem with this is that the instanciator of a derived class doesn't know what bindings the kernel must have in order to keep MySpecialResolver from throwing an exception.

This might be intrinsicly intractable because I don't know from here which types I'll have to resolve. The derived classes are responsible for creating the types parameter, but they aren't hardcoded anywhere. (The types are based on the presence of attributes deep within the derived class's composition hierarchy.)

I've trying to fix this with lazy loading delegates, but so far I haven't come up with a clean solution.

Update

There are really two issues here, one is that the IoC container is passed to the controller, acting as a service locator. This is easy to remove--you can move the location up or down the call stack using all sorts of techniques.

The second issue is the difficult one, how can you ensure that the controller has the necessary services when the requirements aren't exposed until runtime. It should have been obvious from the start: you can't! You will always be dependent upon either the state of the service locator or contents of a collection. In this particular case no amount of fiddling will ever resolve the problem described in this article with staticly typed dependencies. I think that what I'm going to end up doing is passing a Lazy array into the controller constructor and throwing an exception if a required dependency is missing.

15
  • 4
    Um, I realize this isn't the trendy answer, but Service Locator is not an anti-pattern. It is an alternative to the use of an IoC container, but you can use dependency injection with either one (it's a bit weird to inject dependencies into service-located classes, but not impossible). That said, if you're using an IoC container, then using a Service Locator as well would probably qualify as a code smell. Jul 27, 2011 at 5:48
  • It looks like you're using mvc and possibly Windsor but I'm not certain - if so you can use commonservicelocator.codeplex.com and then somewhere in Application_Start call ServiceLocator.SetLocatorProvider(() => { return new WindsorServiceLocator(_container); });DependencyResolver.SetResolver(ServiceLocator.Current); Then this will allow you to inject your dependencies into your controller constructors. Other containers are supported but I've only used Windsor for this
    – Adam
    Jul 27, 2011 at 7:41
  • Paul, can you add an example of the derived class attribute dependency? Are the attributes on the derived controller or the derived controller's dependencies? Jul 27, 2011 at 13:24
  • @Richard, these are based on the derived controllers properties--located in another assembly. Thus while the required dependancies are technically static at build time, the net effect is that they are not known to the IoC container or unit test until the controller has been instantiated.
    – Paul
    Jul 28, 2011 at 14:35
  • 1
    Even though it's an "Anti Pattern", I've found it useful passing in the Kernel interface, especially when it comes to testing and being able to use Ninject.Mock and pass in a Mock version of the Kernel.
    – Makotosan
    Feb 28, 2012 at 20:59

3 Answers 3

4

I agree with @chrisichris and @Mark Seemann.

Ditch the kernel from the controller. I'd switch your resolver composition a little bit so that your controller can remove the dependency on the IoC container and allow the resolver to be the only item that worries about the IoC container.

Then I would let the resolver get passed into the constructor of the controller. This will allow your controller to be far more testable.

For example:

public interface IMyServiceResolver
{
    List<IMyServiceInterface> Resolve(Type[] types);
}

public class NinjectMyServiceResolver : IMyServiceResolver
{
    private IKernal container = null;

    public NinjectMyServiceResolver(IKernal container)
    {
        this.container = container;
    }

    public List<IMyServiceInterface> Resolve(Type[] types)
    {
        List<IMyServiceInterface> services = new List<IMyServiceInterface>();

        foreach(var type in types)
        {
            IMyServiceInterface instance = container.Get(type);
            services.Add(instance);
        }

        return services;
    }
}

public abstract class MyController : Controller
{
    private IMyServiceResolver resolver = null;

    public MyController(IMyServiceResolver resolver) 
    { 
        this.resolver = resolver;
    }

    protected void DoActions(Type[] types)
    {
        var services = resolver.Resolve(types);

        foreach(var service in services)
        {
            service.DoAction();
        }
    }
}

Now your controller isn't coupled to a specific IoC container. Also your controller is much more testable since you can mock the resolvers and not require an IoC container at all for your tests.

Alternatively, if you don't get to control when a controller is instantiated, you can modify it slightly:

public abstract class MyController : Controller
{
    private static IMyServiceResolver resolver = null;

    public static InitializeResolver(IMyServiceResolver resolver)
    {
        MyController.resolver = resolver;
    }

    public MyController() 
    { 
        // Now we support a default constructor
        // since maybe someone else is instantiating this type
        // that we don't control.
    }

    protected void DoActions(Type[] types)
    {
        var services = resolver.Resolve(types);

        foreach(var service in services)
        {
            service.DoAction();
        }
    }
}

You would then call this at your application start up to initialize the resolver:

MyController.InitializeResolver(new NinjectMyServiceResolver(kernal));

We did this to handle elements created in XAML who require dependencies resolved but we wanted to remove Service Locator like requests.

Please excuse any syntactical errors :)

I'm writing a blog post series on the topic of refactoring an MVVM application with Service Locator calls in the view models you might find interesting. Part 2 is coming soon :)

http://kellabyte.com/2011/07/24/refactoring-to-improve-maintainability-and-blendability-using-ioc-part-1-view-models/

5
  • In my opinion, replacing a static service locator with an injected service locator (or resolver) is not solving the issue at hand. It still makes the design brittle, because I have to know what dependencies to register with the resolver to test my class. And as in the presented question, the derived class still does not know what Kernal dependency is needed. Jul 27, 2011 at 13:22
  • 1
    No problem, I am posting my suggestion. (Would have liked more info first, tho.) The issue at hand is that an IOC container is being passed to a class. The class is then responsible for more than it should be, which is what we are trying to fix with DI/IOC. Jul 28, 2011 at 14:21
  • @codeprogression because I have to know what dependencies to register with the resolver to test my class this is false. You do not need to test this. You test that the class invokes the service locator, you do not test the services assuming you're testing MyController. Each individual service should have it's own stand alone test. This is one of the greatest benefits to proper service location. You have extremely testable services that you can compose and sequence to do awesome things. To test the composition do a full stack test otherwise it's meaningless. Oct 21, 2014 at 14:35
  • Failure to test composition of services in a full stack scenario results in false expectations. So you stand up a MyController in your integration test, you configure the locator to return X, Y, Z. Now all the tests are passing and you have a false sense of security. A developer goes in and changes the service registration code to return A, B, C. Your integration test is passing, but your site is crashing. Oct 21, 2014 at 14:37
  • I like your solution but I also don't like your solution. When you think about it, it is just castrated service locator (resolver) that you injecting into constructor.
    – Ondřej
    Aug 31, 2018 at 11:21
4

Maybe you should just do away the Kernel, Types and MySpecialResolver and let the subclasses call DoActions with the IMyServiceInterface instances they need as argument directly. And let the subclasses decide how they get to these instances - they should know best (or in case they don't know which exactly the one who ever decides which instances of IMyServiceInterface are needed)

2
  • 1
    +1 It seems like the types array is the key. Get rid of that and the rest should be straight forward. Jul 26, 2011 at 18:29
  • @MarkSeemann so basically throw away all dynamicism and go for a tightly coupled brittle solution. Awesome. Who needs polymorphism when we can just write everything in 1 giant method? Oct 21, 2014 at 14:29
0

I would have liked to have a bit more information before posting this answer, but Kelly put me on the spot. :) Telling me to put my code where my mouth is, so to speak.

Like I said in my comment to Kelly, I disagree with moving the resolver/locator from a static implementation to an injected implementation. I agree with ChrisChris that the dependencies the derived type needs should be resolved in that class and not delegated to the base class.

That said, here is how I would remove the service location...

Create Command Interface

First of all I would create a command interface for the specific implementation. In this case the types sent with the DoActions method are generated from attributes, so I would create an IAttributeCommand. I am adding a Matches method to the command in order to declare the command for use by certain types.

public interface IAttributeCommand
{
    bool Matches(Type type);
    void Execute();
}

Add Command Implementations

To implement the interface, I pass in the specific dependencies I need to execute my command (to be resolved by my container). I add a predicate to my Matches method, and define my Execute behavior.

public class MyTypeAttributeCommand : IAttributeCommand
{
    MyDependency dependency;
            SomeOtherDependency otherDependency;

    public MyTypeAttributeCommand (MyDependency dependency, ISomeOtherDependency otherDependency)
    {
        this.dependency = dependency;
                    this.otherDependency = otherDependency
    }

    public bool Matches(Type type)
    {
        return type==typeof(MyType)
    }
    public void Execute()
    {
        // do action using dependency/dependencies
    }
}

Register Commands with Container

In StructureMap (use your favorite container), I would register the array like so:

Scan(s=>
       {
                s.AssembliesFromApplicationBaseDirectory();
                s.AddAllTypesOf<IAttributeCommand>();
                s.WithDefaultConventions();
       } 

Select and Execute Commands Based on Type

Finally, on the base class, I define an IAttributeCommand array in my constructor arguments to be injected by the IOC container. When the derived type passes in the types array, I will execute the correct command based on the predicate.

public abstract class MyController : Controller
{
    protected IAttributeCommand[] commands;

    public MyController(IAttributeCommand[] commands) { this.commands = commands); }

    protected void DoActions(Type[] types)
    {
        foreach(var type in types)
        {
            var command = commands.FirstOrDefault(x=>x.Matches(type));
            if (command==null) continue;

            command.Execute();
        }
    }
}

If you multiple commands can handle one type, you can change the implementation: commands.Where(x=>x.Matches(type)).ToList().ForEach(Execute);

The effect is the same, but there is a subtle difference in how the class is constructed. The class has no coupling to an IOC container and there is no service location. The implementation is more testable as the class can be constructed with its real dependencies, with no need to wire up a container/resolver.

3
  • 1
    But this is just a primative service locator. Under the skin of any IoC container is something similar to what you've written here. The array of IAttributeCommand doesn't resolve a type itself, but the concept is the same.
    – Paul
    Jul 28, 2011 at 17:42
  • Does this mean you are not using an IOC container at all? If so, you won't have much luck removing the service location code. Jul 28, 2011 at 18:31
  • 1
    When you have service location DoActions(Type[] types) there is by definition service location. Who wrangles the bits is a rather insignificant point. Service location applied well results in incredibly dynamic and extensible systems. Service location applied poorly results in incredibly fragile and inextensible systems. This is the fundamental basis of software architecture. Solving the correct problems results in success, solving the wrong problems results in a steaming pile of... Oct 21, 2014 at 14:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.