Logging System - Follow Up

Question

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);
            }
        }
    }
}

Answer 1

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.