Reputation: 32798

Is this an efficient way to catch and handle exceptions / errors?

I am now using the following code to catch errors and throw exceptions in my service layer:

 ...
            if (pk == null || rk == null) return null;
            try
            {
                var item = repo.GetPkRk(pk, rk);
                return (T)item;
            }
            catch (Exception ex)
            {
                throw new ServiceException("", typeof(T).Name + rk + " data retrieval error");
            }
  ...

The ServiceException class:

public class ServiceException : ApplicationException {     
        public Dictionary<string, string> Errors { get; set; }
        public ServiceException() : this(null) {}enter code here
        public ServiceException(string key, string message)
        {
            Errors = new Dictionary<string, string>(); 
            Errors.Add(key, message);
        }
        public ServiceException(Exception ex)
            : base("Service Exception", ex)
        {
            Errors = new Dictionary<string, string>();
        }
    }

The error message then caught in my controller:

catch (Exception e) { log(e); }

Finally handled in the log method:

protected void log(Exception ex)
        {
            if (ex is ServiceException)
            {
                ModelState.Merge(((ServiceException)ex).Errors);  
            } else {
                Trace.Write(ex);
                ModelState.AddModelError("", "Database access error: " + ex.Message);
            }
        }

Can anyone comment if this is a good or bad method. I had a previous comment before about catching inner exceptions. Is this possible and if so then how can I catch and retain that inner exception detail.

Update 1

I modified the exception class so there's a constructor that takes ex. Not sure if this ideal but I think it works. Any more suggestions on how to improve would be appreciated.

Update 2

The code below fails with a message saying that

Error   2   Property or indexer 'System.Exception.InnerException' cannot be assigned to -- it is read only

I'm not sure how to fix this.

public class ServiceException : ApplicationException {

    public Dictionary<string, string> Errors { get; set; }
    public ServiceException() : this(null) {}
    public ServiceException(Exception ex, string key, string message)
    {
        Errors = new Dictionary<string, string>();
        InnerException = ex;
        Errors.Add(key, message);
    }
    public ServiceException(string key, string message)
    {
        Errors = new Dictionary<string, string>(); 
        Errors.Add(key, message);
    }
    public ServiceException(Exception ex)
        : base("Service Exception", ex)
    {
        Errors = new Dictionary<string, string>();
    }
}

Upvotes: 1

Answers (2)

P.K

Reputation: 19127

First and foremost

Include the caught exception as an inner exception

So instead of

throw new ServiceException("", typeof(T).Name + rk + " data retrieval error");

You should set the inner exception. Change your constructor

public ServiceException(string key, string message, Exception innerException)
            :base(message, innerException)
        {
            Errors = new Dictionary<string, string>(); 
            Errors.Add(key, message);
        }

Now,

//form  your exception message
 var message = typeof(T).Name + rk + " data retrieval error";
//create service exception with the new overloaded constructor
 var exception  = new ServiceException("", typeof(T).Name + rk + " data retrieval error", message);
 throw exception;

This will retain your inner exception.

Upvotes: 1