ErrorManager management

Question

Recently I created a class that would manager errors, log them, and write details about them to files when the function logError was called. It would be done by having an error code and error message and then displaying the error to the console and writing it to the file if the user wanted to.

I just posted this here to see if there was any way I can improve the class.

using log4net;
using System;
using System.Collections.Generic;

namespace Kiwi.Application.Base.Error
{
    sealed class ErrorManager
    {
        private readonly Dictionary<string, string> errorCodes;
        private readonly ILog myLogger;

        public ErrorManager()
        {
            errorCodes = new Dictionary<string, string>();
            myLogger = LogManager.GetLogger(typeof(ErrorManager));

            // Add some errors and their messages to the dictionary
            errorCodes.Add("bn1x", "Error when beginning to listen on server socket.");
            errorCodes.Add("dm9e", "Unable to locate the error log file.");
        }

        public void logError(string errorCode, bool writeErrorToFile = false)
        {
            string errorMessage;
            if (errorCodes.TryGetValue(errorCode, out errorMessage))
            {
                myLogger.Warn(errorMessage);

                if (writeErrorToFile)
                    logToFile(errorCode);
            }
            else
            {
                logUnhandeldError("[Error code " + errorCode + "] " + errorCode);
            }
        }

        private void logUnhandeldError(string errorCode)
        {
            myLogger.Error("Unhandeld error " + errorCode);
        }

        private void logToFile(string errorCode = "", string fileName = "logs/error.log")
        {
            try
            {
                using (System.IO.StreamWriter file = new System.IO.StreamWriter(fileName, true))
                {
                    file.WriteLine("Error " + errorCode + " logged at " + getErrorTimestamp());
                    file.WriteLine("Message: " + tryGetErrorMessage(errorCode));
                }
            }
            catch (System.IO.FileNotFoundException)
            {
                logError("dm9e");
            }
        }

        private string getErrorTimestamp()
        {
            return DateTime.Now.ToLongDateString();
        }

        private string tryGetErrorMessage(string errorCode)
        {
            string errorMessage;

            if (errorCodes.TryGetValue(errorCode, out errorMessage))
                return errorMessage;

            else return "";
        }
    }
}

Answer 1

Generally in .NET (especially C#) public members are PascalCase.

On a more important note: don't use strings for the errorCode. Instead, use an enum.

public enum ErrorCode
{
    ServerSocketBeginListeningFailure, // Rename this to something shorter if desired.
    LogFileNotFound,
}

You could also, potentially, make this a Flags enum, which would mean that error codes could be easily combined and used together to indicate multiple errors occurring at a time.

This will allow you to make more certain that the user doesn't specify a bad error code.

Use braces even when not necessary, they won't generally prevent bugs, but they will help prevent bugs.

Just as well, do not inline an if/else if/else statement when not using braces. Typically, when I see one of those, my eyes immediately follow the next line. I didn't notice return "" on your else in tryGetErrorMessage until I realized there was not a statement below it.

Answer 2

There are some points that I need to comment on.

First of all, it is unthinkable for a class that deals with errors to not work with Exception instances.

This would be acceptable if the errors that this class logs are pure business exceptions, but an error Error when beginning to listen on server socket is not a business exception.

Your class will only log messages and error codes, and the most valuable information that exceptions do contain are lost:

Exception Type
StackTrace
InnserException
    ExceptionType
    StackTrace
    InnerException
        ExceptionType
        StackTrace
        InnerException
            ....
            ....

In the case of the error Error when beginning to listen on server socket, what is the root cause? Is the port in use? Is it because insufficient access rights? Is the port number out of range?

Therefore, you should definitely change your class to accept Exception instances along with the error codes / messages.

When you call ToString() on an exception instance, it will dump all the nested exceptions, their stack traces etc. so you won't miss anything.

Writing only the code/message pair to the console might be OK, but the log file should definitely contain the original exception message and all the details about the exception's root cause and location