4
\$\begingroup\$

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
}
\$\endgroup\$
1
\$\begingroup\$

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);
}
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – OmniOwl May 25 '13 at 0:09
  • \$\begingroup\$ 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. \$\endgroup\$ – Joe F May 25 '13 at 0:25
1
\$\begingroup\$

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);
    }
}
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – OmniOwl May 25 '13 at 9:56
  • \$\begingroup\$ @Vipar I believe it can be installed via a Nuget package as well which should make it a bit easier to get started \$\endgroup\$ – dreza May 25 '13 at 19:42
  • \$\begingroup\$ How delightful. I'll look into it :) \$\endgroup\$ – OmniOwl May 25 '13 at 20:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.