Error-Handling Class and Logging

Question

I am relatively new to C# and would like to see if what I have been using for my exception handling, message and logging is along industry standards. I have created a simple class for the Error Handler and for the logging I use Log4Net. After reading Clean Code, I was thinking about separating DisplayMessage into 2 different procedures for the message and logging. The log file is usually written to a file server so multiple users can access it; I have enabled MinimalLock for this purpose. For my example, I have changed the log path to C:\Temp\. An example of a project I use the Error Handler class and logging in is on GitHub. For reference, I am using Visual Studio 2017 Community.

---

ErrorHandler.cs

using System;
using System.Windows.Forms;
using log4net;
[assembly: log4net.Config.XmlConfigurator(Watch = true)]
/// <summary> 
/// Used to handle exceptions
/// </summary>
public class ErrorHandler
{
 private static readonly ILog log = LogManager.GetLogger(typeof(ErrorHandler));
 /// <summary> 
 /// Used to produce an error message and create a log record
 /// <example>
 /// <code lang="C#">
 /// ErrorHandler.DisplayMessage(ex);
 /// </code>
 /// </example> 
 /// </summary>
 /// <param name="ex">Represents errors that occur during application execution.</param>
 /// <remarks></remarks>
 public static void DisplayMessage(Exception ex)
 {
 System.Diagnostics.StackFrame sf = new System.Diagnostics.StackFrame(1);
 System.Reflection.MethodBase caller = sf.GetMethod();
 string currentProcedure = (caller.Name).Trim();
 string errorMessageDescription = ex.ToString();
 errorMessageDescription = System.Text.RegularExpressions.Regex.Replace(errorMessageDescription, @"\r\n+", " "); //the carriage returns were messing up my log file
 string msg = "Contact your system administrator. A record has been created in the log file." + Environment.NewLine;
 msg += "Procedure: " + currentProcedure + Environment.NewLine;
 msg += "Description: " + ex.ToString() + Environment.NewLine;
 log.Error("[PROCEDURE]=|" + currentProcedure + "|[USER NAME]=|" + Environment.UserName + "|[MACHINE NAME]=|" + Environment.MachineName + "|[DESCRIPTION]=|" + errorMessageDescription);
 MessageBox.Show(msg, "Unexpected Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
 }
}

Usage Example:

/// <summary> 
/// Return the count of items in a delimited list
/// </summary>
/// <param name="valueList">Represents the list of values in a string </param>
/// <param name="delimiter">Represents the list delimiter </param>
/// <returns>the number of values in a delimited string</returns>
public int GetListItemCount(string valueList, string delimiter)
{
 try
 {
 string[] comboList = valueList.Split((delimiter).ToCharArray());
 return comboList.GetUpperBound(0) + 1;
 }
 catch (Exception ex)
 {
 ErrorHandler.DisplayMessage(ex);
 return 0;
 }
}

---

Log4Net

screenshot

app.config

<log4net>
 <appender name="ConsoleAppender" type="log4net.Appender.ConsoleAppender">
 <layout type="log4net.Layout.PatternLayout">
 <conversionPattern value="%date [%thread] %-5level %logger [%ndc] - %message%newline"/>
 </layout>
 </appender>
 <appender name="FileAppender" type="log4net.Appender.FileAppender">
 <file value="C:\Temp\MyLogFile.log"/>
 <appendToFile value="true"/>
 <lockingModel type="log4net.Appender.FileAppender+MinimalLock"/>
 <layout type="log4net.Layout.PatternLayout">
 <conversionPattern value="%date|%-5level|%message%newline"/>
 </layout>
 </appender>
 <root>
 <level value="ALL"/>
 <appender-ref ref="FileAppender"/>
 </root>
</log4net>

Answer 1

I'm not familiar with log4net, so I will focus on the code itself.

Here is a list of what could be improved:

• --- Style ---
• ErrorHandler should be a static class to prevent newing up an instance of it
• Lack of using namespace: Generally, you should declare all of them, unless:
  • there is an conflic in which case you can alias it: using A = Some.Namespace.A
  • a namespace is particularly noisy and is polluting the intellisense

• Lack of var: Using the type name or the full name can make the code difficult to scan through. It is more difficult to locate a variable when they are not aligned. For the most part, the right hand side assignment should give enough of hint.

