Generic error message factory

Question

The purpose of this code is to centralize all error / status messages to present to the user. For example, registering an account and the user email address is already registered. A status code is set then sent off to get a friendly error message to present to the user. All services will have its own set of status code and status messages which is handled by a status handler for filtering. While this is only a basic class I want to make sure that I am going in the right direction before extending.

Status codes

namespace Services.RegistrationService
    {
        public enum StatusCodes
        {
            UserAlreadyExists,
            // more to come

        }
    } 

List of statuses for registration

using Framework;
using System.Collections.Generic;

namespace Services.RegistrationService
{
    public class RegistrationStatusMessages : MessageStatusHandler
    {
        public void AddErrors(List<StatusCodes> codes)
        {
            foreach(var errorCodes in codes)

            switch (errorCodes)
            {
                case StatusCodes.UserAlreadyExists: { base.add("User already exists", "EmailAddress", true); break; }
            }
        }
    }
}

Message status handler

using System.Collections.Generic;
using System.Linq;

namespace Framework
{
    public class MessageStatusHandler
    {
        protected List<MessageStatusHandler> _errorList;

        private string _friendlyErrorMessage;
        private bool _isError;
        private string _propertyName;

        public MessageStatusHandler()
        {
            _errorList = new List<MessageStatusHandler>();

        }

        protected void add(string friendlyErrorMessage, string propertyName, bool isError)
        {
            _errorList.Add(new MessageStatusHandler { _friendlyErrorMessage = friendlyErrorMessage, _propertyName = propertyName, _isError = isError });
        }

        public bool HasErrors()
        {
            return _errorList.Any(x => x._isError == true);
        }
    }
}

Again, it is a simple class setup that centralizes status codes, error messages for each service. Is there any way to improve on this, or am I going in the right direction?

Answer 1

First a few minor things:

1. Standard naming convention for methods in C# is PascalCase.

2. You should use braces { } for pretty much all blocks. This is asking for maintenance trouble:

   foreach(var errorCodes in codes)
   
   switch (errorCodes)
   {
       case StatusCodes.UserAlreadyExists: { base.add("User already exists", "EmailAddress", true); break; }
   }
   

3. The Enum naming convention suggests to use singular form for enums.

---

Now design:

I find it a bit odd that your class represents a descriptor for a status code and acts as a container for all status codes at the same time. This looks to me like an SRP (Single Responsibility Principle) violation.

I would split it up and maybe put some generics in the mix (unfortunately enum can't be used as generic type constraint).

So something along these lines:

public class StatusCodeDescriptor<T>
{
    public readonly T StatusCode;
    public readonly string FriendlyErrorMessage;
    public readonly bool IsError;
    public readonly string PropertyName;

    public StatusCodeDescriptor(T statusCode, string friendlyError, string propertyName, bool isError)
    {
        StatusCode = statusCode;
        FriendlyErrorMessage = friendlyError;
        IsError = isError;
        PropertyName = propertyName;
    }
}

public abstract class StatusCodeHandlerBase<T>
{
    protected List<StatusCodeDescriptor<T>> _StatusCodes = new List<StatusCodeDescriptor<T>>();

    public void AddStatusCodes(IEnumerable<T> statusCodes)
    {
        foreach (var code in statusCodes)
        {
            _StatusCodes.Add(GetDescriptor(code));
        }
    }

    protected abstract StatusCodeDescriptor<T> GetDescriptor(T statusCode);

    public bool HasError()
    {
        return _StatusCodes.Any(c => c.IsError);
    }
}

public enum RegistrationStatusCode
{
    UserAlreadyExists,
    // ...
}

public class RegistrationStatusCodeHandler : StatusCodeHandlerBase<RegistrationStatusCode>
{
    protected override StatusCodeDescriptor<RegistrationStatusCode> GetDescriptor(RegistrationStatusCode statusCode)
    {
        switch (statusCode)
        {
            case RegistrationStatusCode.UserAlreadyExists: 
                return new StatusCodeDescriptor<RegistrationStatusCode>(statusCode, "User already exists", "EmailAddress", true);
        }
    }
}

Answer 2

Interesting idea, much better than having status codes / messages / magic strings wandering everywhere. This is also a good starting point for making messages loclaziable - if they are centralized, the can also be taken from resources in centralized manner.

Regarding possible improvements:

1. I would consider changing AddErrors(List<StatusCodes> codes) to AddErrors(params StatusCodes[] codes) - it will be easier to add just one error (what I suppose is most probable scenrio) or many with sam emethod without need to construct the List object.
2. It might be interesting to add some events to the Manager class - this way some important Listeneres might be configured, i.e. when status Unauthorized appears some listener just cancels all current responses and redirects user to error / login page. Or DatabaseNotResponding might have listener attached that counts occurences per minute and when there is more than one or two (what can happen in heavilly trafficked network) reports this immediatelly to the Admin. These two examples might not be a best design anyway - but I just point the purpose.
3. Probably you are thought about this already, but I would definitelly divide StatusCodes into specialized classes for ease of use like SystemStatusCodes, UserStatusCodes, OrderStatusCodes etc...
4. I think there is some error in your code in class MessageStatusHandler: protected List<MessageStatusHandler> _errorList;. I don't see a point in creating collection of handlers inside on of the exact handler type, perhaps you thought about gathering more results in one object - but I think this part needs more designing that just that simple exmaple.