Suggest design: almost every object in app has loggger

Question

I'm writing an application. I use NLog for logging. In this application almost every object can write to log. I define protected member for that:

protected Logger logger;

protected virtual Logger Logger
{
     get { return logger ?? (logger = LogManager.GetLogger(this.GetType().ToString())); }
}

In that case I need to copy/paste this code for every base class in application. Or I see other option: define app-specific root object with logger in it and subclass it. But semantically this sounds wrong because it is not true "is-a" situation for me.

are there any better options?

"Constructor Dependency Injection" (e.g. Ninject, Funq, Your DI Pick) + "An Awesome IDE Extension" (Like ReSharper). Using a subclass here is mostly likely incorrect. It takes about 20 keystrokes - depending on identifier name length - to add an particular service as a DI to one of my classes. — , Mar 13 '13 at 08:43
What I don't like about that approach is that it forces every type to have a constructor with Logger parameter, while logger is quite often an optional dependency. Injecting it via property also doesn't seem right as this is not something which should be done from outside. — Zdeslav Vojkovic, Mar 13 '13 at 08:46
@ZdeslavVojkovic As long as the entire stack uses constructor DI injection, there is no issue. Because no (service) constructor is ever called manually - so there are no overloadeds or alternative constructors (there is always property injection). Of course, if it's not a component .. but in that case, why does it want a logger? I think a logger is the prime example of something that usually *should* be controlled externally - I don't like the Service Locator pattern (i.e. `GetLogger`) and prefer direct injection. — , Mar 13 '13 at 08:47
Sure, but it forces you to do everything via DI (which might be good or bad, won't go into that), and also results in objects with wild constructors. Additional false assumption, IMO, is that you only want to use logger in services. If you want to use it elsewhere, you need to hook up everything with IoC container (I assume you mean IoC when you say DI, as DI itself doesn't guarantee that ctors are not called manually) — Zdeslav Vojkovic, Mar 13 '13 at 08:53

Zdeslav Vojkovic · Accepted Answer · 2016-10-18T12:22:20.913

Sometimes I really wish C# would support multiple inheritance or mixins....

You could write an extension method:

public static Logger Logger(this object obj) {
    return LogManager.GetLogger(obj.GetType());
}

The drawback is that it would be a bit slower as the created instance is not cached (except internally in NLog, which is an implementation detail), but you could do it yourself:

public static Logger Logger(this object obj) {
    Logger logger;
    Type type = obj.GetType();
    // s_loggers is static Dictionary<Type, Logger>
    if (!s_loggers.TryGetValue(type, out logger)) { // not in cache
        logger = LogManager.GetLogger(type);
        s_loggers[type] = logger;  // cache it
    }
    return logger;
}

Than you can call it like this:

this.Logger.Log(...)

The obvious drawback is that any object could write to logger of any other object.

With regard to comment about memory leak (now deleted):

The first implementation solves that. However, it is not a leak more than any static object. It would be a leak if you could not access these objects. As an alternative you could cache WeakReference to logger instead of logger itself, but I see no point, as I believe that there already is some caching in NLog itself. Otherwise, NLog would always have to create a new instance of logger for each type.

David Moore · Answer 2 · 2013-03-13T20:43:43.120

I would recommend you use a static logger so that you get a per-class logger. This stops the overhead of creating a logger for each instance (the logger is thread safe):

class MyClass
{
    static readonly Logger logger = LogManager.GetCurrentClassLogger();
}

GetCurrentClassLogger will save you from having to name the logger explicitly, but the drawback is additional overhead as it has to figure the name of the logger out for you from the stack trace at runtime.

This is likely not a big deal in most cases, but otherwise, it's a bit faster to do this:

class MyClass
{
    static readonly Logger logger = LogManager.GetLogger("MyClass");
}

I would stick with GetCurrentClassLogger until / unless the slight overhead turns out to be an issue in your solution.

The impression I'm getting is that you're trying to cut down on typing, and/or you're addressing the (good) natural aversion to copying and pasting duplicate code.

However, this pattern of having a logger per class with a static initializer is well accepted and works best in most cases. You might be best served sticking with that, and perhaps setting up a code snippet to save you some typing.

If you're still not happy with that, Dependency Injection (either through constructor or property injection) or even Aspect Oriented logging management could be other things for you to investigate.

Suggest design: almost every object in app has loggger

2 Answers2

Linked