3
\$\begingroup\$

I have the following method that's in a common library. I want to know how to write it better, but mostly, I want to know how to think about this better so I don't end up with code like this.

public virtual void PrintConsoleAndLog(string verboseMessage = null, Exception e = null,
                                        OnErrorEventsArgs.ShowExceptionLevel exceptionLevel =
                                            OnErrorEventsArgs.ShowExceptionLevel.Error)
{
    switch (exceptionLevel)
    {
        case OnErrorEventsArgs.ShowExceptionLevel.Info:
            if (e == null)
            {
                Logger.Info("\n[{0}] {1}", DateTime.Now.ToShortTimeString(), verboseMessage);
                Console.WriteLine("\n[{0}] {1}", DateTime.Now.ToShortTimeString(), verboseMessage);
            }
            else
            {
                if (verboseMessage == null)
                {
                    Logger.Info(String.Format("\n[{0}]Base Exception: {1}\n\nStacktrace: {2}",
                                                DateTime.Now.ToShortTimeString(),
                                                e.GetBaseException(), e.StackTrace));

                    Console.WriteLine("[{0}]Base Exception: {1}\n\nStacktrace: {2}",
                                        DateTime.Now.ToShortTimeString(),
                                        e.GetBaseException(), e.StackTrace);
                }
                else
                {
                    Logger.Info(String.Format("\n[{0}] Error: {1}\n\nBase Exception: {2}\n\nStacktrace: {3}",
                                                DateTime.Now.ToShortTimeString(), verboseMessage,
                                                e.GetBaseException(), e.StackTrace));

                    Console.WriteLine("[{0}] Error: {1}\n\nBase Exception: {2}\n\nStacktrace: {3}",
                                        DateTime.Now.ToShortTimeString(), verboseMessage,
                                        e.GetBaseException(), e.StackTrace);
                }
            }
            break;

        case OnErrorEventsArgs.ShowExceptionLevel.Debug:
            if (e == null)
            {
                Logger.Debug("\n[{0}] {1}", DateTime.Now.ToShortTimeString(), verboseMessage);
                Console.WriteLine("\n[{0}] {1}", DateTime.Now.ToShortTimeString(), verboseMessage);
            }
            else
            {
                if (verboseMessage == null)
                {
                    Logger.Debug(String.Format("\n[{0}]Base Exception: {1}\n\nStacktrace: {2}",
                                                DateTime.Now.ToShortTimeString(),
                                                e.GetBaseException(), e.StackTrace));

                    Console.WriteLine("[{0}]Base Exception: {1}\n\nStacktrace: {2}",
                                        DateTime.Now.ToShortTimeString(),
                                        e.GetBaseException(), e.StackTrace);
                }
                else
                {
                    Logger.Debug(String.Format("\n[{0}] Error: {1}\n\nBase Exception: {2}\n\nStacktrace: {3}",
                                                DateTime.Now.ToShortTimeString(), verboseMessage,
                                                e.GetBaseException(), e.StackTrace));

                    Console.WriteLine("[{0}] Error: {1}\n\nBase Exception: {2}\n\nStacktrace: {3}",
                                        DateTime.Now.ToShortTimeString(), verboseMessage,
                                        e.GetBaseException(), e.StackTrace);
                }
            }
            break;

        // note that OnErrorEventsArgs.ShowExceptionLevel.Error will end up here also
        default:
            if (e == null)
            {
                Logger.Error("\n[{0}] {1}", DateTime.Now.ToShortTimeString(), verboseMessage);
                Console.WriteLine("\n[{0}] {1}", DateTime.Now.ToShortTimeString(), verboseMessage);
            }
            else
            {
                if (verboseMessage == null)
                {
                    Logger.Error(String.Format("\n[{0}]Base Exception: {1}\n\nStacktrace: {2}",
                                                DateTime.Now.ToShortTimeString(),
                                                e.GetBaseException(), e.StackTrace));

                    Console.WriteLine("[{0}]Base Exception: {1}\n\nStacktrace: {2}",
                                        DateTime.Now.ToShortTimeString(),
                                        e.GetBaseException(), e.StackTrace);
                }
                else
                {
                    Logger.Error(String.Format("\n[{0}] Error: {1}\n\nBase Exception: {2}\n\nStacktrace: {3}",
                                                DateTime.Now.ToShortTimeString(), verboseMessage,
                                                e.GetBaseException(), e.StackTrace));

                    Console.WriteLine("[{0}] Error: {1}\n\nBase Exception: {2}\n\nStacktrace: {3}",
                                        DateTime.Now.ToShortTimeString(), verboseMessage,
                                        e.GetBaseException(), e.StackTrace);
                }
            }
            break;
    }
}
\$\endgroup\$
  • 5
    \$\begingroup\$ Which logger are you using? Instead of doing logging everything twice (once with your logger and once to console) can't you just reconfigure your logger to also write to the console? ie: log4net.Appender.ConsoleAppender && log4net.Appender.RollingFileAppender \$\endgroup\$ – Peter Kneale Oct 4 '13 at 5:28
  • \$\begingroup\$ I'm using Nlog - I'm not sure if that's possible with it but I will look into it \$\endgroup\$ – Michael A Oct 4 '13 at 5:36
  • 2
    \$\begingroup\$ It is, look here under multiple targets \$\endgroup\$ – Peter Kneale Oct 4 '13 at 5:44
