patterncsharpMinor
C# Logging System
Viewed 0 times
loggingsystemstackoverflow
Problem
I created a small logging class library and wanted you guys to check it out, I used to use Log4Net but thought I could create something more user friendly that was more suitable for my project than Log4Net was, don't get me wrong Log4Net is an amazon library but it can just be hard to edit to your personal preference at times.
What do I want to achieve by posting this question?
LogManager.cs
```
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
namespace Sahara.Sahara.Core.Log
{
class LogManager
{
///
/// Holds the logging settings class.
///
private LogSettings logSettings;
///
/// Initializes a new instance of LogManager.
///
public LogManager()
{
this.logSettings = new LogSettings
{
TypesToSave = new List(),
LogFile = "Logs/line_logs.txt"
};
}
///
/// Prints a line to the console using custom settings.
///
/// Line you want to print.
/// Type of line you want to print.
public void Log(string line, LogType logType)
{
ConsoleColor existingColor = Console.ForegroundColor;
Console.WriteLine(GetLogDateTime() + line);
Console.ForegroundColor = existingColor;
if (logSettings.TypesToSave.Contains(logType))
WriteContentToFile(logSettings.LogFile, line);
}
///
/// Retruns a short string with the current time in brackets
///
///
private string GetLogDateTime()
{
return "[" + DateTime.Now.ToLongTimeString() + "] ";
}
///
/// Adds text to the log file.
///
/// The path of the log file.
What do I want to achieve by posting this question?
- Improoving the classes below.
- Is the method WriteContentToFile() thread safe, if not how can it be?
LogManager.cs
```
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
namespace Sahara.Sahara.Core.Log
{
class LogManager
{
///
/// Holds the logging settings class.
///
private LogSettings logSettings;
///
/// Initializes a new instance of LogManager.
///
public LogManager()
{
this.logSettings = new LogSettings
{
TypesToSave = new List(),
LogFile = "Logs/line_logs.txt"
};
}
///
/// Prints a line to the console using custom settings.
///
/// Line you want to print.
/// Type of line you want to print.
public void Log(string line, LogType logType)
{
ConsoleColor existingColor = Console.ForegroundColor;
Console.WriteLine(GetLogDateTime() + line);
Console.ForegroundColor = existingColor;
if (logSettings.TypesToSave.Contains(logType))
WriteContentToFile(logSettings.LogFile, line);
}
///
/// Retruns a short string with the current time in brackets
///
///
private string GetLogDateTime()
{
return "[" + DateTime.Now.ToLongTimeString() + "] ";
}
///
/// Adds text to the log file.
///
/// The path of the log file.
Solution
LogManager
-
Because
-
-
Omitting braces
-
documentation is good as long as it doesn't tell the obvious. Having
won't buy you anything, because any developer knows what a constructor is for.
-
Hardcoding
-
Because
private LogSettings logSettings; won't change after created in the constructor you should make it readonly. -
Log() is doing too much. It's composing the message to log, Changing the Console color, output to console. Better extract the console stuff and message stuff to separate methods. -
Omitting braces
{} although they might be optional is a dangorous path to walk. I would like to encourage you to use them always which leads in less error-prone code. -
documentation is good as long as it doesn't tell the obvious. Having
///
/// Initializes a new instance of LogManager.
///
public LogManager()won't buy you anything, because any developer knows what a constructor is for.
-
Hardcoding
LogSettings in the constructor isn't that good either. Assume a user of this class wants to log somewhere else or just want to read the log. How should he/she knows where the log is created ? Passing it into the constructor, maybe as an interface instead of an object, would be the better way.Code Snippets
/// <summary>
/// Initializes a new instance of LogManager.
/// </summary>
public LogManager()Context
StackExchange Code Review Q#143982, answer score: 3
Revisions (0)
No revisions yet.