1
\$\begingroup\$

Previous question: C# Logging System

I have took your answers on board and added the following changes to my LogManager.cs class:

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

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

        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;
            PrintLine(line);
            Console.ForegroundColor = existingColor;

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

        /// <summary>
        /// Prints a line to the console.
        /// </summary>
        /// <param name="line">Line to print to the console.</param>
        private void PrintLine(string line)
        {
            Console.WriteLine(GetLogDateTime() + 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);
            }
        }
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ I don't see any significant difference. \$\endgroup\$ – t3chb0t Oct 12 '16 at 17:03
  • \$\begingroup\$ Just clarifying the changes make sense and there no other unseen way of improving the code. \$\endgroup\$ – Liam Hardy Oct 12 '16 at 17:10
3
\$\begingroup\$
ConsoleColor existingColor = Console.ForegroundColor;
PrintLine(line);
Console.ForegroundColor = existingColor;

This suggests something in PrintLine changes the Console.ForegroundColor... but nothing does, so why even bother with the foreground color at all?

Seems this would have the exact same effect:

PrintLine(line);

But let's pretend for a minute, that PrintLine takes an argument that does change the foreground color:

PrintLine(line, logType);

Whose job is it to cache the original ForegroundColor, and reset it? That's right, PrintLine should be responsible for that.

By abstracting the Console.WriteLine call behind a method call, you've introduced mixed abstraction levels in your Log method: you have low-level Console.ForegroundColor reads and writes, and a higher-level PrintLine call, in the same method: if Console.WriteLine is too low-level implementation details for Log, then so is Console.ForegroundColor.

Keep abstraction levels separated.


You're still omitting braces:

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

Should be:

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

XML comments are great, but completely useless on private members; private members don't show up in IntelliSense, so what's the use? At best, they're redundant - if you're looking at WriteContentToFile, you are looking at the source code. If the code needs such comments, you need to work on naming things properly - but WriteContentToFile, filePath and content are pretty darn crystal-clear.

Actually the XML comments are not so great:

/// <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>

This is wrong. It's printing a line to the console and writing that line to a file, using hard-coded settings. This would be more accurate:

/// <summary>
/// Writes a new log entry.
/// </summary>
/// <param name="line">The text content of the log entry.</param>
/// <param name="logType">The type/level of log entry.</param>

The notion of "printing" is an implementation detail that doesn't belong in the public interface or anywhere in the documentation: one day that method will be writing log entries to a database - would "printing" still apply?


I would say the other isses have already been pointed out in the previous iteration - and they're important issues too, and you haven't addressed them here.

  • Log timestamps are still culture-dependent.
  • Settings are still hard-coded in the constructor.
  • Using that logger in a console app will still wreck the app's output.
\$\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.