4
\$\begingroup\$

I've asked a question about this particular code before, although it was a little different back then. I took the suggestions from people who replied to my question and decided to post a follow up to that question, in attempt to improve the class even more.

Simple console logger used instead of packages like log4net and nlog for minimalism.

public class ConsoleLogger <TClass> : ILogger
{
    private static readonly string FilePath = Path.GetDirectoryName(System.Reflection.Assembly.GetExecutingAssembly().Location + "/resources/logging/");

    private readonly Type _className;

    public ConsoleLogger()
    {
        _className = typeof(TClass);
    }

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

    public void Warn(string message, bool logToFile = false)
    {
        Log(message, ConsoleColor.Yellow, logToFile);
    }

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

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

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

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

    private void Log(string message, ConsoleColor color, bool logToFile = false)
    {
        var oldColor = Console.ForegroundColor;
        Console.ForegroundColor = color;
        Console.WriteLine($"[{DateTime.Now:MM/dd HH:mm:ss}] " + message);
        Console.ForegroundColor = oldColor;

        string logFile;

        switch (color)
        {
            case ConsoleColor.Yellow:
                logFile = "error.log";
                break;
            case ConsoleColor.Cyan:
                logFile = "debug.log";
                break;
            case ConsoleColor.Red:
                logFile = "error.log";
                break;
            default:
                logFile = "trace.log";
                break;
        }

        if (logToFile)
        {
            LogToFile(logFile, $"Occurred at [{DateTime.Now:MM/dd HH:mm:ss}] in [{_className.FullName}]: " + message);
        }
    }

    private static void LogToFile(string file, string content)
    {
        try
        {
            using (var fileWriter = new StreamWriter(FilePath + file, true))
            {
                fileWriter.WriteLine(content);
                fileWriter.Close();
            }
        }
        catch (DirectoryNotFoundException)
        {
            Directory.CreateDirectory(FilePath);
        }
        catch (FileNotFoundException)
        {
            File.Create(file);
        }
    }
}

ILogger interface:

public interface ILogger
{
    void Trace(string message, bool logToFile = false);
    void Warn(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 Error(Exception e, bool logToFile = true);
}
\$\endgroup\$
4
  • 2
    \$\begingroup\$ You might be interested in merging your accounts. \$\endgroup\$
    – t3chb0t
    May 28, 2018 at 6:56
  • \$\begingroup\$ what you want ? \$\endgroup\$ May 28, 2018 at 7:23
  • \$\begingroup\$ Should it be Thtread-Safe, are you going to use it in multithreaded applications? TraceSource /TraceSwitch and TextWriterTraceListener already do the job. \$\endgroup\$ May 28, 2018 at 11:28
  • \$\begingroup\$ Yes, I need it to be multithreaded. As I said I want to write my own class for logging. \$\endgroup\$ May 28, 2018 at 12:46

1 Answer 1

5
\$\begingroup\$

Quickfire review

  • I see no reason to make this a generic class. You're effectively using the generic type as a name. While having intellisense is nice, keep in mind that the compiler will generate an optimized class for every type you use in your code. E.g. if TClass can either be Foo, Bar or Baz in your codebase; then the compiler created three different ConsoleLogger classes in order to optimize the assembly (actually it creates four, but the point still stands)
    • Since you're going to cast your generic type to a Type object anyway, it's better to just pass the Type object via the constructor. Functionally equivalent, minor difference in syntax which is ultimately negligible, lessened impact on the compiled assembly.
    • Personally, I would store the string name (of the type) directly instead of the Type itself, but that's a matter of style.
  • It seems needlessly repetitive to specify a writeToFile boolean for every message that is logged. I suggest making it a public property or adding it to the constructor so you only have to set it once.
  • Use Path.Combine(folder, file) instead of folder + file. This has some extra logic to make sure that the appropriate slashes are present and not duplicated. Note that you can use more than two parameters: Path.Combine(folder, subfolder, anotherSubFolder, file)
  • $"Occurred at [{DateTime.Now:MM/dd HH:mm:ss}] in [{_className.FullName}]: " + message You use interpolation for the first two values (good), but then concatenate the third value. You should interpolate that one as well $"Occurred at [{DateTime.Now:MM/dd HH:mm:ss}] in [{_className.FullName}]: {message}"
  • In the comments, you mention that it's supposed to be threadsafe, but I see no actual implementation of thread safety in any of the posted code. Are you using separate loggers per thread? (that's different from wanting multple threads to use the same logger).

