3
\$\begingroup\$

I created a small logging class library and wanted you guys to check it out, I used to use Log4Net but thought I could create something more user friendly that was more suitable for my project than Log4Net was, don't get me wrong Log4Net is an amazon library but it can just be hard to edit to your personal preference at times.

What do I want to achieve by posting this question?

  • Improoving the classes below.
  • Is the method WriteContentToFile() thread safe, if not how can it be?


LogManager.cs

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

namespace Sahara.Sahara.Core.Log
{
    class LogManager
    {
        /// <summary>
        /// Holds the logging settings class.
        /// </summary>
        private LogSettings logSettings;

        /// <summary>
        /// Initializes a new instance of LogManager.
        /// </summary>
        public LogManager()
        {
            this.logSettings = new LogSettings
            {
                TypesToSave = new List<LogType>(),
                LogFile = "Logs/line_logs.txt"
            };
        }

        /// <summary>
        /// Prints a line to the console using custom settings.
        /// </summary>
        /// <param name="line">Line you want to print.</param>
        /// <param name="logType">Type of line you want to print.</param>
        public void Log(string line, LogType logType)
        {
            ConsoleColor existingColor = Console.ForegroundColor;
            Console.WriteLine(GetLogDateTime() + line);
            Console.ForegroundColor = existingColor;

            if (logSettings.TypesToSave.Contains(logType))
                WriteContentToFile(logSettings.LogFile, line);
        }

        /// <summary>
        /// Retruns a short string with the current time in brackets
        /// </summary>
        /// <returns></returns>
        private string GetLogDateTime()
        {
            return "[" + DateTime.Now.ToLongTimeString() + "] ";
        } 

        /// <summary>
        /// Adds text to the log file.
        /// </summary>
        /// <param name="filePath">The path of the log file.</param>
        /// <param name="content">The text to write in the file.</param>
        private void WriteContentToFile(string filePath, string content)
        {
            try
            {
                using (FileStream fileStream = new FileStream(filePath, FileMode.Append, FileAccess.Write, FileShare.Read))
                using (StreamWriter writer = new StreamWriter(fileStream, Encoding.Unicode))
                {
                    writer.Write(content);
                }
            }
            catch (Exception e)
            {
                Console.WriteLine(e.Message);
                Console.WriteLine(e.StackTrace);
            }
        }
    }
}

LogSettings.cs

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

namespace Sahara.Sahara.Core.Log
{
    class LogSettings
    {
        /// <summary>
        /// Type of LogTypes to save.
        /// </summary>
        public List<LogType> TypesToSave
        {
            get;
            set;
        }

        /// <summary>
        /// The file path to the log file.
        /// </summary>
        public string LogFile
        {
            get;
            set;
        }
    }
}

LogType.cs (ENUM)

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

namespace Sahara.Sahara.Core.Log
{
    enum LogType
    {
        /// <summary>
        /// Used for information and debugging.
        /// </summary>
        **Information = ConsoleColor.Gray,

        /// <summary>
        /// Used to log errors and exception details.
        /// </summary>
        Error = ConsoleColor.Red,

        /// <summary>
        /// Used to log warnings.
        /// </summary>
        Warning = ConsoleColor.Yellow
    }
}

Usage:

GetLogManager().Log("Hi, a message from the console.", LoggingType.Information);
\$\endgroup\$
  • 1
    \$\begingroup\$ Off topic but NLog is worth checking out. Highly configurable, extensible and modular with a good community. Very easy to make it just work right now \$\endgroup\$ – Gusdor Oct 12 '16 at 16:29
  • \$\begingroup\$ @Gusdor off-topic^2 NLog isn't bad but it's not good either. It's configuration is better then log4net's but still pretty absurd. \$\endgroup\$ – t3chb0t Oct 12 '16 at 16:41
  • 1
    \$\begingroup\$ You could also try System.Diagnostics.TraceSource for logging. I try to err on the side of using something in the framework before I roll my own or pull in additional dependencies. \$\endgroup\$ – Dan Lyons Oct 12 '16 at 18:05
3
\$\begingroup\$

LogManager

  • Because private LogSettings logSettings; won't change after created in the constructor you should make it readonly.

  • Log() is doing too much. It's composing the message to log, Changing the Console color, output to console. Better extract the console stuff and message stuff to separate methods.

  • Omitting braces {} although they might be optional is a dangorous path to walk. I would like to encourage you to use them always which leads in less error-prone code.

  • documentation is good as long as it doesn't tell the obvious. Having

    /// <summary>
    /// Initializes a new instance of LogManager.
    /// </summary>
    public LogManager()  
    

    won't buy you anything, because any developer knows what a constructor is for.

  • Hardcoding LogSettings in the constructor isn't that good either. Assume a user of this class wants to log somewhere else or just want to read the log. How should he/she knows where the log is created ? Passing it into the constructor, maybe as an interface instead of an object, would be the better way.

\$\endgroup\$
2
\$\begingroup\$
    private string GetLogDateTime()
    {
        return "[" + DateTime.Now.ToLongTimeString() + "] ";
    }

The user should be able to define whether he wans to log local or utc time. If there is any doubt then I would log the utc.

Another issue here is that you don't log the timestamp as invariant. Its format will change between EN and DE. If you will need to import/compare logs from two different worlds it will be really hard.


    public void Log(string line, LogType logType)
    {
        ConsoleColor existingColor = Console.ForegroundColor;
        Console.WriteLine(GetLogDateTime() + line);
        Console.ForegroundColor = existingColor;

        if (logSettings.TypesToSave.Contains(logType))
            WriteContentToFile(logSettings.LogFile, line);
    }

You need to split it into configurable modules so the user can decide whether he wants to log to the console and/or to the file. Currently he has no choice and if he uses your library in a console project it will destroy his console output by writing some logs there.

There are other issues but @Heslacher has already mentioned them.

\$\endgroup\$
  • \$\begingroup\$ Custom date formatting sounds a lot like a feature, not a code correction. Using the invariant culture is a sound suggestion. \$\endgroup\$ – Gusdor Oct 12 '16 at 16:27
  • 2
    \$\begingroup\$ @Gusdor but I didn't suggest any custom date formatting. I only said that with the current implementation the timestamp will be different in the EN and the DE cultures and that for logging this is really bad. \$\endgroup\$ – t3chb0t Oct 12 '16 at 16:36

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.