  System.Diagnostics.StackFrame sf = new System.Diagnostics.StackFrame(1);
  System.Reflection.MethodBase caller = sf.GetMethod();
  string currentProcedure = (caller.Name).Trim();
  // --- for comparison
  var sf = new System.Diagnostics.StackFrame(1);
  var caller = sf.GetMethod();
  var currentProcedure = (caller.Name).Trim();
  

• Lack of "spacing": Nobody likes reading a wall of text, and a wall of code is no better. You can divide the logical blocks of your code with some empty lines: (Think of them as paragraphs)

  // gather context
  var sf = new System.Diagnostics.StackFrame(1);
  var caller = sf.GetMethod();
  // format message
  var currentProcedure = (caller.Name).Trim();
  var errorMessageDescription = ex.ToString();
  errorMessageDescription = System.Text.RegularExpressions.Regex.Replace(errorMessageDescription, @"\r\n+", " "); //the carriage returns were messing up my log file
  var msg = "Contact your system administrator. A record has been created in the log file." + Environment.NewLine;
  msg += "Procedure: " + currentProcedure + Environment.NewLine;
  msg += "Description: " + ex.ToString() + Environment.NewLine;
  // handle exception
  log.Error("[PROCEDURE]=|" + currentProcedure + "|[USER NAME]=|" + Environment.UserName + "|[MACHINE NAME]=|" + Environment.MachineName + "|[DESCRIPTION]=|" + errorMessageDescription);
  MessageBox.Show(msg, "Unexpected Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
  

• Use of unnecessary parenthesis that doesn't highlight order of operation:

  string currentProcedure = (caller.Name).Trim();
  string[] comboList = valueList.Split((delimiter).ToCharArray());
  

• --- code ---

  string[] comboList = valueList.Split((delimiter).ToCharArray());
  return comboList.GetUpperBound(0) + 1;
  

  You can simply return comboList.Length here.

   /// <param name="delimiter">Represents the list delimiter </param>
   string[] comboList = valueList.Split((delimiter).ToCharArray());
  

  You are using it as multiple delimiters, and not just one. The name should reflect that.

   /// <returns>the number of values in a delimited string</returns>
  

  The return of value 0 in error handling should be documented in the <returns> or <remarks> tag.

---

public static class ErrorHandler
{
 public static void DisplayMessage(Exception ex)
 {
 var sf = new System.Diagnostics.StackFrame(1);
 var caller = sf.GetMethod();
 var currentProcedure = caller.Name.Trim();
 var logMessage = string.Concat(new Dictionary<string, string>
 {
 ["PROCEDURE"] = currentProcedure,
 ["USER NAME"] = Environment.UserName,
 ["MACHINE NAME"] = Environment.MachineName,
 ["DESCRIPTION"] = ex.ToString().Replace("\r\n", " "), // the carriage returns were messing up my log file
 }.Select(x => $"[{x.Key}]=|{x.Value}|"));
 log.Error(logMessage);
 // pick one:
 var userMessage = new StringBuilder()
 .AppendLine("Contact your system administrator. A record has been created in the log file.")
 .AppendLine("Procedure: " + currentProcedure)
 .AppendLine("Description: " + ex.ToString())
 .ToString();
 var userMessage = string.Join(Environment.NewLine,
 "Contact your system administrator. A record has been created in the log file.",
 "Procedure: " + currentProcedure,
 "Description: " + ex.ToString(),
 ) + Environment.NewLine;
 MessageBox.Show(userMessage, "Unexpected Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
 }
}