The rest will be addressed in separate chapters.

Conflated logic

switch (color)
{
    case ConsoleColor.Yellow:
        logFile = "error.log";
        break;
    case ConsoleColor.Cyan:
        logFile = "debug.log";
        break;
    case ConsoleColor.Red:
        logFile = "error.log";
        break;
    default:
        logFile = "trace.log";
        break;
    }

This is not great. What you're doing here is determining the file to write to based on the color you want the log to be written in.

Imagine if Word automatically saved your document to a different location when you change the text color. No one would like that feature.

What you've done here is merged two separate design decisions:

  • Where do I want to make my log?
  • What do I want my log to look like?

These questions have nothing to do with each other, but you've merged them. That being said, you're of course going to have to map both behaviors, but not like this.

Create a new enum:

public enum LogType { Success, Warn, Error, Debug }

Change your method call:

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

Create separate mappings. There are a few ways to do this. For simple mappings, you can use dictionaries; but a method with a switch gives you more direct control if you need to apply some custom logic in certain cases.

public Dictionary<LogType, string> FilenameForLogType = new Dictionary<LogType, string>() {
    {LogType.Success , "trace.log"},
    {LogType.Warn ,    "error.log"},
    {LogType.Error ,   "error.log"},
    {LogType.Debug ,   "debug.log"},
};

public Dictionary<LogType, ConsoleColor> ColorForLogType = new Dictionary<LogType, ConsoleColor>() {
    {LogType.Success , ConsoleColor.Green},
    {LogType.Warn ,    ConsoleColor.Yellow},
    {LogType.Error ,   ConsoleColor.Red},
    {LogType.Debug ,   ConsoleColor.Cyan},
};

And then your Log() method can use these dictionaries based on the LogType you've passed in:

private void Log(string message, LogType logType, bool logToFile = false)
{
    // ...

    Console.ForegroundColor = ColorForLogType[logType];

    // ...

    if (logToFile)
    {
        LogToFile(FilenameForLogType[logType],  /* ... */);
    }
}

The main benefit here is that if you ever decide to change any of the colors or file names; then you won't need to actually care about the file names or colors (respectively).


Writing to file

private static void LogToFile(string file, string content)
{
    try
    {
        using (var fileWriter = new StreamWriter(FilePath + file, true))
        {
            fileWriter.WriteLine(content);
            fileWriter.Close();
        }
    }
    catch (DirectoryNotFoundException)
    {
        Directory.CreateDirectory(FilePath);
    }
    catch (FileNotFoundException)
    {
        File.Create(file);
    }
}

There are a few things to remark here.

Handling exceptions.

It doesn't make sense for you to create the directory/file after you've tried to write to it.

Suppose you do this:

for(int i = 0; i < 10; i++)
    myLogger.Trace(i.ToString(), true);

The console will show:

0
1
2
3
4
5
6
7
8
9

But the file will show:

1
2
3
4
5
6
7
8
9

You've swallowed the first log message (only when writing to disk!), because the file did not exist, you did not create it, and then you didn't try to write it again.

A better approach is to first check if the file/directory exists, create it if it doesn't, and then log the file.

private static void LogToFile(string file, string content)
{
    string fullPath = Path.Combine(FilePath, file);

    if(!Directory.Exists(FilePath))
         Directory.CreateDirectory(FilePath);

    if(!File.Exists(fullPath))
         File.Create(fullPath);

    using (var fileWriter = new StreamWriter(fullPath, true))
    {
        fileWriter.WriteLine(content);
        fileWriter.Close();
    }
}

However, this point may be moot if you read the following suggestion. But it's important to understand that your approach had a bug in it.

Streams COULD be your friend.

If you're going to use a stream to write to file, then you should leverage the benefits of using a stream.

