1
\$\begingroup\$

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();
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ This might help you, don't look at the accepted answer but at the second one. stackoverflow.com/questions/2954900/… The context is logging in a multithread app but I found the producer/consumer pattern very interesting. \$\endgroup\$ – Guillaume Beauvois Feb 2 '17 at 16:10
  • \$\begingroup\$ Definitely something to consider, thank you for posting this! \$\endgroup\$ – Ashkru Feb 2 '17 at 16:15
  • \$\begingroup\$ If you're just doing it to learn, that's fine. For production code, I would recommend using NLog instead of rolling your own logger. \$\endgroup\$ – default.kramer Feb 2 '17 at 19:19
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Its not really a logging system where its going to be going to the public. Its mainly for my eyes only and it only needs to log simple things that appear on the console. I don't really see anything being majorly changed right now as I built it for what I need. But thank you for taking the time to write a comment, it sure has given me some ideas. \$\endgroup\$ – Ashkru Feb 2 '17 at 15:40
  • \$\begingroup\$ @Ashkru I don't really see anything being majorly changed right now as I built it for what I need. then you shouldn't call it a logger, this is misleading, but rather a console-logger and clearly state it's purpose. \$\endgroup\$ – t3chb0t Feb 2 '17 at 18:25
1
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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