debugcsharpMinor
Console printing and logging method for a common library
Viewed 0 times
loggingmethodprintingforlibraryandconsolecommon
Problem
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.GetBaseExcepti
```
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.GetBaseExcepti
Solution
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
So: build them first and then simply decide the logging level. Something like this:
Looks slightly cleaner to me. If you require the additional timestamp then it should be easy enough to add.
Update: Incorporated suggestions from comments.
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
- Remove the time stamps. NLog can do that for you.
- Especially
ShortTimeStringusually 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.
- 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 = "";
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.
Code Snippets
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) : "";
}Context
StackExchange Code Review Q#32218, answer score: 6
Revisions (0)
No revisions yet.