Thursday, May 15, 2014

Service Locators and the Interface Segregation Principle

Okay, it has been a long time since I've posted here, and boy have I learned a lot in the years since my last post! A co-worker has been encouraging me to start writing blog posts about programming, and in a conversation on Google Plus, Mark Seemann suggested that I should put some of my thoughts in a blog post, so here goes:

Mark Seemann recently blogged (again) about the Service Locator anti-pattern. He tried to describe why it's an antipattern in terms of the Interface Segregation Principle:

From a client's perspective, there no limit to how many variations of the Create method it can invoke:
var foo = serviceLocator.Create<IFoo>();
var bar = serviceLocator.Create<IBar>();
var baz = serviceLocator.Create<IBaz>();
// etc.Again, from the client's perspective, that's equivalent to multiple method definitions like:
IFoo CreateFoo();
IBar CreateBar();
IBaz CreateBaz();
// etc.
However, the client can keep coming up with new types to request, so effectively, the number of Create methods is infinite!

This argument goes part-way there, but needs a little more fleshing out. After all, you could say that any method with parameters can be expanded out in this way:

var x = service.Foo(1);
var y = service.Foo(2);
var z = service.Foo(3);
// etc.

corresponds to

var x = service.Foo1();
var y = service.Foo2();
var z = service.Foo3();
// etc.



The key difference is that programmers almost never write code like what's in the example above: what's up with those magic numbers? We generally say `var a = Foo(x)`, where `x` is somehow derived from the method's input. That means you can write a meaningful unit test that ensures a given `x` gets passed into `Foo()` when you provide a given `y` as a parameter. This aspect of your method is unlikely to change as you modify implementation details--or if it does, it will probably be the sort of change which will require the test to be updated before it compiles.

On the other hand, if you're using a service locator, you're effectively passing hard-coded "magic classes" into the method all over the place. The values you pass in are rarely based on the input provided to the method, and can easily change as you refactor, without forcing anyone to recognize that your method now makes use of a different "interface" method, which needs to be mocked up before the test will run.

The fact that the intended usage of the interface involves passing in hard-coded values that are not defined by an `enum` indicates that you are, in essence, creating an infinite number of different "ways" to call methods on your interface. If you subscribe to the notion of "role-based" interfaces (which I do), then the usage of the interface should be what informs how the interface is written, and if there are a large number of ways for calling code to invoke a method on your interface, it creates the same problems as having a large number of methods on the interface.

What about Generics?

"Okay," you may be saying, "but wouldn't that make this an argument against all generics? After all, you can pass any type you want into your `Maybe.Not<>()` method, and generic parameters are hard-coded by definition: isn't that an anti-pattern?

The difference is that the behavior of a `List<>` constructor, or `Maybe.Not<>()` is exactly the same no matter what type gets passed in. These are data structures, not service methods. 

On the other hand, if you're talking about something like Automapper's `Mapper.Map<,>()` method, its behavior is going to vary widely based on the generic type arguments, and I'd suggest that this is, indeed, an anti-pattern. Your call to AutoMapper will be working fine one day, and then fail the next just because you renamed a property somewhere, and no compiler ever gave you so much as a hint that you broke your mapping.

I've got more opinions to offer on AutoMapper, but now's not the time. Hopefully this was enlightening.

4 comments:

  1. I don't really buy Mark's argument.

    If a DI container is being used to instantiate objects, then the 'client' can just add constructor arguments of any Type they like, in much the same way. At design time, a class is a blank canvas.

    ReplyDelete
  2. That is true, but with DI you get the added benefit of those dependencies being laid out clearly in the ctor. Yes, it does give you the same functionality as a service locator, but it does so by adding a constraint (dependencies must be outlined in the ctor) which gives you a much better idea about the dependencies a class requires. Once the amount of dependencies a class requires grows to a certain point (easily spotted with DI - you have a bunch of parameters), then you refactor.

    With the service locator, the existence of those dependencies carries much less weight and are much less visible.

    ReplyDelete
    Replies
    1. I think Nelson just nailed it.

      In truth there's not much difference. Though using DI where dependencies are visible in the constructor, not in the middle of the method somewhere helps. This also allows the bulk of code to be agnostic of the DI mechanism, which helps unit testing. You could "manually" use a DI container like Unity to resolve instances, and you'd face the same problem as the service locator.

      Delete
    2. Making your dependencies explicit in the constructor tends to promote fail-fast behavior, which I'm a big fan of. Since most people don't use DI frameworks in their unit tests (tests use mocks instead), this helps you to catch problems in tests at compile time (mere seconds after introducing the error) with the IDE pointing you straight at the problem, rather than having to run the tests (minutes) and track down why the test is failing.

      I've also found it beneficial to write a single integration test that scans my assembly and tries to instantiate all of my context roots (in my case, MVC controllers) with the DI container. That way, if there are any bad DI configurations, I can catch this automatically with a fast-running test. Using a service locator, you wouldn't know there's a problem until you hit the specific code path that uses that dependency.

      Delete