Throw Custom Exception

Question

First, I know - there are a lot of online resources that talk about the topic Exception handling - but yet there is still something that's unclear to me.

Consider having this code in an external library, called from another project.

public Task<List<Ident>> GetFoo(){
 List<Ident> idents = new List<Ident>();
 idents = await IdentRepo.GetIdents(); // some rest-API call with HttpClient..
 return idents;
} 

At some point I'd like to implement some sort of exception handling - e.g if the webservice I'm connecting to is not available. I could do something like

public Task<List<Ident>> GetFoo(){
 List<Ident> idents = new List<Ident>();
 try{ 
 idents = await IdentRepo.GetIdents(); // some rest-API call with HttpClient..
 } catch (HttpRequestException e){
 Diag.Log(e);
 }
 return idents;
} 

I don't like that approach because the caller doesn't know that the webservice request failed and doesn't know if the list is actually empty or if something else happened.

A better solution would be something like this

public Task<List<Ident>> GetFoo(){
 List<Ident> idents = new List<Ident>();
 try{ 
 idents = await IdentRepo.GetIdents(); // some rest-API call with HttpClient..
 } catch (HttpRequestException e){
 Diag.Log(e);
 throw;
 }
 return idents;
} 

Now the caller has to handle the exception as well. That would work BUT I don't like the idea of the caller knowing the specific type of the exception. The caller doesn't care if it's a rest API request with HTTP or if I'm loading the data from a local file - if it fails it fails.

So - imo - the best solution would be to throw a custom exception.

public Task<List<Ident>> GetFoo(){
 List<Ident> idents = new List<Ident>();
 try{ 
 idents = await IdentRepo.GetIdents(); // some rest-API call with HttpClient..
 } catch (HttpRequestException e){
 Diag.Log(e);
 throw new IdentProviderNotAvailable();
 }
 return idents;
} 

With that code the only has to care about the IdentProviderNotAvailable Exception. It doesn't matter if I add any new Exception types in the catch block as I only throw IdentProviderNotAvailable.

Is that a good approach? Should I include the stack-trace of the original exception or is there a better way to deal with this?

• David Arno: It looks good to me. Two things though: 1. it would be good to add the original HttpRequestException (or whatever exception it might be) to the InnerException of your IdentProviderNotAvailable. and secondly, it's convention to put Exception on the end of the name of all exceptions. Whether that's a good convention or not is another issue entirely ;)
• Robert Harvey: Whether it's a "good" approach depends on whether or not it meets your specific needs. Does it?
• Doc Brown: @DavidArno: your comment sounds good enough for an answer.
• John Wu: How is the client software to be deployed and supported, and by whom? If there is a network problem, do you expect that person to be able to troubleshoot it? If so, it would make things difficult if you hid the original exception from them. You should at least provide it as an inner exception.

Answer 1

In your example,

public Task<List<Ident>> GetFoo(){
 List<Ident> idents = new List<Ident>();
 try{ 
 idents = await IdentRepo.GetIdents(); // some rest-API call with HttpClient..
 } catch (HttpRequestException e){
 Diag.Log(e);
 throw;
 }
 return idents;
}

you have one big advantage with this approach. But rethrowing the exception, via throw, you preserve the full stack trace.

But as you say, it has the disadvantage of effectively being an implementation detail. The exact exception thrown depends on the underlying mechanism of retrieving the information. This can force the developer into having to just catch Exception, which is generally considered bad practice.

Introducing a custom exception therefore can be a good idea in these situations. There are two things to note about your example code though:

1. It's a strong convention in .NET to suffix all exception classes with Exception. So make its name, IdentProviderNotAvailableException. You may think that ugly and unnecessary, and I'd agree with you if you think that. But following convention, rather than "doing the right thing" will make your code easier for others to understand, so just go with the flow on this one.

2. As you are losing information (including the stack trace) that led to that exception being thrown, it's therefore a good idea to include the HttpRequestException value in the inner exception of your custom exception. So change the constructor to something like:

   public IdentProviderNotAvailableException(Exception inner) : base("some message", inner) {}

to capture that information.

Whilst just swallowing the exception and returning an empty list may be a bad idea, there is still an alternative to throwing an exception in this case: the try pattern. In C#, the convention is to implement the try pattern via a boolean return and an out pattern:

public bool TryGetFoo(out List<Ident> idents)
{
 List<Ident> idents = new List<Ident>();
 try
 { 
 idents = await IdentRepo.GetIdents();
 return true;
 } 
 catch (HttpRequestException e)
 {
 Diag.Log(e);
 idents = null;
 return false;
 }
}

• GHP: if you're following the .NET try pattern, should the method be named TryGetFoo() for clarity?
• David Arno: @Graham, oops. Yes, you are right. Updated.

Answer 2

Always remember the most important part of Structured Exception Handling is that last bit - handling the Exception or, in my parlance "doing something useful with it".

At some point I'd like to implement some sort of exception handling ...

And that's fine.
If your code catches and handles an Exception, then the Client doesn't need to know anything about it. You've "handled" it.

Now the caller has to handle the exception as well.

This is a case where you've done as much as you can to keep your house in order, cleaning up resources and all that good stuff and, probably, recording the complete Exception somewhere, for your own, diagnostics, purposes. As you say, though, the Client cannot "handle" this Exception; they can only be notified that something went wrong. There is no point in sending them all the details of the "inner" Exception so construct and throw a new Exception at the process boundary interface - you catch the detailed Exception, then create and throw a bland ComputerSaysNoException instead. This also truncates the Stack Trace, hiding all your implementation details as well.

All the Client can (probably) do is to display that message; there's very little, if anything, that it can do to "recover" from the error, so a single Class that it can [safely] display will probably do.

• Doc Brown: So you think it is a good idea to not let the client decide if it can do something useful with the "inner exception" and the full stack trace? (Like logging for debugging purposes?)
• Phill W.: @DocBrown: If the client can /usefully/ handle an Exception then fine, it can do so. Here, though, we're talking more about notification, not handling. In that case, no, I don't want to rely on the client to log the Exception, because there's no way of enforcing what it will do. The worst case I've seen was when such a client logged only the Exception /Message/ not even the Type, let alone the Stack Trace! Thanks, but I'll have my code do that for itself, and then notify the client with some more bland and less implementation-revealing.