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);
}

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);
    }
}