The Problem

One of the challenges many legacy code bases face is that there are static service locators and static service classes sprinkled throughout the code. This is an anti-pattern which makes isolation of code for testing difficult and the tight coupling makes the code base more brittle. I would go into details about the anti-pattern, but this article pretty much covers it:

http://www.devtrends.co.uk/blog/how-not-to-do-dependency-injection-the-static-or-singleton-container

How we can go about fixing it

Scenario 1

Here is an simplified example of a static service class code smell leading to the anti-pattern: below is a static method providing some sort of service.namespace StaticServiceLocator

  1. public class Consumer
  2. {
  3. public void DoSomething()
  4. {
  5. // do some work
  7. var fuel = new Fuel();
  8. var smoke = SmokeService.MakeSmoke(fuel);
  10. // do some more work
  11. }
  12. }

Here is the culprit: A static service class.

  1. namespace StaticServiceLocator
  2. {
  3. public class SmokeService
  4. {
  5. public static Smoke MakeSmoke(Fuel fuel)
  6. {
  7. return fuel.Burn();
  8. }
  9. }
  10. }

  • One internal master constructor rule

In my experience, having one internal master constructor with all the class dependencies responsible for setting state is the way to go in terms of maintainability and flexibility. The master constructor should be the ONLY constructor that sets state in the class. If there is a need to instantiate the class from the consumer you can add public constructors that call the master constructor. You can then test the class if you make your test assembly a friend assembly (using the InternalsVisibleTo assembly attribute) but can control the public surface and keep it simple for consumers. If the class to too complex for a master constructor, it is time to look into a factory or refactoring the class.

Since this is a simple example, we have no constructors. In real life there may be work to do in order to consolidate the existing constructors.
  • Define an internal interface if the service is a static class
Usually static methods external to the class are a code smell that will usually lead you to some kind of fire. If the static method behavior is not a candidate for encapsulation withing the class then it is a true dependency and it should be injected. If the static method behavior is only used by the consuming class then the method should be moved inside the class and made private.

Extension methods on classes are a bit less smelly but having core functionality live outside your core class increases complexity and it is easy to have broken encapsulation. Extension methods on interfaces generally make more sense as long as they don’t impact state.

To fix a static service class, we will create an internal interface with the same method stubs. We don’t want to clutter the public surface of our library and many of the service interfaces should remain internal to the assembly.

Add the new interface to the internal master constructor and create an overloaded public constructor with the original contract so that we don’t break any consumers.
At this point we swap out the implementation in the consumer as well.
  1. namespace StaticServiceLocator
  2. {
  3. public class Consumer
  4. {
  5. private ISmokeService _SmokeService;
  7. internal Consumer(ISmokeService smokeService)
  8. {
  9. _SmokeService = smokeService;
  10. }
  12. public void DoSomething()
  13. {
  14. // do some work
  16. var fuel = new Fuel();
  17. var smoke = _SmokeService.MakeSmoke(fuel);
  19. // do some more work
  20. }
  21. }
  22. }

  • Refactor the static service class to be non-static and have it implement the interface

This helps find all other places where the service is consumed by creating compile-time breaks so we know what needs to be cleaned up.
  1. namespace StaticServiceLocator
  2. {
  3. public class SmokeService : ISmokeService
  4. {
  6. public Smoke MakeSmoke(Fuel fuel)
  7. {
  8. return fuel.Burn();
  9. }
  10. }
  11. }

  • Implement the overloaded constructor(s)

The constructor overload that calls to the master constructor will resolve the service to be injected (either through IOC or just instantiation). If we use dependency injection correctly here we can achieve loose coupling. If we use instantiation, we are still coupled but it is not as tight and we can work around it when writing unit tests by using the master constructor. If the instantiation of the service library is hidden from our consumers we will be able to refactor it later to be IOC or at the very least swap out the implementation in code much easier.

This example uses poor man’s dependency injection which we would later change to use an IOC framework to provide DI.
  1. namespace StaticServiceLocator
  2. {
  3. public class Consumer
  4. {
  5. private ISmokeService _SmokeService;
  7. internal Consumer(ISmokeService smokeService)
  8. {
  9. _SmokeService = smokeService;
  10. }
  12. public Consumer() : this(new SmokeService()) { }
  14. public void DoSomething()
  15. {
  16. // do some work
  18. var fuel = new Fuel();
  19. var smoke = _SmokeService.MakeSmoke(fuel);
  21. // do some more work
  22. }
  23. }
  24. }

Now we are able to isolate the consumer class code for testing, have not changed the public surface and shouldn’t have broken any consumers.

Scenario 2

Sometimes there will be an IOC framework providing a static service locator. This is still a smell, but much less invasive to clean up because we are already working with an abstraction for the service class.

  1. namespace StaticServiceLocator
  2. {
  3. public class Consumer2
  4. {
  5. public void DoSomething()
  6. {
  7. // do some work
  9. var fuel = new Fuel();
  10. var smoke = ServiceLocator.GetService<ISmokeService>().MakeSmoke(fuel);
  12. // do some more work
  13. }
  14. }
  15. }

This issue is a bit easier to fix in that we just have to modify this class by creating the internal constructor in order to do dependency injection and move the service locator up to the overloaded public constructor so that we don’t change the public surface and can isolate our consumer class code for unit testing. Eventually moving to an IOC framework to provide DI we can safely refactor the behavior of the new constructor.

  1. namespace StaticServiceLocator
  2. {
  3. public class Consumer2
  4. {
  5. private readonly ISmokeService _SmokeService;
  7. internal Consumer2(ISmokeService smokeService)
  8. {
  9. _SmokeService = smokeService;
  10. }
  12. public Consumer2() : this(ServiceLocator.GetService<ISmokeService>()) { }
  14. public void DoSomething()
  15. {
  16. // do some work
  18. var fuel = new Fuel();
  19. var smoke = _SmokeService.MakeSmoke(fuel);
  21. // do some more work
  22. }
  24. }
  25. }