Logging System - Follow Up 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
    {
        /// &lt;summary&gt;
        /// Holds the logging settings class.
        /// &lt;/summary&gt;
        private readonly LogSettings logSettings;

        public LogManager()
        {
            this.logSettings = new LogSettings
            {
                TypesToSave = new List&lt;LogType&gt;(),
                LogFile = "Logs/line_logs.txt"
            };
        }

        /// &lt;summary&gt;
        /// Prints a line to the console using custom settings.
        /// &lt;/summary&gt;
        /// &lt;param name="line"&gt;Line you want to print.&lt;/param&gt;
        /// &lt;param name="logType"&gt;Type of line you want to print.&lt;/param&gt;
        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);
        }

        /// &lt;summary&gt;
        /// Prints a line to the console.
        /// &lt;/summary&gt;
        /// &lt;param name="line"&gt;Line to print to the console.&lt;/param&gt;
        private void PrintLine(string line)
        {
            Console.WriteLine(GetLogDateTime() + line);
        }

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

        /// &lt;summary&gt;
        /// Adds text to the log file.
        /// &lt;/summary&gt;
        /// &lt;param name="filePath"&gt;The path of the log file.&lt;/param&gt;
        /// &lt;param name="content"&gt;The text to write in the file.&lt;/param&gt;
        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);
            }
        }
    }
}
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:


/// &lt;summary&gt;
/// Prints a line to the console using custom settings.
/// &lt;/summary&gt;
/// &lt;param name="line"&gt;Line you want to print.&lt;/param&gt;
/// &lt;param name="logType"&gt;Type of line you want to print.&lt;/param&gt;



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:

/// &lt;summary&gt;
/// Writes a new log entry.
/// &lt;/summary&gt;
/// &lt;param name="line"&gt;The text content of the log entry.&lt;/param&gt;
/// &lt;param name="logType"&gt;The type/level of log entry.&lt;/param&gt;


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.