0
\$\begingroup\$

Before I begin showing code, I have two main questions which I feel could help me.

  1. What is a service or factory, I've heard of them but why use them?
  2. Should I use one shared instance of ILogger throughout my whole application, I only plan on persisting errors and I can get the reporter from the stacktrace.

So,

I decided to write a simple logger for my dotnet core application. This logger will write messages to the console, and optionally save them to a file. I intend on only saving errors to a file.

For every class that needs a logger, I implement ILogger like so

var typeOfClass = typeof(myClass);
var myClass = new MyClass(
    new HybridLogger(
        new ConsoleLogger(typeOfClass),
        new FileLogger(typeOfClass)
    )
)

I thought that was too much, so I simplified it

public static class LogProvider
{
    public static ILogger CreateLogger(Type type)
    {
        return new HybridLogger(new ConsoleLogger(type), new FileLogger(type));
    }
}

This feels much better

var myClass = new MyClass(LogProvider.GetLogger(typeof(MyClass));

I have an interface, ILogger which all implementations implement. This ensures all implementations have the correct methods and I can use ILogger to encapsulate the concrete implementation

public interface ILogger
{
    void Trace(string message, bool logToFile = false);
    void Warning(string message, bool logToFile = false);
    void Debug(string message, bool logToFile = false);
    void Success(string message, bool logToFile = false);
    void Error(string message, bool logToFile = false);
    void Exception(Exception e, bool logToFile = true);
}

Let's start with the first out of the 3 implementations, ConsoleLogger. This one Console.WriteLine's messages to the console.

The implementation has different methods for different LogLevel's and will use different ConsoleColor's depending on the LogLevel chosen.

You can think of my LogType enum as LogLevel when reading the code below.

public class ConsoleLogger : ILogger
{
    private readonly Type _owner;

    public ConsoleLogger(Type owner)
    {
        _owner = owner;
    }

    private readonly Dictionary<LogType, ConsoleColor> _colorsForLogType = new Dictionary<LogType, ConsoleColor>() {
        {LogType.Trace , ConsoleColor.White},
        {LogType.Success , ConsoleColor.Green},
        {LogType.Warning ,    ConsoleColor.Yellow},
        {LogType.Debug ,   ConsoleColor.Cyan},
        {LogType.Error ,   ConsoleColor.Red},
    };

    public void Trace(string message, bool logToFile = false)
    {
        Log(message, LogType.Trace, logToFile);
    }

    public void Warning(string message, bool logToFile = false)
    {
        Log(message, LogType.Warning, logToFile);
    }

    public void Debug(string message, bool logToFile = false)
    {
        Log(message, LogType.Debug, logToFile);
    }

    public void Success(string message, bool logToFile = false)
    {
        Log(message, LogType.Success, logToFile);
    }

    public void Error(string message, bool logToFile = false)
    {
        Log(message, LogType.Error, logToFile);
    }

    public void Exception(Exception e, bool logToFile = true)
    {
        Log("An error occurred: " + Environment.NewLine + e, LogType.Error, logToFile);
    }

    private void Log(string message, LogType type, bool logToFile = false)
    {
        var oldColor = Console.ForegroundColor;
        var newColor = _colorsForLogType[type];

        Console.ForegroundColor = newColor;
        Console.WriteLine($"[{DateTime.Now:MM/dd HH:mm:ss}] " + message);
        Console.ForegroundColor = oldColor;
    }
}

Now here is the FileLogger - this is used to persist errors logs so that I can read them later, I use this library in a server so its unwatched for most of its lifetime.

public class FileLogger : ILogger
{
    private readonly Type _owner;

    public FileLogger(Type owner)
    {
        _owner = owner;
    }

    private readonly Dictionary<LogType, string> _fileNameForLogType = new Dictionary<LogType, string>() {
        {LogType.Trace , "trace.log"},
        {LogType.Success , "success.log"},
        {LogType.Warning ,    "warn.log"},
        {LogType.Debug ,   "debug.log"},
        {LogType.Error ,   "error.log"},
    };

    public void Trace(string message, bool logToFile = false)
    {
        // Should only need the Log method in this class but
        // I need to comply to ILogger, I'll keep them until
        // I come up with a solution, an ideal one would be 
        // I just call Log(message, type) from the HybridLogger

        // But thinking, is extra methods better? Encapsulation for LogType...

        LogToFile(message, LogType.Trace);
    }

    public void Warning(string message, bool logToFile = false)
    {
        LogToFile(message, LogType.Warning);
    }

    public void Debug(string message, bool logToFile = false)
    {
        LogToFile(message, LogType.Debug);
    }

    public void Success(string message, bool logToFile = false)
    {
        LogToFile(message, LogType.Success);
    }

    public void Error(string message, bool logToFile = false)
    {
        LogToFile(message, LogType.Error);
    }

    public void Exception(Exception e, bool logToFile = true)
    {
        LogToFile(e.ToString(), LogType.Exception);
    }

    private void LogToFile(string message, LogType type)
    {
        FileUtilities.WriteToFile(
            Path.GetFullPath(FileUtilities.GetStoragePath(), "/logging/" + _fileNameForLogType[type]),
            $"Occurred at [{DateTime.Now:MM/dd HH:mm:ss}] in [{_owner.FullName}]: " + Environment.NewLine + message + Environment.NewLine
        );
    }
}

Lastly, I thought maybe I want to write to the console AND persist it to a file, so maybe an implementation that does two in one? I feel like there could be a better way to merge implementations, so if anyone has any bright ideas you can let me know.

public class HybridLogger : ILogger
{
    private readonly ILogger _consoleLogger;
    private readonly ILogger _fileLogger;

    public HybridLogger(ILogger consoleLogger, ILogger fileLogger)
    {
        _consoleLogger = consoleLogger;
        _fileLogger = fileLogger;
    }

    public void Trace(string message, bool logToFile = false)
    {
        _consoleLogger.Trace(message, logToFile);

        if (logToFile)
        {
            _fileLogger.Success(message);
        }
    }

    public void Warning(string message, bool logToFile = false)
    {
        _consoleLogger.Warning(message, logToFile);

        if (logToFile)
        {
            _fileLogger.Success(message);
        }
    }

    public void Debug(string message, bool logToFile = false)
    {
        _consoleLogger.Debug(message, logToFile);

        if (logToFile)
        {
            _fileLogger.Success(message);
        }
    }

    public void Success(string message, bool logToFile = false)
    {
        _consoleLogger.Success(message, logToFile);

        if (logToFile)
        {
            _fileLogger.Success(message);
        }
    }

    public void Error(string message, bool logToFile = false)
    {
        _consoleLogger.Error(message, logToFile);

        if (logToFile)
        {
            _fileLogger.Success(message);
        }
    }

    public void Exception(Exception e, bool logToFile = true)
    {
        _consoleLogger.Exception(e, logToFile);

        if (logToFile)
        {
            _fileLogger.Success(e.ToString());
        }
    }
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Please change the title So that it fits the standard on this site. That Is to state what the code Is about rather then your concerns about it. \$\endgroup\$ Commented Feb 10, 2020 at 18:43
  • \$\begingroup\$ Your HybridLogger logs Sucess for all file logs. Doesn't look right. \$\endgroup\$ Commented Feb 11, 2020 at 6:59
  • \$\begingroup\$ you're trying to do what is already implemented in NLog. \$\endgroup\$ Commented Feb 11, 2020 at 12:20
  • \$\begingroup\$ @iSR5 NLog is fairly heavy - even though I've already stated this is for educational purposes and not to reinvent the wheel. \$\endgroup\$ Commented Feb 11, 2020 at 20:09

1 Answer 1

Reset to default
2
\$\begingroup\$

What is a service?

There are mupltiple definitions. Probably the most generic one is that it is any object that provides some useful functionality. Sometimes it may be a synonym for a singleton.

What is a factory?

Factory is basically a service that instantiates other objects. A class constructor can be considered a factory in a way that is not polymorhic (in runtime). More complex factories should often stand aside from the class being instantiated as it involves more dependencies needed only for the instantiation but not by the instantiated object once it exists (like in your case HybridLogger does not need to know which loggers it is given but your factory combines the two concrete implementations). Factory method generally helps separate concerns and maintain single responsibility principle. One class should either create objects, or act on them when they already exist, but not both. If they need new objects for their acting, they delegate to factories.

One generic method vs specific method for each level

Should only need the Log method in this class but I need to comply to ILogger, I'll keep them until I come up with a solution, an ideal one would be I just call Log(message, type) from the HybridLogger

But thinking, is extra methods better? Encapsulation for LogType..

You might want to design the interface so that it only has that one method.

public interface ILogger
{
  void Log(LogType type, string message);
}

logger->Log(LogType.Success, "message");
// instead of
logger->Success("message");

Now if you stil want those individual methods, you can define a wrapper.

public interface ILoggerFacade
{
  void Success(string message);
  void Warning(string message);
  // etc.
}

public class LoggerFacade : ILoggerFacade
{
  private ILogger Logger;
  public LoggerFacade(ILogger logger) {Logger = logger;}
  public void Success(string message) {Logger.Log(LogType.Success, message);}
  public void Warning(string message) {Logger.Log(LogType.Warning, message);}
}

var facade = new LoggerFacade(logger);
facade.Success("message");

Log to file or not log to file?

Notice I omitted the bool logToFile parameter, thats because whether file logging occurs is implementation detail of the logger. If you want that control from outside for some reason, you should probably merge console logger and file logger together avoiding the hybrid logger. But it makes very little sense to me.

It might make sense to decide whether file logging should occur based on the log type, you can have a configurable Ilogger implementation that follows the decorator pattern and allows to turn on/off specific log types (refered to as SwitchableLogger in the last code snippet).

"Merging" Implementations

I feel like there could be a better way to merge implementations

It is absolutely fine to combine them this way, although it could be generalized to an Enumerable to be able to log to any amount of ILogger implementations, not just two.

public class MultiLogger : ILogger
{
  private IEnumerable<ILogger> Loggers;
  public MultiLogger(IEnumerable<ILogger> loggers) {Loggers = loggers;}
  public void Log(LogType type, string message) {
    foreach (var logger in Loggers) logger.Log(type, message);
  }
}

Further we have made sure that every logger is called with the correct type, unlike your HybridLogger which calls Success on the second logger regardless of the type requested.

Logger aware of the class using it

This is actualy almost a circular reference. The loggers dont depend on the class who owns the logger, but they depend on its class type. It means you need at least one logger for each distinct class that uses a logger. You could shift this to only have one logger service in your application by having the type passed to the Log method.

public interface ILogger
{
  public void Log(LogType type, string message, Type callerType);
  //or even as template
  public void Log<T>(LogType type, string message);
}

But honestly, your ConsoleLogger does not even use that value. And secondly if the logger consumer wishes, he can add the class name to the message.

Dependency Injection

You should be explicit about where the FileLogger puts the logs. Let it get the file path mapper in constructor.

Final result

var logger = new SwitchableLogger(new FileLogger(errorLogPaths));
logger.AllOff();
logger.ErrorOn();
var facade = new LoggerFacade(new MultiLogger([new ConsoleLogger(), logger]));
// the facade is output of the factory method, im putting it here to simplify...

facade.Error("whoops"); //logs both
facade.Success("success"); // logs only to console
```
\$\endgroup\$
4
  • \$\begingroup\$ Thanks for your answer I'm gradually reading through it. When you say that I'm not even using type yes that's correct, usually only for exceptions but that's in the stack trace anyway. How bad is class-based logging compared to one logger for the entire application? \$\endgroup\$ Commented Feb 13, 2020 at 6:20
  • \$\begingroup\$ @AshKetchum well it Is not bad per se. But it may get annoying to have to instantiate new logger everytime you instantiate an object which may want to log something. You should at least again follow the decorator pattern to create a logger that knows another logger And a type And Will prepend the type name to every message that Is to be logged before delegating the message to the inner logger. The same goes for prepending the message with a timestamp. \$\endgroup\$ Commented Feb 13, 2020 at 10:47
  • \$\begingroup\$ It Is btw a very nice example of decorator pattern because it literally decorates the message before saving it. \$\endgroup\$ Commented Feb 13, 2020 at 10:48
  • \$\begingroup\$ Another option might be to not use the generic logger facade And instead define class specific facades with methods encapsulating not only log type but also the log message together with params used to build the message. MyClassLoggerFacade.ThisSucceeded(input) or MyClassLoggerFacade.ThatFailed(error), etc. Although it May be wise to use domain And not the class. I.e. HttpClientLogger And the actual http client implementation using it Is not that important. \$\endgroup\$ Commented Feb 13, 2020 at 11:05

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.