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

Question

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>();
    }
}

Do not derive from `ApplicationException`: http://stackoverflow.com/questions/52753/should-i-derive-custom-exceptions-from-exception-or-applicationexception-in-net, http://blogs.msdn.com/b/kcwalina/archive/2006/06/23/644822.aspx — Bradley Grainger, Dec 24 '11 at 08:04

P.K · Answer 1 · 2011-12-24T10:51:08.887

1

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.

edited Dec 24 '11 at 10:51

answered Dec 24 '11 at 03:23

P.K

18,587
11
45
51

Thanks for your advices. Could I pass the InnerException in as a constructor argument? – Samantha J T Star Dec 24 '11 at 03:46
@Matthias - Just tried compilation and I get an error message with your code: Error 2 Property or indexer 'System.Exception.InnerException' cannot be assigned to -- it is read only – Samantha J T Star Dec 24 '11 at 05:59
InnerException is a readonly property. You cannot set it. I had not ran the code and hence didn't realize it. I have now corrected the code. Please try it. – P.K Dec 24 '11 at 06:31
Thanks. I had to change "new innerException" to "innerException" to get it compile. Is that a correct change that I made? – Samantha J T Star Dec 24 '11 at 08:01
@P.K `:base(message, new innerException)` is a syntax error; `new` should not be there. Also, the third constructor parameter is an `Exception`, but you're passing a `string` (`message`), which has the same contents as the second argument. Why create a local variable `exception` instead of just `throw new ServiceException(...)`? – Bradley Grainger Dec 24 '11 at 08:05

score 1 · Answer 2 · edited May 23 '17 at 12:27

1

In almost all cases, you should not catch exceptions at all.

You should only catch exceptions you can actually do something about.

Please see the many excellent questions under the exception-handling tag: https://stackoverflow.com/questions/tagged/exception-handling?sort=votes.

Also, be certain to read the Microsoft guidelines on this: "Design Guidelines for Exceptions".

edited May 23 '17 at 12:27

Community

1
1

answered Dec 24 '11 at 08:47

John Saunders

160,644
26
247
397

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

2 Answers2