Currently, you are opening a new stream every time you write a message to a log file. Streams make more sense when you repeatedly write data to the same stream, and only close it once you're done with it.

Note that this can be dependent on what you're logging.

  • A log that catches an exception now and then will not receive much use, and keeping the stream open may be more harmful than beneficial.
  • A log that catches all the trace messages of a particular processing job is going to receive a high volume of messages in a short time. Streams make sense here.

There are many ways to solve this, without resorting to opening a stream for every message:

  • A ConsoleLogger opens a stream when the first message is written, but then holds on to that stream until it is manually told to close it, or the logger gets disposed himself.
  • An intermediary StreamHandler is created. A ConsoleLogger simply requests a stream from StreamHandler, which is able to decide (by itself) when it wants to cache stream and when it should create a new one. This allows you to e.g. not keep an error log stream open, but keep a trace log stream open for an extended time.
  • You can create your own StreamWriter (or use a StreamHandler) to implement a timeout feature, where streams are automatically closed when they haven't received a message in the last X seconds. This is nice, but not easy to implement.

You can make this as complex as you want it to be. I'll suggest the simplest improvement (the first option).

First, make a cache for your streams:

private Dictionary<string, StreamWriter> _streams = new Dictionary<string, StreamWriter>();

When writing to a file, check if you already have a streamwriter for the file, otherwise create it:

private static void LogToFile(string file, string content)
{
    string fullPath = Path.Combine(FilePath, file);

    //...

    if (!_streams.ContainsKey(file)) 
    {
        _streams.Add(file, new StreamWriter(fullPath , true));
    }

    _streams[file].WriteLine(content);
}

Don't stop yet. We still need to handle closing the stream! I prefer giving two options to consumers:

  • Manually close the streams. Should they write another message afterwards, then it will automatically open a new stream.
  • Automatically close the streams if the logger is disposed of.

Manually closing them can be done as follows:

public void CloseStreams()
{
     //close all streams
     foreach(var kvp in _streams)
         kvp.Value.Close();

     //empty the cache
     _streams.Clear();
}

Automatically closing them can rely on the same method; but it gets triggered by the IDisposable interface:

public class ConsoleLogger <TClass> : ILogger, IDisposable
{
    public void Dispose()
    {
       CloseStreams();
    }
}

Note that this allows you to use the using statement with your logger class:

using(var logger = new ConsoleLogger<Foo>());
{
     logger.Debug("Let's go!");
     MethodWhichAlsoLogsThings(logger);
     logger.Debug("Done! Streams will be closed automatically!");
}

//At this point, the streams are closed.
\$\endgroup\$
9
  • \$\begingroup\$ Thank you for this, I went through it all and gave my class some refactoring. Couple of things I wanted to discuss with you. The reason I didn't check files and directories exist like that as I thought it could be a performance/speed issue, that it would be a bad practice to check it on every log, but i guess that my only real option is what you've suggested. \$\endgroup\$ May 28, 2018 at 13:11
  • \$\begingroup\$ One more thing, you suggested not making a new class instance for each class that needs a logger? What would you suggest, just making it a static class? I don't think you spoke on that. \$\endgroup\$ May 28, 2018 at 13:11
  • \$\begingroup\$ @LeeNelson: You need to only check it before opening a stream. If you keep the same stream open, the file cannot be deleted (since the stream locks the file while it is open). Also, while I can understand (but not necessarily agree with) the idea of only handling an exception when it arises, you still should have actually written the message again after fixing the issue. \$\endgroup\$
    – Flater
    May 28, 2018 at 13:13
  • \$\begingroup\$ @LeeNelson: As to your second comment; I don't recall talking about that. I only referred to using the same stream multiple times (from the same logger, not other ones). If you're referring to my comment on the multithreading, that was a question directed at you. I can't actually answer this as there is no multithreading code in your question. \$\endgroup\$
    – Flater
    May 28, 2018 at 13:14
  • \$\begingroup\$ So you're saying only check the files and directories before initiating a new stream? Let me know on that one. My second comment was about your "Since you're going to cast your generic type to a...." comment, I didn't really understand what you were trying to say. \$\endgroup\$ May 28, 2018 at 13:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.