Simple Logging Class

Question

I can coded a simple logging class in C#, my main aim for this is to make it easily usable, fast at logging and saving, and to handle pretty much everything that errors. Overall I am just looking for advice if this is a good logging setup, and how I can improve the code inside it.

I didn't want to manually save each error when it occurs to a outside file as I feel it will take up too much time between logs and if multiple errors are being logged it could cause an issue where the file is already in use. Could anyone share their views on this?

Thanks.

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Sirius.Core.Logging
{
    internal class LogManager : IDisposable
    {
        private bool _isDisposing;
        private readonly List<string> _logsToSave;
        private readonly string _errorLogsFile;

        public LogManager()
        {
            _isDisposing = false;
            _logsToSave = new List<string>();
            _errorLogsFile = "/logs/error_logs.txt";
        }

        public void Information(string log)
        {
            Log(LogType.Information, log);
        }

        public void Debug(string log)
        {
            Log(LogType.Debug, log);
        }

        public void Error(string log, Exception exception = null)
        {
            if (exception != null)
            {
                HandleException(exception);
            }

            Log(LogType.Error, log);
        }

        public void Warn(string log)
        {
            Log(LogType.Warning, log);
        }

        private static void Log(LogType logType, string log)
        {
            var oldConsoleColor = Console.ForegroundColor;
            SetColor(logType);
            Console.WriteLine(DateTime.Now.ToString("yyyyMMddHHmmss") + " [" + logType.ToString() + "] " + log);
            Console.ForegroundColor = oldConsoleColor;
        }

        private void HandleException(Exception exception)
        {
            _logsToSave.Add("Error occurred at " + DateTime.Now.ToString("yyyyMMddHHmmss"));
            _logsToSave.Add(exception.Message);
            _logsToSave.Add(exception.StackTrace);
            _logsToSave.Add(string.Empty);
        }

        private static void SetColor(LogType logType)
        {
            switch (logType)
            {
                default:
                case LogType.Information:
                    Console.ForegroundColor = ConsoleColor.White;
                    break;
                case LogType.Debug:
                    Console.ForegroundColor = ConsoleColor.Cyan;
                    break;
                case LogType.Error:
                    Console.ForegroundColor = ConsoleColor.Red;
                    break;
                case LogType.Warning:
                    Console.ForegroundColor = ConsoleColor.Yellow;
                    break;
            }
        }

        // Not sure if its a good idea saving on application close as delay
        // Maybe add a command while program is active to save logs while running?
        private void SaveLogs()
        {
            if (IsFileLocked(new FileInfo(_errorLogsFile))) // Check if file is in use or any other IO related issues.
            {
                return;
            }

            var stringBuilder = new StringBuilder();

            foreach (var errorLog in _logsToSave)
            {
                stringBuilder.AppendLine(errorLog);
            }

            File.WriteAllText(_errorLogsFile, stringBuilder.ToString());
        }

        protected virtual bool IsFileLocked(FileInfo file)
        {
            FileStream stream = null;

            try
            {
                stream = file.Open(FileMode.Open, FileAccess.Read, FileShare.None);
            }
            catch (IOException)
            {
                return true;
            }
            finally
            {
                stream?.Close();
            }

            return false;
        }

        public void Dispose()
        {
            if (_isDisposing)
            {
                return;
            }

            _isDisposing = true;

            SaveLogs();
        }
    }
}

Answer 1

The only problem with this is what happens after you've been logging things, and you want to switch to the 'Release' version of your app that has no console logging, and only logs, say, major errors?

What happens if you want to log to a different type of medium, say, a database?

In a logging situation, you should build on an interface along with your POCO (plain-old-CLR-object). You should have an ILogger interface, that way if I decide I want to kill logging in the release version of my app, I can simply implement an ILogger that doesn't do anything in any of the methods, if I want to log to a different medium, just build a new ILogger for that.

The ILogger should expose the basic logging messages, Debug, Error, etc. Then your LogManager : ILogger should implement those, that also means you can build a BufferedFileLogger : ILogger, or a MsSqlLogger : ILogger, OracleLogger : ILogger, the possibilities are endless.

As it stands, this code is not mockable, which is a very important aspect of a logging system.

Answer 2

I would suggest that it would be better to use WriteLine when writing to the file, instead of iterating through each string to copy and build a long string and writing the whole thing at once:

private void SaveLogs()
{
    if (IsFileLocked(new FileInfo(_errorLogsFile))) // Check if file is in use or any other IO related issues.
    {
        return;
    }

    using (StreamWriter sw = new StreamWriter(_errorLogsFile,false))
    {
        foreach (var errorLog in _logsToSave)
        {
            sw.WriteLine(errorLog);
        }
    }

}

You'll notice this gives you the option to Append instead of overwrite if necessary.

Also, the LogType enum is public, it would be more concise and easier to maintain to let the user just call Log directly, passing the appropriate enum value. This way, if you need a new LogType, you only need to add the value.

To change the console color I would think that a Dictinonary<LogType,Color> would help to clean things up as well.