6
\$\begingroup\$

In the comments it was already mentioned to configure the logger to determine the logging target which avoids logging everything twice.

Another thing to improve: Eliminate duplicate code (statements and expressions). From looking at the code there are many things which are repeated over and over again

  1. Remove the time stamps. NLog can do that for you.
    • Especially ShortTimeString usually omits the seconds (at least in most standard locales I have seen). Not sure what kind of application you have but for error logging I would assume that it is usually better to have the timestamp as accurate as possible.
  2. You log basically three types of messages:
    • Something when you do not have an exception object
    • Base Exception and Stacktrace
    • Base Exception and Stacktrace with extra error message

So: build them first and then simply decide the logging level. Something like this:

private const string NOMESSAGE = "<NOMESSAGE>";

public virtual void PrintConsoleAndLog(string verboseMessage = null, Exception e = null,
                                    OnErrorEventsArgs.ShowExceptionLevel exceptionLevel =
                                        OnErrorEventsArgs.ShowExceptionLevel.Error)
{
    var message = verboseMessage ?? NOMESSAGE;
    if (e != null)
    {
         var additionalMessage = GetAdditionalFormattedMessage(verboseMessage);
         message = string.Format("{0}Base Exception: {1}\n\nStacktrace: {2}", 
                            additionalMessage, e.GetBaseException(), e.StackTrace);
    }
    switch (exceptionLevel)
    {
        case OnErrorEventsArgs.ShowExceptionLevel.Info:
             Logger.Info(message);
             break;
        case OnErrorEventsArgs.ShowExceptionLevel.Debug:
             Logger.Debug(message);
             break;
        case OnErrorEventsArgs.ShowExceptionLevel.Error:
        default:
             Logger.Error(message);
             break;
    }
}

private string GetAdditionalFormattedMessage(string verboseMessage)
{
    return verboseMessage != null ? string.Format("Error: {0}\n\n", verboseMessage) : "";
}

Looks slightly cleaner to me. If you require the additional timestamp then it should be easy enough to add.

Update: Incorporated suggestions from comments.

\$\endgroup\$
  • 4
    \$\begingroup\$ OP indicated he uses NLog, which has a Log (LogLevel, string) overload. You could get rid of the switch as long as you had a way to convert from OnErrorEventsArgs.ShowExceptionLevel to the appropriate NLog LogLevel value. \$\endgroup\$ – Dan Lyons Oct 4 '13 at 17:34
  • 1
    \$\begingroup\$ I see a 'magic constant' (the string "<NO MESSAGE>") there: convert it to a constant. Also the creation of the additionalMessage should be refactored (i.e. move to a two-line methode) to give the method a constant level of abstraction. \$\endgroup\$ – alzaimar Oct 7 '13 at 5:38

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.