Custom error-logging

Question

I'm writing a static class to log my application errors into a text file. I am planning on using this as a library on any application I intend to develop in the future. Please let me know how I can improve this more and what could be changed so it would perform better.

public static class Logger
{
    public static void Log(Exception ex)
    {
        StreamWriter sw = null;
        try
        {
            sw = new StreamWriter(AppDomain.CurrentDomain.BaseDirectory + "\\Log.txt", true);
            sw.WriteLine(
                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() ;
            sw.Flush();
            sw.Close();
        }
        catch (Exception e)
        {
            sw = new StreamWriter(AppDomain.CurrentDomain.BaseDirectory + "\\Log.txt", true);
            sw.WriteLine("Error Could not be logged!!");
            throw ex;
        }
    }

    /// <summary>Writes a log file in the root directory with a custom message.
    /// <param name="message">String that will be displayed as a message</param>
    /// </summary>
    public static void writeCustomErrorLog(string message)
    {
        StreamWriter sw = null;
        try
        {
            sw = new StreamWriter(AppDomain.CurrentDomain.BaseDirectory + "\\PassiveLog.txt", true);
            sw.WriteLine(DateTime.Now.ToString() + " : " + "\n Custom Message: " + message);
            sw.Flush();
            sw.Close();
        }
        catch (Exception ex)
        {
            throw ex;
        }
    }

    /// <summary>Writes a log file in the a directory  passed in as a location with a custom message and a custom file-name.
    /// <param name="message">String that will be displayed as a message</param>
    /// <param name="location">String value that accepts a location for the log file to be generated</param>
    /// <param name="fileName">String value that accepts a file-name for the log file to be generated</param>
    /// </summary>
    public static void writeCustomErrorLog(string location, string message,string fileName)
    {
        StreamWriter sw = null;
        try
        {
            sw = new StreamWriter(location + "\\"+fileName+".txt", true);
            sw.WriteLine(DateTime.Now.ToString() + " : " + "\n" + message);
            sw.Flush();
            sw.Close();
        }
        catch (Exception ex)
        {
            throw ex;
        }
    }

    /// <summary>Returns the Mathod name the exception occured
    /// <param name="ex">The <c>exception</c> must be passed as a parameter</param>
    /// <returns>returns the method name where the exception occured as a <c>string</c></returns>
    /// </summary>
    public static string getExceptionGeneratedMethod(Exception ex)
    {
        var s = new StackTrace(ex);
        var thisasm = Assembly.GetExecutingAssembly();
        string methodname = s.GetFrames().Select(f => f.GetMethod()).First(m =>
        m.Module.Assembly == thisasm).Name.ToString();
        return methodname;
    }

    /// <summary>Returns the line number on which the exception occured
    /// <param name="ex">The <c>exception</c> must be passed as a parameter</param>
    /// <returns>returns the line number where the exception occured as a <c>integer</c> value</returns>
    /// </summary>
    public static int getExceptionGeneratedLineNumber(Exception ex)
    {
        var st = new StackTrace(ex, true);
        var frame = st.GetFrame(0);
        int line = frame.GetFileLineNumber();
        return line;
    }
}

Answer 1

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 OutOfMemoryExceptions 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:

• NLog
• Log4Net
• Elmah

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.

Answer 2

Coding style

Variable names

Avoid variable names like sw or st. In your code they still make sense because they're the abbreviation of the class name and it's not a lot of code, but it is still bad practice. When you have lots of code with less obvious class names or origins for the abbreviations, it'll become unclear what they are really fast. Rename them to streamWriter and trace or something similar that clearly indicates what it is.

Casing

Per the Naming Guidelines by Microsoft, use the following rules:

• PascalCasing for methods, class names, ...
• camelCasing for fields, parameters, ...

Your method names will become: WriteCustomErrorLog, GetExceptionGeneratedLineNumber, ...

---

Correctly rethrowing an Exception

Everywhere in your catch block you are rethrowing the exception with throw ex. This is bad practice since you'll lose the stacktrace. Just use throw to rethrow the exception. Implement it this way:

try
{
    //try something that might fail
}
catch (Exception ex)
{
    //Do something with the exception

    //Rethrow the exception
    throw;
}

More reading on this:

• https://stackoverflow.com/questions/730250/is-there-a-difference-between-throw-and-throw-ex
• http://scottdorman.github.io/2007/08/20/Difference-between-quotthrowquot-and-quotthrow-exquot-in-.NET/

---

Disposing of the StreamWriter

Since the StreamWriter class derives from the TextWriter class and the latter implements the IDisposable interface, you can implement a using statement instead of trying to dispose of the object yourself:

using(var streamWriter = new StreamWriter("C:\\PATH_TO_FILE\\Log.txt", true))
{
    streamWriter.WriteLine("THE MESSAGE TO LOG");
}

---

StringBuilder instead of string concatenation

The quickest way of adding strings together is pasting them together with a + sign. Quicker isn't always the better practice, as is in this case. The better practice to combine strings is to use the StringBuilder.

Instead of something like this:

string result = "";
for(var i = 0; i < 100; i++)
{
    result += "SomeString";
}

You get this:

StringBuilder builder = new StringBuilder();
for(var i = 0; i < 100; i++)
{
    builder.Write("SomeString");
}
string result = builder.ToString();

Now in case, where you're not combining a lot of variables it might be better to use the String.Concat method instead of the Stringbuilder. And that code will look like following:

string firstName = "John";
string lastName = "Doe";
string fullName = String.Concat(firstname, " ", lastName);

Jon Skeet has an interesting article about this: http://jonskeet.uk/csharp/stringbuilder.html

---

Avoiding double code

Both variations of the writeCustomErrorLog method have almost the exact same logic in them. A better practice would be to extract the common logic in a separate method and call that method from the variations. This way you avoid a lot of work when expanding it or making changes to the implementation. Here's how you can do it:

public static void WriteCustomErrorLog(string message)
{
    var path = AppDomain.CurrentDomain.BaseDirectory + "\\PassiveLog.txt";
    WriteError(path, message);
}

public static void WriteCustomErrorLog(string location, string message,string fileName)
{
    var path = String.Format("{0}\\{1}.txt", location, fileName);
    WriteError(path, message)
}

private static void WriteError(string filePath, string message)
{
    using (var sw = new StreamWriter(filePath, true))
    {
        sw.WriteLine(message);
    }
}

And now that you have the WriteError method, even your Log method can use it:

public static void Log(Exception ex)
{
    var sb = new StringBuilder();
    sb.WriteLine(DateTime.Now.ToString());
    sb.WriteLine($"Source {ex.Source.ToString().Trim()}");
    sb.WriteLine($"Message : {ex.Message.ToString().Trim()}");
    sb.WriteLine($"Inner Exceptions: {Convert.ToString(ex.InnerException)}");
    sb.WriteLine($"Exception thrown from: {GetExceptionGeneratedMethod(ex)}");
    sb.WriteLine($"Line Number: {GetExceptionGeneratedLineNumber(ex)}");
    sb.WriteLine($"Stack Trace: {ex.StackTrace.ToString()}");

    var path = AppDomain.CurrentDomain.BaseDirectory + "\\Log.txt";
    WriteError(path, sb.ToString());
}

You'll notice that I use this in the code: $"Source {ex.Source.ToString()}". In case you don't know, it's called String interpolation. You can read more about it here: https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/tokens/interpolated.

Answer 3

A few other things:

1) Your implementation is not thread safe. It is not clear how you are going to use it in multi-threaded environment.

2) Reopening file stream for every log message is inefficient approach if you log frequently. You will get a better performance if you keep the stream open for a lifetime of your app. If you rarely write something to log, than your approach is fine, I guess.

3) This looks weird:

    catch (Exception e)
    {
        sw = new StreamWriter(AppDomain.CurrentDomain.BaseDirectory 
           + "\\Log.txt", true);

        sw.WriteLine("Error Could not be logged!!");
        throw ex;
    }

So you try to white an error to log file, write operation fails and throws an exception, you catch this exception and... try to write it to the same file? I think you can safely assume that in most cases it will throw the same exception again. You also forgot to flush and dispose the stream.

4) Overall I do not see anything particularly "custom" about your implementation, that can't be done by pretty much any other popular logging library available on nuget. My advice would be: do not waste your time on writing yet another logging library. Just pick an existing one that was already optimized and tested by thousands of other developers. And focus on writing actual business logic for your actual application.