2

Recently I was reading a lot of stuff about application design patterns: about DI, SL anti-pattern, AOP and much more. The reason for this - I want to come to a design compromise: loosely coupled, clean and easy to work with. DI seems ALMOST like a solution except one problem: cross-cutting and optional dependencies leading to constructor or property pollution. So I come with my own solution for this and I want to know what do you think of it.

Mark Seemann (the author of DI book and famous "SL is anti-patter" statement) in his book mentions a pattern called Ambient Context. Though he says he doesn't like it much, this pattern is still interesting: it's like old good singleton except it is scoped and provide default value so we don't have to check for null. It has one flaw - it doesn't and it can't know about it's scope and how to dispose itself.

So, why not to apply Service Locator here? It can solve problem of both scoping and disposing of an Ambient Context objects. Before you say it's anti-pattern: it is when you hide a contract. But in our case we hide OPTIONAL contract, so it's not so bad IMO.

Here some code to show what I mean:

public interface ILogger
{
    void Log(String text);
}

public interface ISomeRepository
{
    // skipped
}


public class NullLogger : ILogger
{
    #region ILogger Members

    public void Log(string text)
    {
        // do nothing
    }

    #endregion
}

public class LoggerContext
{
    public static ILogger Current
    {
        get
        {
            if(ServiceLocator.Current == null)
            {
                return new NullLogger();
            }
            var instance = ServiceLocator.Current.GetInstance<ILogger>();
            if (instance == null)
            {
                instance = new NullLogger();
            }
            return instance;
        }
    }
}

public class SomeService(ISomeRepository repository)
{
    public void DoSomething()
    {
        LoggerContext.Current.Log("Log something");
    }
}

Edit: I realize that asking not concrete question goes in conflict with stack overflow design. So I will mark as an answer a post best describing why this design is bad OR better giving a better solution (or maybe addition?). But do not suggest AOP, it's good, but it's not a solution when you really want to do something inside your code.

Edit 2: I added a check for ServiceLocator.Current is null. It's what I intent my code to do: to work with default settings when SL is not configured.

Sebastian Weber
  • 6,766
  • 2
  • 30
  • 49
Dmitry Golubets
  • 570
  • 5
  • 13
  • What I'm missing from your question is an example where you clearly show that you need to use `LoggerContext.Current` instead of injecting an `ILogger` in your code. In my experience, if you need to inject many `ILogger` dependencies throughout your code, you are either logging too much (instead of throwing exceptions) or you are not adhering the SRP (and you actually DO need AOP). Take a look at this answer: http://stackoverflow.com/questions/9892137/windsor-pulling-transient-objects-from-the-container. – Steven May 21 '12 at 10:32

2 Answers2

3

A problem with an ambient context as the one you propose, is that it makes testing much harder. For a couple of reasons:

  1. While running your unit tests, there must always be a valid instance registered in "ServiceLocator.Current". But not only that, it must be registered with a valid ILogger.
  2. When you need to use a fake logger in a test (other than the simple NullLogger), you will have to configure your container, since there is no way in hooking into this, but since the container is a singleton, all other tests will use that same logger.
  3. It will be non-trivial (and a waste of time) to create a solution that works when your unit tests are run in parallel (as MSTest does by default).

All these problems can be solved by simply injecting ILogger instances into services that need it, instead of using a Ambient Context.

And if many classes in your system depend on that ILogger abstraction, you should seriously ask your self whether you're logging too much.

Also note that dependencies should hardly ever be optional.

Steven
  • 166,672
  • 24
  • 332
  • 435
  • 1. Steven, I forgot about checking ServiceLocator.Current for null, thanks for pointing this out. With this check tests aren't forced to initialize SL. 2. why should I test something like ILogger in the first place? But if I want it's still doable. 3. I agree - this is a problem if you want to test it. But again - it's doable. In the end, what's more important: write real code easy or write tests easy? – Dmitry Golubets May 21 '12 at 11:58
  • 1
    3. There are a lot of studies that prove that you can't write quality code without being able to test it. So being able to write tests easily is crucial for writing quality applications. – Sebastian Weber May 21 '12 at 12:56
  • 1
    @DmitryGolubets: Let me turn this around. Why shouldn't you want to test the usage of an ILogger inside a class? Since you have written this, this must be valuable business logic and you should want to know whether this code is correct. If this code isn't really valuable, why didn't you write it in the first place? Perhaps this line is a cross cutting concern (AOP) and is not important business logic. In that case you should't complicate your business logic and you should have extracted this logic to a decorator or some kind. – Steven May 21 '12 at 12:56
  • About point 3. It seems MSTest by default runs tests in serial (http://stackoverflow.com/questions/154180/how-does-nunit-and-mstest-handle-tests-that-change-static-shared-variables) – Karsten Feb 26 '14 at 12:12
3

You can add cross-cutting concerns using hand-crafted decorators or some kind of interception (e.g. Castle DynamicProxy or Unity's interception extension).

So you don't have to inject ILogger into your core business classes at all.

Sebastian Weber
  • 6,766
  • 2
  • 30
  • 49
  • 2
    Yeah, I read the same suggestion everywhere, but it doesn't feel right to me. Sometimes logs should be written inside of a method in some "if" branch. Interception doesn't solve this. And ILogger is just an example, but there can be other interfaces with methods for reading some values. – Dmitry Golubets May 21 '12 at 12:04
  • 2
    @DmitryGolubets: If you think logging is an essential aspect of your class make that obvious and inject it using constructor injection. If its for tracing input values and logging exceptions (I treat logging and tracing as fundamentally different things!) use decorators or interceptors. But never hide a dependency! That will bite you rather sooner than later. Always. – Sebastian Weber May 21 '12 at 12:49
  • If you want the logger to be part of encapsulated class logic, maybe its better to split your class on few fine-grained classes and decorate each of them? – Dmitriy Startsev May 21 '12 at 12:52
  • I prefer finer-grained class structures over few large classes. But if being able to use decorators for logging is the only reason to divide your code into smaller pieces I would question that approach. – Sebastian Weber May 21 '12 at 13:03