5
\$\begingroup\$

There are quite a few things I am considering:

  1. I will have to check for null values
  2. I will have serious trouble persisting it in the database.

How do I improve this or re-factor this for better results?

public class ErrorLog
    {
        public static void LogError(Exception e)
        {
            var exceptionLog = new Dictionary<String, String>
                                   {
                                       {"Inner Exception", e.InnerException.Message},
                                       {"Message", e.Message},
                                       {"Source", e.Source},
                                       {"StackTrace", e.StackTrace},
                                       {"MethodName",e.TargetSite.Name}
                                   };
            foreach (KeyValuePair<String, String> kvp in e.Data)
                exceptionLog.Add(kvp.Key, kvp.Value);
        }
    }

Update with the new and simpler class

  public class ErrorLog
    {
        public static void LogError(Exception e)
        {
            var innerExcpetionMessage = e.InnerException == null ? "Null" : e.InnerException.Message;
            using (var connection = new SqlConnection(ConfigurationManager.ConnectionStrings["ConnectionString"].ConnectionString))
            {                
                    var cmd = new SqlCommand("ApplicationErrorLog", connection) { CommandType = System.Data.CommandType.StoredProcedure };
                    cmd.Parameters.AddWithValue("@Type", e.GetType().Name);
                    cmd.Parameters.AddWithValue("@InnerException", innerExcpetionMessage);
                    cmd.Parameters.AddWithValue("@Message", e.Message);
                    cmd.Parameters.AddWithValue("@Source", e.Source);
                    cmd.Parameters.AddWithValue("@StackTrace", e.StackTrace);
                    cmd.Parameters.AddWithValue("@MethodName", e.TargetSite.Name);
                    connection.Open();
                    cmd.ExecuteNonQuery();
                    connection.Close();
                }

            }
        }
    }

Now what if there is an error in updating this log, how will I log that?

\$\endgroup\$
  • 1
    \$\begingroup\$ I dont know if there is any good way to log logging errors. Interesting question. \$\endgroup\$ – Mattias Oct 29 '11 at 12:28
  • \$\begingroup\$ yes this stumped me as well..if i re-log this it will be an infinite loop and yes it was fun trying that too :P .. \$\endgroup\$ – ashutosh raina Oct 29 '11 at 12:31
  • 1
    \$\begingroup\$ I would perhaps log to a file, or to the event log on critical log failure? \$\endgroup\$ – dreza Oct 29 '11 at 22:18
5
\$\begingroup\$

Why would you have serious trouble with persistance?

Just do null checks for InnerException and TargetSite, rest will be inserted as null or string empty depending on your db setup.

And why not just simplify it as:

var innerExceptionMessage = e.InnerException == null ? string.Empty : e.InnerException.Message;
var targetSiteMessage     = e.TargetSite == null ? string.Empty : e.TargetSite.Message;

exceptionLog.Add("Inner Exception", innerExceptionMessage);
exceptionLog.Add("Message", e.Message);
exceptionLog.Add("Source", e.Source);
exceptionLog.Add("StackTrace", e.StackTrace);
exceptionLog.Add("MethodName",targetSiteMessage);

Keep it simple :)

\$\endgroup\$
  • \$\begingroup\$ upvote for thinkiing like me i have updated the question.. \$\endgroup\$ – ashutosh raina Oct 29 '11 at 12:19
2
\$\begingroup\$

I would pose several suggestions:

  1. First, you should add exception handling code to your logging. You may not want your logging code raising unhandled exceptions (which, depending where it happens in your code, could try to recursively log again). At the very least, it would give you the opportunity to write something out about why it failed and perhaps include the original exception to some backup log destination like system event logs, console output, or a file.
  2. Second, I would implement your database logging as a listener class for a separate logging class. As this is C#, you may take a look at the TraceSource class. Your database logger would be written to extend TraceListener and then attached to the Listeners collection on the trace object. This gives you the freedom to attach multiple types of listeners that can log to various different outputs, each potentially having different filtering. This can even be done via app.config file.
  3. You may also want to use DBNull instead of "Null" or string.Empty for values which are unavailable, assuming your stored procedure and logging schema support it.
\$\endgroup\$
  • \$\begingroup\$ the exception handling for my logging code is also a concern , I have mentioned in the comments for the question above that I did face this situation and a suggested alternative was to log the exception in the logging mechanism to a file. \$\endgroup\$ – ashutosh raina Nov 1 '11 at 5:40
2
\$\begingroup\$

You're doing ugly things here. The "new and simpler class" tightly couples you to SQL and persistence concerns. A dictionary of String String is an implementation concern, and more likely the basis for a Class.

A better approach would be to go with your original approach, then define a separate method to do the database write that takes in the dictionary/class as a parameter.

\$\endgroup\$
  • \$\begingroup\$ if I am understanding it right you would like me to have one class whose instance would be populated with the exception details I wish to log and then have another method to which I pass the instance and then have it persisted to the database. \$\endgroup\$ – ashutosh raina Nov 1 '11 at 5:34
  • \$\begingroup\$ correct. This would be a cleaner approach that would provide the most adaptability down the line. \$\endgroup\$ – Visionary Software Solutions Nov 2 '11 at 12:24
1
\$\begingroup\$

Just a quick comment on @Mattias's answer. I think you would want to capture the original exception message if the InnerException is null. If so, you could write:

var innerExceptionMessage = e.InnerException == null ? string.Empty : e.InnerException.Message;

as

var innerExceptionMessage = (e.InnerException ?? e).Message ?? string.Empty;
\$\endgroup\$
  • \$\begingroup\$ The original exception message is already captured in: exceptionLog.Add("Message", e.Message). An InnerException can be null and therefore should be inserted as null to avoid misleading troubleshooting. \$\endgroup\$ – Mattias Nov 1 '11 at 9:36
  • \$\begingroup\$ Good point, @Mattias \$\endgroup\$ – neontapir Nov 1 '11 at 16:00
  • \$\begingroup\$ AFAIK in your code the left operand of ?? will never be null ... \$\endgroup\$ – ashutosh raina Nov 5 '11 at 6:53
  • \$\begingroup\$ @ashutoshraina, thank you, I couldn't recall. \$\endgroup\$ – neontapir Nov 5 '11 at 20:51

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.