More Elegant Solution to Logging Class

Question

I got this class in C# which works fine. But I was wondering if there was a more elegant solution to what I am trying to do. It looks rather clumsy and inefficient for a Logging functionality.

The code should be fairly self-explanatory.

public static class Log
{
 public static string EngineName { get; set; }
 private static List<String> logdata = new List<string>();
 static void LogMessage(string msg)
 {
 Console.WriteLine("[{0}][LOG]: {1}", EngineName, msg);
 StringBuilder sb = new StringBuilder();
 sb.Append("[").Append(EngineName).Append("][LOG]: ").Append(msg);
 logdata.Add(sb.ToString());
 }
 public static void EngineMessage(string msg)
 {
 Console.WriteLine("[{0}][ENGINE]: {1}", EngineName, msg);
 StringBuilder sb = new StringBuilder();
 sb.Append("[").Append(EngineName).Append("][Engine]: ").Append(msg);
 logdata.Add(sb.ToString());
 }
 public static void ExceptionMessage(Exception e)
 {
 Console.WriteLine("[{0}][EXCEPTION]: {1}", EngineName, e.Message);
 StringBuilder sb = new StringBuilder();
 sb.Append("[").Append(EngineName).Append("][EXCEPTION]: ").Append(e.Message);
 logdata.Add(sb.ToString());
 }
 public static void CustomMessage(string title, string msg)
 {
 title.ToUpper();
 Console.WriteLine("[{0}][{1}]: {2}", EngineName, title, msg);
 StringBuilder sb = new StringBuilder();
 sb.Append("[").Append(EngineName).Append("][").Append(title).Append("]: ").Append(msg);
 logdata.Add(sb.ToString());
 }
 public static void ClearLogData()
 {
 logdata.Clear();
 }
 public static void PrintLog()
 {
 Console.WriteLine("===== LOG DATA =====\n");
 Console.WriteLine(new DateTime().ToString()+"\n");
 foreach(string s in logdata)
 {
 Console.WriteLine(s + "\n");
 }
 }
 public static void SaveLog(string path)
 {
 throw new NotSupportedException();
 }
}

EDIT

Based on feedback from the accepted answer here is my revised code:

public static class Log
{
 public static string EngineName { get; set; }
 private static List<String> logdata = new List<string>();
 public static void LogMessage(string msg, ELogflag flag, string title = "")
 {
 StringBuilder sb = new StringBuilder();
 sb.Append("[" + EngineName + "]");
 switch (flag)
 {
 case ELogflag.LOG:
 sb.Append("[LOG]");
 break;
 case ELogflag.ENGINE:
 sb.Append("[ENGINE]");
 break;
 case ELogflag.CRITICAL:
 sb.Append("[CRITICAL]");
 break;
 case ELogflag.CUSTOM:
 title = title.ToUpper();
 sb.Append("[" + title + "]");
 break;
 default:
 sb.Append("[UNKNOWN]");
 break;
 }
 sb.Append(msg);
 Console.WriteLine(sb.ToString());
 logdata.Add(sb.ToString());
 }
 public static void ClearLogData()
 {
 logdata.Clear();
 }
 public static void PrintLog()
 {
 Console.WriteLine("===== LOG DATA =====\n");
 Console.WriteLine(new DateTime().ToString() + "\n");
 foreach (string s in logdata)
 {
 Console.WriteLine(s + "\n");
 }
 }
 public static void SaveLog(string path)
 {
 throw new NotSupportedException();
 }
}

And my new Enum:

enum ELogflag
{
 LOG,
 ENGINE,
 CRITICAL,
 CUSTOM
}

Answer 1

C# is not my main language, so I apologize for any mistakes that may exist in the following code.

---

There is quite a bit of duplication in this class. It seems like LogMessage(), EngineMessage() and ExceptionMessage() could all delegate to CustomMessage(). For example:

public static void LogMessage(string msg)
{
 CustomMessage("LOG", msg);
}
// etc ...

---

It also looks like the same string is being built twice in each function. It would be better to create the string once. Maybe something like this instead:

string logMsg = String.Format("[{0}][{1}]: {2}", EngineName, title, msg);
Console.WriteLine(logMsg);
logdata.Add(logMsg);

---

title.ToUpper();

Does this work? I would not expect this to modify the string in place, but rather return a new string. You probably need to do this:

title = title.ToUpper();

---

With the above changes:

public static void LogMessage(string msg)
{
 CustomMessage("LOG", msg);
}
public static void EngineMessage(string msg)
{
 CustomMessage("ENGINE", msg);
}
public static void ExceptionMessage(Exception e)
{
 CustomMessage("EXCEPTION", msg);
}
public static void CustomMessage(string title, string msg)
{
 string logMsg = String.Format("[{0}][{1}]: {2}", EngineName, title.ToUpper(), msg);
 Console.WriteLine(logMsg);
 logdata.Add(logMsg);
}

• OmniOwl: Just wanted some standard methods to call. If nothing fits the standard calls, do a Custom message for the log. Maybe I could make an Enum with flags. ENGINE, LOG, EXCEPTION, CUSTOM, and then just check for that.
• Joe F: You can keep those standard methods and still eliminate much of the duplication by delegating to CustomMessage(). I've updated my answer to hopefully make that clearer.

Answer 2

Have you considered using log4net? If not I would recommend this as a simple and well tested logging framework:

Other considerations to the implementation might include:

1. Threadsafety. I don't think (?) this code is threadsafe. You could call LogMessage at the same time as PrintLog is being called causing contention in the logData list. You might want to take a look at thread safe lists if available.

My simple re-work of the methods to try and make your LogMessage method a bit more focused:

public static class Log
{
 public static string EngineName { get; set; } 
 private static readonly List<String> logdata = new List<string>();
 public static void LogMessage(string msg, ELogflag flag, string title = "")
 {
 StringBuilder sb = new StringBuilder();
 sb.Append(Format(EngineName));
 sb.Append(Format(LogType(flag, title)));
 sb.Append(msg);
 string errorMessage = sb.ToString();
 WriteLine(errorMessage);
 logdata.Add(errorMessage);
 }
 public static void ClearLogData()
 {
 logdata.Clear();
 }
 public static void PrintLog()
 {
 WriteLine("===== LOG DATA =====");
 WriteLine(new DateTime().ToString());
 foreach (string s in logdata)
 {
 WriteLine(s);
 }
 }
 // Single method reponsible for doing the actual write. If you later
 // decide to write to a file for example this is the only method that needs changing
 private static void WriteLine(string msg)
 {
 Console.WriteLine(msg + "\n");
 }
 private static void WriteLine(StringBuilder msg)
 {
 WriteLine(msg.ToString());
 }
 // We don't need to use a switch here. Just get the name from the enum
 private static string LogType(ELogflag flag, string customMsg = "")
 {
 string message = flag == ELogflag.CUSTOM ? customMsg : flag.ToString();
 return message.ToUpper();
 }
 // Single method for determining the format of your headers
 private static string Format(string message)
 {
 return string.Format("[{0}]", message);
 }
}

• OmniOwl: You are right that it isn't thread-safe which is something I have to check on if I started to thread my application. I will have a look at the Log Framework though. Could be interesting.
• dreza: @Vipar I believe it can be installed via a Nuget package as well which should make it a bit easier to get started
• OmniOwl: How delightful. I'll look into it :)