Reusable methods
Notice that you have several methods that essentially do the same thing. Log
and both writeCustomErrorLog
methods follow the same approach:
StreamWriter sw = null;
try
{
sw = new StreamWriter("some file path", true);
sw.WriteLine("some string");
sw.Flush();
sw.Close();
catch (Exception ex)
{
throw ex;
}
This can easily be put into a reusable method:
private void WriteMessageToFile(string filepath, string message)
{
StreamWriter sw = null;
try
{
sw = new StreamWriter(filepath, true);
sw.WriteLine(message);
sw.Flush();
sw.Close();
catch (Exception ex)
{
throw ex;
}
}
And then your methods become much simpler and less repetitive:
public void writeCustomErrorLog(string location, string message,string fileName)
{
string filePath = location + "\\" + fileName +".txt";
string logMessage = DateTime.Now.ToString() + " : " + "\n" + message;
WriteMessageToFile(filePath, logMessage);
}
Catching and throwing
try
{
// ...
}
catch (Exception ex)
{
throw ex;
}
- Catching an exception should be done when you want to handle the exception.
- Throwing an exception should be done when you don't want to handle the exception (or raise a new exception)
There is no point to catching an exception, only to then throw it. That is essentially like queueing at a supermarket checkout when you don't have any items you want to buy. There's no point to doing so.
The try/catch can be removed altogether in these cases. Note that the try/catch could be useful if you actually did something with it:
try
{
// ...
}
catch (Exception ex)
{
Trace.Write("Exception occurred here!");
throw;
}
Note the different between throw ex;
and throw;
.
throw ex;
resets the stacktrace to where you call throw ex;
. This effectively removes deeper stack trace information
throw;
retains the stack trace of the exception as it was initially raised. This does not remove data.
Throwing an existing exception is rarely a good idea.
Hardcoding
AppDomain.CurrentDomain.BaseDirectory + "\\Log.txt"
AppDomain.CurrentDomain.BaseDirectory + "\\PassiveLog.txt"
Don't hardcode your location, nor your filename.
You intend to use this in future projects. It's not unforeseeable that you're going to want to decide where to put the file, or what you should name it (especially if you want to use more than one log file in the same application).
Also, I have no idea what a "passive log" is. The name isn't very descriptive.
String concatenation
DateTime.Now.ToString() + " : " + "\r\nSource " + ex.Source.ToString().Trim() +
"\r\nMessage : " + ex.Message.ToString().Trim() +
"\r\nInner Exceptions: " + Convert.ToString(ex.InnerException) +
"\r\nException thrown from: " + getExceptionGeneratedMethod(ex) +
"\r\nLine Number: " + getExceptionGeneratedLineNumber(ex) +
"\r\nStack Trace: " + ex.StackTrace.ToString()
Do not concatenate strings with +
.
Strings are immutable. That means that you can't change a string, you can only create a new string (and the old one will be discarded.
Let's take a simple example:
"a" + "b" + "c" + "d"
These concatenations are done step by step. Look at the needed memory allocation:
- Create a 1 character string (a)
- Create a 1 character string (b)
- Create a 2 character string (a+b)
- Create a 1 character string (c)
- Create a 3 character string (ab+c)
- Create a 1 character string (d)
- Create a 4 character string (abc+d)
To concatenate 4 strings, you're had to allocate 7 strings.
For a 4 character result, you've effectively allocated 13 characters' worth of strings.
This problem quickly grows out of proportion and leads to bad performance, massive memory usages, and eventual OutOfMemoryException
s being thrown. Furthermore, avoiding this is very simple:
Option 1 - String.Format
This is more useful for cases where you are replacing a fixed set of placeholders.
string result = String.Format("{0} is the father of {1}", nameOfFather, nameOfChild);
Note that interpolated strings are a fairly recent addition that simplify the syntax:
string result = $"{nameOfFather} is the father of {nameOfChild}";
Option 2 - StringBuilder
This is more useful for cases where you dynamically generate a string, instead of replacing a fixed set of placeholders:
StringBuilder sb = new StringBuilder();
sb.Append(nameOfFather);
sb.Append(" is the father of ");
sb.Append(nameOfChild);
string result = sb.ToString();
I repeat again: Do not concatenate strings with +
. Especially since interpolated strings have been added; there's no reasonable "simply syntax" argument to using +
concatenation.
Addendum
Piedar made a really interesting remark in the comments. Apparently, string + string
concatenation is now converted to String.Concat
by the compiler. This effectively negates the issue I'm pointing at.
However, I still urge you to not do +
concatenation on big string anyway, from a readability perspective. value + " seconds"
is readably enough, but you're pasting a lot of different things together and the code starts looking bulky and ugly.
However, this is a style (and, in extreme cases, readability) argument, not a technical one.
Statics
Everything you've listed is static. Why?
This issue is strongly related to why you hardcoded your values. You think that your single log is a catch-all for any future application's needs and you'll never need to tweak it or change it. You're effectively making it so that a future consumer of your library has no configuration options.
This is inherently bad design. This is equivalent to Word being able to save files in a single (hardcoded) directory, or Firefox telling you what your homepage should be without alowing you to change it.
Good applications (and libraries) give control to the user, they don't tell the user what to do.
Note: there is nothing wrong with providing a default behavior, but you need to give your user the option of changing the default behavior if they so choose.
Separation of concerns
Other than not allowing the user to change key settings, you're also handling too much here. Your one class is handling many responsibilities:
- Deciding where to put the file
- Accessing and writing to the file
- Deciding how to format the log messages
These things should ideally be split into separate classes.
I understand that the need for separating this into classes is not that great at the moment. However, once you implement the other suggested changes (configuration options), your code will quickly grow to a size where the need for separation becomes more apparent.
Ideally, this is a better logging approach:
LogFile
allows you to create a log with a specific configuration (filename, location)
ExceptionLogMessageFormatter
takes an exception and turns it into a log message (string).
Logger
takes a pre-formatted string message and writes it to a pre-configured logfile.
You're currently only logging exceptions, but the need for separation becomes more apparent when you want to also log other objects. If you want to e.g. log an object of type Person
, you'll create a separate PersonLogMessageFormatter
class, but you will be able to reuse the same LogFile
class (a different file with different settings, but the same class in code!) and Logger
class (which logs the string message to this different log).
Don't reinvent the wheel!
This is the most important lesson to draw here. However, it does come with a caveat, which I will mention at the end.
What you've built here is a fairly simple tool, for a problem that is not new to the field of software development. (That's not an insult - in case you interpreted it as such).
Odds are that someone is already going to have created a library to handle this exact job. And they have:
These tools are very well developed, offer an incredibly amount of configuratbility, and are able to handle fringe cases that you probably haven't even thought of but will eventually have to implement a solution for.
Personally, I'm a big fan of NLog. I've never come across a need for logging that could not be handled by NLog. I like how it works, and I find that it gives me a perfect balance of good default behavior and exceedingly configurable custom behavior if I need it.
If you were going to write a story about a world based on medieval fantasy with magic and multiple humanoid races; I would suggest that you watch e.g. the Lord of the Rings movies (or read the books of course).
Even if your story is going to be different, it gives you a nice overview of the ins and outs of your story's setting, pitfalls to avoid, how to create interesting an immersive plots, ...
Similarly, if you want to write a logging framework, I suggest that you first try working with existing tools.
Even if your tool is going to do something new; seeing the existing framework gives you a good overview of what features you're going to need to add to your tool to make it user friendly, and it helps you draw the line on how much configuration options you need to add to it to fulfill most needs.
Caveat
However, writing your own logging framework from scratch (with no knowledge of existing frameworks) can be rewarding in and of itself as a training exercise.
Based on your code, I surmise that you are a beginner, at least to creating reusable libraries. So if writing this library benefits you for training purposes, don't let my review stop you.
But if your only goal is to create a functionally usable tool, then I do suggest first looking for existing solutions, either to already solve your problem or even just to get some inspiration/ideas.