.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 Ознаке: ,,,,
Filed under: Uncategorized 5 Comments
16Oct/0912

Inversion Of Control, Single Responsibility Principle and Nikola’s laws of dependency injection

Today I stumbled upon the stack overflow question regarding using DI frameworks for classes with many dependencies where the example given and couple of answers have reminded me about a subject I want to post for a long time.

So the example from question basically goes like this:

MyClass(ILog log, IAudit audit, IPermissions permissions, IApplicationSettings settings)

// ... versus ...

ILog log = DIContainer.Get<ILog>();

And the 3 questions related to this example are:

  1. How to avoid passing those common but uninteresting dependencies interfaces to every class?
  2. How do you approach dependencies that might be used, but may be expensive to create?
  3. How to build object graph of MyClass considering the fact that any of those dependencies can have their own dependencies etc..

So, let’s start and keep it short…

What is wrong fundamentally with his example?

Being a big proponent of DDD principles as the one leading to clean and maintainable design, I found the design of the question example is wrong in a sense that an entity (which MyClass is) should NOT have dependencies on infrastructure (or any other) services.

Related to that is Nikola’s 1st law of IoC

Store in IoC container only services. Do not store any entities.

I strongly believe that if the author of example was following that principle he wouldn’t be in the position to ask the question he did ask. 

Second thing I learned in past years to detect as wrong in code like the one in example is the high number of dependencies in a constructor which usually in my experience points to SRP (Single Responsibility Principle) violations and low SOC (Separation of Concerns) in code. The example is not showing the code of the MyClass but (based on my experience) I am pretty sure it can be broken to couple of coherent SRP separate classes where each one of them would have very few (if any) number of dependencies

Related to that is Nikola’s 2nd law of IoC

"Any class having more then 3 dependencies should be questioned for SRP violation"

Basically all answers on the question are agreeing that second approach is worst (together with me) because burring up the dependencies a class has prevents effective black box unit testing and makes harder refactoring. I already blogged about  transparent vs. opaque DI, so I’ll skip beating the dead horse here.

The thing I do want here to discuss are the answers which are recommending in general replacing the first case with the single dependency on IServiceLocatorIContainer.

IServiceLocatorIContainer injection doesn’t make a lot of sense in general

There is no real advantage of using the IServiceLocator vs the DIContainer from the second solution. I mean we would be decoupled from the specific IoC container but the problems of low testability and hard refactoring would stay due to the same opaque nature of dependencies class has. In other words, from my point of view using singleton service locator in this sense is even better then the IServiceLocator being injected (same set of problems, one parameter less in constructor).

The only exception from this rule is the case when we have multiple components mapped to a service with a different key (IoC version of Strategy pattern implementation) there is no way to inject the one with a given key (as long they share the same interface) so injecting IContainer in that case is acceptable.

Setter type of dependency injection shouldn’t be used

Second type of answer is not to use constructor dependency injection but instead setter type of DI (I’ve blogged about the differences long time ago). Couple a years ago I was fan of the setter type from the same reasons (removes the constructor noise) but the set of problems I was facing in real world related to it convince me that it is much worse choice then the constructor type. The main reasons behind that opinion are primarily due to the facts that setter type of DI makes dependencies again opaque and (in this case) even more important result with creation of public API members which only purpose is to satisfy infrastructure needs which (I believe) is deeply wrong. No API members should be created just in order to satisfy the infrastructure andor unit testing needs.

Nikola’s 3rd law of IoC

Every dependency of the class has to be presented in a transparent manner in a class constructor.

Factories and IoC

The question contains a thought of injecting the factory interface which reminded me also on a discussion I had with one of my colleagues regarding usage of factories in certain scenarios (as proposed in Art Of Unit Testing – go buy that book in case you haven’t done that already).

IMO, using factories together with IoC doesn’t make a lot of sense because IoC container is in a sense “universal abstract factory”.

In other words, in my experience any time I thought about adding a factory I ended with much simpler IoC based solution so that’s why I would dare to say that “IoC kill the Factory star"

Q2: How do you approach dependencies that might be used, but may be expensive to create?

Very simple, don’t create them to be like that. A constructor of a class being resolved from a IoC should be as light as possible just defining (if any) its own dependencies. Any class initialization or implementation shouldn’t be implicitly triggered from constructor but instead explicitly by invoking a specific member on instance resolved from a container. That’s how resolving all of them could be done without any significant performance issues.

Nikola’s 4th law of IoC

Every constructor of a class being resolved should not have any implementation other then accepting a set of its own dependencies.

Q3: How to build object graph of MyClass?

Without going into the details of how to do this with given framework (which other did in the SO answers and I covered it for Unity here) I would like to emphasize again the need for moving the mapping definition and resolution from UI elements (as proposed as option question) to dedicated application bootstrapper classcomponent  which (implementing the Builder design pattern) sole responsibility would be to define the mappings in a single place or orchestrating other component bootstrappers.

On the beginning of the application life cycle, bootstrapper would build up the dependencies removing (ideally) the need for other parts of code base to be aware of the IoC container awareness.

(More about Builder pattern in dusty but still good Jeremy’s blog post)

Nikola’s 5th law of IoC

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."

Appendix

So here we go, my dear reader – my first blog post in a while. I decided to fight with my Twitter addiction which sucked up all of my blogging energy and to start writing down (in my awful English) the thoughts and experiences I collected in last year so stay tuned for the bunch of very diverse and (hopefully) amusing blog posts

Filed under: Uncategorized 12 Comments