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?

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

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.