HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

C# Logging System

Submitted by: @import:stackexchange-codereview··
0
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?

  • 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 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.