2
\$\begingroup\$

I have a base class called ExceptionPlus(please suggest a better name). Which is being inherited by multiple child class like BusinessNotFoundException, ArgumentInvalidException etc.

My question is the way the child classes and base classes are implemented are not very OOP. Each child class has just a constructor which passes all the parameters to base class and base class handled everything.

I can see only polymorphism but single responsibilty is missing of each child class. Each Child class is having the same implmentation just with different parameters. The Exception is set using asp.net core FilterMiddleware. Also I am a bit concerned about all those properties. Here is my code.

Base Class ExceptionPlus.cs

public class ExceptionPlus : Exception
    {
        public LogType LogType { get; set; } = LogType.ErrorSevere;

        public HttpStatusCode StatusCode { get; set; } = HttpStatusCode.InternalServerError;

        public ExceptionCode? Code { get; set; } = null;

        public string FriendlyMessage { get; set; } = "";

        public string DetailedMessage { get; set; } = "";

        public ExceptionPlus(string message) : base(message)
        {

        }

        public ExceptionPlus(string message, 
            LogType errorType = LogType.ErrorSevere, 
            HttpStatusCode statusCode = HttpStatusCode.InternalServerError, 
            ExceptionCode? code = null) : base(message)
        {
            LogType = errorType;
            StatusCode = statusCode;
            Code = code;
        }

        public ExceptionResponse ToResponse()
        {
            var exResponse = new ExceptionResponse();

            if (Code.HasValue)
                exResponse.ErrorCode = Code.Value;
            else
            {
                switch (StatusCode)
                {
                    case HttpStatusCode.BadRequest:
                        exResponse.ErrorCode = ExceptionCode.BadRequest;
                        break;
                    case HttpStatusCode.NotFound:
                        exResponse.ErrorCode = ExceptionCode.NotFound;
                        break;
                    case HttpStatusCode.Unauthorized:
                        exResponse.ErrorCode = ExceptionCode.Unauthorized;
                        break;
                    case HttpStatusCode.ServiceUnavailable:
                        exResponse.ErrorCode = ExceptionCode.ServiceUnavailable;
                        break;
                    case HttpStatusCode.GatewayTimeout:
                    case HttpStatusCode.RequestTimeout:
                        exResponse.ErrorCode = ExceptionCode.Timeout;
                        break;
                    default:
                        exResponse.ErrorCode = ExceptionCode.InternalServerError;
                        break;
                }
            }

            if (!string.IsNullOrWhiteSpace(FriendlyMessage))
                exResponse.ErrorMessage = FriendlyMessage;
            else if (LogType.ToString().StartsWith("Client"))
                exResponse.ErrorMessage = Message;
            else
                exResponse.ErrorMessage = "Service currently unavailable - please try again later";

            if (!string.IsNullOrWhiteSpace(DetailedMessage))
            {
                exResponse.ErrorDetail = DetailedMessage;
            }

            return exResponse;
        }

    }

Child class BusinessNotFoundException.cs

public class BusinessNotFoundException : ExceptionPlus
    {
        public BusinessNotFoundException(int businessId, string message = null) : base($"{message ?? "Business not found"} (BusinessId: {businessId})", LogType.ClientFault, HttpStatusCode.NotFound)
        {

        }

        public BusinessNotFoundException(string businessGuid, string message = null) : base($"{message ?? "Business not found"} (BusinessGuid: {businessGuid})", LogType.ClientFault, HttpStatusCode.NotFound)
        {

        }
    }

Class that throw the Exception Manager.cs

.....

  public async Task<ThreeDSecureRequestResultViewModel> ThreeDSecureRequest(int businessId, ThreeDSecureRequestViewModel request, IDistributedCache cache)
        {
            var business = await Sys.Sql.QuerySingleAsync<Business>("spBusinessGetApiSetupInfo", new { BusinessID = businessId });
            if (business == null)
            {
                throw new BusinessNotFoundException(businessId);
            }
}

This handle Exception Creation ExceptionPlusFilterAttribute.cs

if (context.Exception is ExceptionPlus)
                {
                    var ipEx = (ExceptionPlus) context.Exception;
                    le.Type = ipEx.LogType;
                    httpStatus = ipEx.StatusCode;
                    exResponse = ipEx.ToResponse();
                    le.AddProperty("HttpStatus", ipEx.StatusCode.ToString());
                }
                else
                {
                    exResponse = ExceptionResponse.FromException(context.Exception);
                }
\$\endgroup\$
1
\$\begingroup\$

A heirarchy of exception classes where the child classes only differ in their constructor is a common enough and useful pattern - but mainly because in your catch blocks you can specify which ones you're interested in and handle them appropriately. If you're not doing that, then there's not a lot of point in using distinct classes, and instead of, e.g. BusinessNotFoundException you could just have a static 'CreateBusinessNotFoundError' function that creates an instance of your ExceptionPlus class. And yeah, I wouldn't call it that, I usually use something like APIException.

\$\endgroup\$
  • \$\begingroup\$ I agree with this answer (upvoted), but it may be worth noting that from C# 6 and above, you can use exception filters, so the catch motivation no longer applies in such code bases. F# has had that capability longer (maybe always; can't remember). I don't know what the status is regarding Visual Basic. The bottom line, though, is that you might only need this idiom for C# 5 and below; otherwise, I don't see that Exception inheritance like this solves any problem. \$\endgroup\$ – Mark Seemann Dec 11 '17 at 11:58

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.