.NET and me Coding dreams since 1998!

20Oct/095

Say no to ServiceLocator

(Disclaimer: As I stated here, while I find over the time ServiceLocator based code to be a bad practice, I do understand the need for its usage in certain brown-field  scenarios as a way of reducing the risk while introducing the IoC.)

In my previous Nikola’s laws of dependency injection blog post I stated couple of IoC laws based on my experience:

  1. Store in IoC container only services. Do not store any entities.
  2. Any class having more than 3 dependencies should be questioned for SRP violation
  3. Every dependency of the class has to be presented in a transparent manner in a class constructor.
  4. Every constructor of a class being resolved should not have any implementation other then accepting a set of its own dependencies.
  5. IoC container should be explicitly used only in Bootstrapper.
    Any other “IoC enabled” code (including the unit tests) should be completely agnostic about the existence of IoC container.

The #5 (“IoC container should be a ghost even in unit tests”) resulted with couple of blog post comments and emails asking for clarification what I mean by that.

In short:

Say no to Service Locator Opaque dependencies!

imageRadenko Zec posted in comment example which with very slight modifications (to reflect better my Law #1) looks like this…

There are 4 projects in the solution used in today blog post:

  1. ClassLibrary containing the two classes where:
    - UserRepository is having dependency on other
    - LoggingService (implementing the ILoggingService)
  2. ConsoleApplication1 simulating the production usage and containing the:
    - bootstrapper class (defining IoC mappings) and the
    - Program.cs (main console file which would execute our functionality
  3. Infrastructure containing my naive implementation of Unity container adapter with the
    - IContainer abstraction of the Unity container,
    - UnityContainerAdapter class implementing the adapter design pattern adapting the UnityContainer to IContainer 
    - UnityContainer service locator class,
  4. TestProject1 containing the unit test for the ClassClibrary1 - UserRepositoryTest

Before diving into the Radenko’s sample implementation let me restate my point by saying that

  • none of the classes using IoC (in this simple example UserRepository) should NOT be aware at all of IoC and
  • UserRepositoryTest should NOT have testfixture setup methods filling the container

Service locator based implementation

As given in Radenko’s comment example Bootstrapper class registers the mappings

namespace ConsoleApplication1
{
    using ClassLibrary1;
    using Infrastructure;

    public static class Bootstraper
    {
        public static void SetUp()
        {
            IContainer cont = UnityContainer.Instance();
            cont.Register<ILoggingService, Loggingservice>();
        }
    }
}

Here’s the simple implementation of that LoggingService (not very important for this blog post but still to be clear)

namespace ClassLibrary1
{
    using System;
    using System.Diagnostics;

    public class LoggingService : ILoggingService
    {
        public void Log(string message)
        {
            Debug.WriteLine(string.Format("LOGGED: {0}.", message));
        }
    }
}

And here’s the class being dependable on that LoggingService

namespace ClassLibrary1
{
    using Infrastructure;

    public class UserRepository
    {
        public void Delete (int userId)
        {
            IContainer cont = UnityContainer.Instance();
            ILoggingService loggingService = cont.Resolve<ILoggingService>();
            loggingService.Log(string.Format("User with id:{0} deleted", userId));
        }
    }
}

So, as you can see in a given example Delete method is implemented like this:

  • get ServiceLocator (singleton instance of the UnityContainerAdapter)
  • using the ServiceLocator retrieve from IoC container component mapped to ILoggingService
  • use that component to log a message.

What is wrong with this code?

  • It violates the Separation of Concerns Single Responsibility Principle  – UserRepository taking care about IoC, instance resolution etc
  • It good example of opaque dependency injection which hides sometime the set of dependencies component has.

    Looks like “not a big deal” but when you face the class with couple of thousands of lines and you have to read them ALL just to get the list and repeat that couple of times, it is a big deal. (Yes, I do agree with you that having that long classes is atrocity of its own kind, but seeing it all the time in my world)

(Couple of more reasons but let’s just continue our journey…)

Here’s how the production usage would look in this example

namespace ConsoleApplication1
{
    using ClassLibrary1;

    class Program
    {
        static void Main(string[] args)
        {
            Bootstraper.SetUp();
            UserRepository userRepository = new UserRepository();
            userRepository.Delete(3);
        }
    }
}

No magic there: code issues a command to bootstrapper to initialize IoC, create a instance of UserRepository and invoke its Delete method.

The last thing left is the example of unit test one might write with this code

namespace TestProject1
{
    using Microsoft.VisualStudio.TestTools.UnitTesting;
    using ClassLibrary1;
    using Infrastructure;
    using Rhino.Mocks;

    [TestClass]
    public class UserRepositoryTest
    {
        private ILoggingService loggingService;

        [TestInitialize]
        public void Would_Be_Executed_Before_Every_Test()
        {
            var serviceLocator = UnityContainer.Instance();
            loggingService = MockRepository.GenerateMock<ILoggingService>();
            serviceLocator.Register(loggingService);
        }


        [TestMethod]
        public void Delete_InCaseOfValidUserID_WouldLoggAMessage()
        {
            // arrange
            var userId = 3;
            loggingService.Expect(p => p.Log(string.Empty)).IgnoreArguments();

            // act
            UserRepository userRepository = new UserRepository();
            userRepository.Delete(userId);

            // assert
            this.loggingService.VerifyAllExpectations();
        }
    }
}

This is not a blog post on unit testing so just a short discussion what the test does :

In Test initialization the singleton instance of IoC containe is retrieved. Then an RhinoMock dynamic mock class of ILoggingService is created and then injected into the container. Summarized: we stored something in a container before the test execution, because we know from the source code that it would be used in SUT.

The test itself is very simple:

  • Arrange defines what is the expected behavior of the logging service which is supposed to be caused as a result of SUT activity
  • Act creates an instance of the UserRepository and invokes the delete method
  • Arrange verifies that the expected behavior of the logging service occurred.

Nothing much –> just another example of behavior unit testing.

What is wrong with this unit test?

Well, I wrote them “a few” like this and it works well but with certain pain points which the more test you write the more painful become:

  1. It prevents black box unit testing.

    I tend lately to think about unit tests more as of a functional specifications and due to that I try always to test the class based on defined functional requirements WITHOUT looking in the implementation (to stay as objective as I can). Only once I cover ALL functional cases, I tend to do a quick check of the test coverage and focus on removing the code not being tested (all reqs satisfied and code not used –> potential overkill)
  2. Prevents effective unit testing

    Sooner or later, with this code you end with writing test following the “run the test, get what is missing, add a mapping, run it again, get what is missing, add a line…”  which IMHO ends to be very slow process.
  3. Makes Fixture'SetUp very big and clunky.

    Here we set up one dependency, but imagine hundreds of unit tests with their own many dependencies dig up in the code and how big this section would become…

    In the past I was trying to tackle that by creating special bootstrapper classes filling the IoC container with test infrastructure stubs and with using AutoMockingContainer (still not completely sure about should I do that or not – another blog post) but the end result is that I got more code to be maintained and which is getting broken wevery time production interfaces change etc.

  4. Decreases test readability

    If you want take tests as a sort of functional specification (and you should) pretty often you would want to “read the tests” in order to get what and how the SUT works. Having a bunch of setup code outside of the method being read makes that much harder.

All the right solutions are always simple as possible

In order to comply to law #5 I need to refactor the UserRepository implementation by removing IoC container from it and in order to comply law #3 I need to explicitly enlist in a constructor all of its dependencies.

So, following those two rules I end with this code:

namespace ClassLibrary1
{
    public class UserRepository
    {
        private readonly ILoggingService loggingService;

        public UserRepository(ILoggingService loggingService)
        {
            this.loggingService = loggingService;
        }

        public void Delete (int userId)
        {
            this.loggingService.Log(string.Format("User with id:{0} deleted", userId));
        }
    }
}

As you can see all I did was to transform the ServiceLocator type of code to dependency injection type of code where there is no more SoCSRP violation and where the UserRepository is unaware of IoC (no single line related to it).

For the folks with concerns that this could result with some very long constructors with too many dependencies I suggest checking out Auto Mocking Container and/or:

Law #2 Any class having more than 3 dependencies should be questioned for SRP violation

Let see the unit test for this class

namespace TestProject1
{
    using Microsoft.VisualStudio.TestTools.UnitTesting;
    using ClassLibrary1;
    using Rhino.Mocks;

    [TestClass]
    public class UserRepositoryTest
    {
        [TestMethod]
        public void Delete_InCaseOfValidUserID_WouldLoggAMessage()
        {
            // arrange
            var userId = 3;
            
            var loggingService = MockRepository.GenerateMock<ILoggingService>();
            loggingService.Expect(p => p.Log(string.Empty)).IgnoreArguments();
            // act
            UserRepository userRepository = new UserRepository(loggingService);
            userRepository.Delete(userId);
            
            // assert
            loggingService.VerifyAllExpectations();
        }
    }
}

As you can see above:

  • There is no more initialization routine – all of the code related to this test is in one place.
  • Every test initialize only what it needs (no big chunk of initializing union of  of mappings all tests need)
  • While writing the test the C# compiler informs me that the UserRepository needs a logging service. If I don’t provide it test won’t compile.

    That’s how I can avoid looking at the source code in order to get what dependency class has,
  • As for law #5, notice that in my unit test there is NO IoC code at all (even infrastructure component is not referenced anymore).

    IoC has to be for us when needed but also it has to stay away from our work. Full orthogonality. Period.

How the production code looks with new approach?

Almost the same

namespace ConsoleApplication1
{
    using ClassLibrary1;

    using Infrastructure;

    class Program
    {
        static void Main(string[] args)
        {
            Bootstraper.SetUp();
            var unityContainer = UnityContainer.Instance();

            var userRepository = unityContainer.Resolve<UserRepository>();
            userRepository.Delete(3);
        }
    }
}

The main difference is that I’ve used the unity to resolve UserRepository and performs (by that) automatic object graph wire up. It may look like that we ended with the same thing just in other class and that is correct (partially)!

It is correct in a sense that if you think for a second about the real world systems you have many classes chained in cross dependencies in which case it is in your interest to push out from all of them the IoC resolution and let the IoC container of your choice do the auto wire up magic.

It is not correct in a sense that in a most part of your system (usually matching the area you want to test( you what have implict injection and not this explicit. For example, if your CompanyRepository would need a UserRepository (not claiming it has real sense to be done) the CompanyRepository would just have in its constructor IUserRepositoryUserRepository and that’s it.

Conclusion

Reafactoring from service locator to real dependency injection type of code is very easy if not trivial but do require a small “bing!” in your head to just realize it.

After switching to this practice, my tests get much lighter, faster, easier to read and maintain.

Radenko, I hope I answered your question :)

Here’s a sample code I used in today’s blog

Technorati Ознаке: ,,,,
Comments (5) Trackbacks (1)
  1. Great post and I think you did a great job of pointing out the benefits of DI through constructor rather then through properties.

    How would you handle the scenario when your business class/entity wants to create another business class on its own? It would still be coupled with the container since you would have to create/resolve this new class through the container.

    Would some sort of aspect framework be better for initializing new entities, can it even be done?

  2. I could not agree with you in all cases. I also using same method that you have described in this article but in some cases you need to have some facade pattern in code.

    If you are building dll that someone else will consume

    for example is much cleaner to have

    {

    Bootstraper.SetUp();  

    UserRepository userRepository = new UserRepository();  

    userRepository.Delete(3);  

    }

    and not use any IOC container.

    And if you are building classes that some other programmers will consume is much better to have Facade pattern rather than having dependencies in constructor.

    For example if I have class for Logging errors that log error in Database, send Email to developer and log error in text file I think that is much better to have call like this without any call to IOC container:

    Bootstraper.SetUp();  

    ILogger logger=new Logger();

    logger.Log("error message");

    rather then

    Bootstraper.SetUp();  

    IContainer cont = UnityContainer.Instance();  

    ILogger logger=unityContainer.Resolve<Logger>();  

    logger.Log("error message")

    On the other hand I will create classes on lower levels like

    DatabaseLogger or EmailLogger same way that you have described here if they have some dependency classes.

    What do you think?

  3. As for using the UserRepository userRepository = new UserRepository() as a way of removing the need for IoC container -> it just moves the IoC container deeper into the UserRepository where it hurts you in your tests, API discoverability, maintainability etc (as explained in this post)..

    As for var logger  = new Logger(); you can do that even in this example (logger is not having any dependencies) but that is corner case (in this discussion)

    There are two major reasons why you may consider using the this.container.Resolve (container is the Facade you mention):

    1. You have in real world quite often this case

    ClassA depends on ClassB

    ClassB depends on ClassC

    ClassC depends on ClassD

    If you follow the rule of explicit ctor dependencies (law #3) you would end in your code (if you don't use the container to resolve with this)

    new ClassA(new classB(new ClassC(new ClassD()))))

    Which violates the encapsulation by leaking the internals of the class B to place where class A is consumed

    Another variation of this problem can be seen if you imagine that the dependency class depends on IOC service in which case you end with

    new ClassA(container.Resolve<ISomething>())

    There are ways you can go around this: poor man dependency injection, opaque service locator etc but as my blog posts show I'm not very fond of them based on my past experience in using them.

    2. Opaque introducing of AOP

    Every IoC major container has interceptors which allow you adding aspects upon resolution enabling very interesting combinations (check out Ayende’s example http://tinyurl.com/eventbrok))

    3. SoCSRP etc

    Let IoC container do what he is supposed to do (building the object graph in this case) and our entities to do what they are supposed to do (business logic only)

     

    As for the inheritance scenario (Logger, EmailLogger etc) I have in my queue already a blog post showing my take on that (IoC combined with attributed ctor parameters)

  4. Both of your recent articles are just awesome (I’m browsing your blog now), just prove ne what I have been "following" in my code without ever realizing that I can go "the other way around", which would be wrong indeed…why to complicate my life?

    I ended using "your laws" in background by digging into the SOLID code principles, where eventually you'll end with constructors having resolvable parameters (of their dependencies) passed by interfaces, on which instantiation you’ll chose some IoC/Di container to easy your life…(writing new (new …(new…) at top of your call just doesn’t make sense)

    All of this wouldn't be needed, if coders would follow SOLID (code to abstractions rather than to implementations etc..) from the start of a Greenfield project, but I understand we live in a real world…

    It needs explanation and big attention once project is heavy in production and refactoring  (due to bad coding practices) has become inevitable…

    Once again, thank you very much for your time and evangelism to good code practices, hope Joel would listen x)

  5. I agree that dependency injection via construction is the way to go, but there are still a few situation in our apps where we either A) don't control the objects lifetime or B) have to provide a default constructor. In these situations I see no harm in using common service locator.


Leave a comment