Quickfire review
- I see no reason to make this a generic class. You're effectively using the generic type as a name. While having intellisense is nice, keep in mind that the compiler will generate an optimized class for every type you use in your code. E.g. if
TClass can either be Foo, Bar or Baz in your codebase; then the compiler created three different ConsoleLogger classes in order to optimize the assembly (actually it creates four, but the point still stands)
- Since you're going to cast your generic type to a
Type object anyway, it's better to just pass the Type object via the constructor. Functionally equivalent, minor difference in syntax which is ultimately negligible, lessened impact on the compiled assembly.
- Personally, I would store the string name (of the type) directly instead of the
Type itself, but that's a matter of style.
- It seems needlessly repetitive to specify a
writeToFile boolean for every message that is logged. I suggest making it a public property or adding it to the constructor so you only have to set it once.
- Use
Path.Combine(folder, file) instead of folder + file. This has some extra logic to make sure that the appropriate slashes are present and not duplicated. Note that you can use more than two parameters: Path.Combine(folder, subfolder, anotherSubFolder, file)
$"Occurred at [{DateTime.Now:MM/dd HH:mm:ss}] in [{_className.FullName}]: " + message You use interpolation for the first two values (good), but then concatenate the third value. You should interpolate that one as well $"Occurred at [{DateTime.Now:MM/dd HH:mm:ss}] in [{_className.FullName}]: {message}"- In the comments, you mention that it's supposed to be threadsafe, but I see no actual implementation of thread safety in any of the posted code. Are you using separate loggers per thread? (that's different from wanting multple threads to use the same logger).
The rest will be addressed in separate chapters.
Conflated logic
switch (color)
{
case ConsoleColor.Yellow:
logFile = "error.log";
break;
case ConsoleColor.Cyan:
logFile = "debug.log";
break;
case ConsoleColor.Red:
logFile = "error.log";
break;
default:
logFile = "trace.log";
break;
}
This is not great. What you're doing here is determining the file to write to based on the color you want the log to be written in.
Imagine if Word automatically saved your document to a different location when you change the text color. No one would like that feature.
What you've done here is merged two separate design decisions:
- Where do I want to make my log?
- What do I want my log to look like?
These questions have nothing to do with each other, but you've merged them. That being said, you're of course going to have to map both behaviors, but not like this.
Create a new enum:
public enum LogType { Success, Warn, Error, Debug }
Change your method call:
public void Warn(string message, bool logToFile = false)
{
Log(message, LogType.Warn, logToFile);
}
Create separate mappings. There are a few ways to do this. For simple mappings, you can use dictionaries; but a method with a switch gives you more direct control if you need to apply some custom logic in certain cases.
public Dictionary<LogType, string> FilenameForLogType = new Dictionary<LogType, string>() {
{LogType.Success , "trace.log"},
{LogType.Warn , "error.log"},
{LogType.Error , "error.log"},
{LogType.Debug , "debug.log"},
};
public Dictionary<LogType, ConsoleColor> ColorForLogType = new Dictionary<LogType, ConsoleColor>() {
{LogType.Success , ConsoleColor.Green},
{LogType.Warn , ConsoleColor.Yellow},
{LogType.Error , ConsoleColor.Red},
{LogType.Debug , ConsoleColor.Cyan},
};
And then your Log() method can use these dictionaries based on the LogType you've passed in:
private void Log(string message, LogType logType, bool logToFile = false)
{
// ...
Console.ForegroundColor = ColorForLogType[logType];
// ...
if (logToFile)
{
LogToFile(FilenameForLogType[logType], /* ... */);
}
}
The main benefit here is that if you ever decide to change any of the colors or file names; then you won't need to actually care about the file names or colors (respectively).
Writing to file
private static void LogToFile(string file, string content)
{
try
{
using (var fileWriter = new StreamWriter(FilePath + file, true))
{
fileWriter.WriteLine(content);
fileWriter.Close();
}
}
catch (DirectoryNotFoundException)
{
Directory.CreateDirectory(FilePath);
}
catch (FileNotFoundException)
{
File.Create(file);
}
}
There are a few things to remark here.
Handling exceptions.
It doesn't make sense for you to create the directory/file after you've tried to write to it.
Suppose you do this:
for(int i = 0; i < 10; i++)
myLogger.Trace(i.ToString(), true);
The console will show:
0
1
2
3
4
5
6
7
8
9
But the file will show:
1
2
3
4
5
6
7
8
9
You've swallowed the first log message (only when writing to disk!), because the file did not exist, you did not create it, and then you didn't try to write it again.
A better approach is to first check if the file/directory exists, create it if it doesn't, and then log the file.
private static void LogToFile(string file, string content)
{
string fullPath = Path.Combine(FilePath, file);
if(!Directory.Exists(FilePath))
Directory.CreateDirectory(FilePath);
if(!File.Exists(fullPath))
File.Create(fullPath);
using (var fileWriter = new StreamWriter(fullPath, true))
{
fileWriter.WriteLine(content);
fileWriter.Close();
}
}
However, this point may be moot if you read the following suggestion. But it's important to understand that your approach had a bug in it.
Streams COULD be your friend.
If you're going to use a stream to write to file, then you should leverage the benefits of using a stream.
Currently, you are opening a new stream every time you write a message to a log file. Streams make more sense when you repeatedly write data to the same stream, and only close it once you're done with it.
Note that this can be dependent on what you're logging.
- A log that catches an exception now and then will not receive much use, and keeping the stream open may be more harmful than beneficial.
- A log that catches all the trace messages of a particular processing job is going to receive a high volume of messages in a short time. Streams make sense here.
There are many ways to solve this, without resorting to opening a stream for every message:
- A
ConsoleLogger opens a stream when the first message is written, but then holds on to that stream until it is manually told to close it, or the logger gets disposed himself.
- An intermediary
StreamHandler is created. A ConsoleLogger simply requests a stream from StreamHandler, which is able to decide (by itself) when it wants to cache stream and when it should create a new one. This allows you to e.g. not keep an error log stream open, but keep a trace log stream open for an extended time.
- You can create your own
StreamWriter (or use a StreamHandler) to implement a timeout feature, where streams are automatically closed when they haven't received a message in the last X seconds. This is nice, but not easy to implement.
You can make this as complex as you want it to be. I'll suggest the simplest improvement (the first option).
First, make a cache for your streams:
private Dictionary<string, StreamWriter> _streams = new Dictionary<string, StreamWriter>();
When writing to a file, check if you already have a streamwriter for the file, otherwise create it:
private static void LogToFile(string file, string content)
{
string fullPath = Path.Combine(FilePath, file);
//...
if (!_streams.ContainsKey(file))
{
_streams.Add(file, new StreamWriter(fullPath , true));
}
_streams[file].WriteLine(content);
}
Don't stop yet. We still need to handle closing the stream! I prefer giving two options to consumers:
- Manually close the streams. Should they write another message afterwards, then it will automatically open a new stream.
- Automatically close the streams if the logger is disposed of.
Manually closing them can be done as follows:
public void CloseStreams()
{
//close all streams
foreach(var kvp in _streams)
kvp.Value.Close();
//empty the cache
_streams.Clear();
}
Automatically closing them can rely on the same method; but it gets triggered by the IDisposable interface:
public class ConsoleLogger <TClass> : ILogger, IDisposable
{
public void Dispose()
{
CloseStreams();
}
}
Note that this allows you to use the using statement with your logger class:
using(var logger = new ConsoleLogger<Foo>());
{
logger.Debug("Let's go!");
MethodWhichAlsoLogsThings(logger);
logger.Debug("Done! Streams will be closed automatically!");
}
//At this point, the streams are closed.
