Skip to main content
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

You're still omitting braces:

You're still omitting braces:

Source Link
Mathieu Guindon
  • 75.6k
  • 18
  • 194
  • 468

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.