1
\$\begingroup\$

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

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);
    }
}
\$\endgroup\$

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.