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

• t3chb0t: I don't see any significant difference.
• Liam Hardy: Just clarifying the changes make sense and there no other unseen way of improving the code.

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.