Handling exceptions: What should the controller return?

Question

I have a Web API 2 controller action which returns a list of users

public List<User> GetAll()
{
    try
    {
        return _businessLogic.GetAll();
    }
    catch (Exception ex)
    {
        ExceptionHelper.HandleException(ex, _logger, ControllerContext);
    }
}

class ExceptionHelper {
    public static void HandleException(Exception ex, ILogger _logger, HttpControllerContext controllerContext) {
        _logger.LogError(ex);
        // If possible handle the exception here
        // Code for handling

        // Throw it again
        throw new HttpResponseException(
            controllerContext.Request.CreateErrorResponse(HttpStatusCode.InternalServerError, errorMessagError)
        );
    }
}

C# compiler is complaining that not all code paths in GetAll() return a value. The thing is, I don't want to return anything when an exception occurs, because the HandleException will log the error and throw an exception again. How do I explicitly say that I don't want to return anything.

Are you sure, that you want to handle exceptions here at all? How do you plan to handle `_businessLogic.GetAll()` exceptions? If you just want to log exception, there's another way - to add custom `IExceptionLogger`. — Dennis, Oct 30 '15 at 07:25
This could be useful: http://stackoverflow.com/questions/10732644/best-practice-to-return-errors-in-asp-net-web-api — 3615, Oct 30 '15 at 07:31

Dennis · Answer 1 · 2015-10-30T08:18:05.730

As a rule, you should handle exceptions, if you know, how to handle it, and what do you want to get after exception was handled. Since exceptions are about corrupted state, you either can restore state, or you shouldn't handle exception.

For the sample above, the way to go is to remove exception handling at all. That is:

public List<User> GetAll()
{
    return _businessLogic.GetAll();
}

What can go wrong in GetAll method:

inaccessible data source (infrastructure problems);
invalid query/EF model/etc (code problems).

Anyway, these all perfectly fit HTTP 500 "Internal server error", because this is actually server errors. The earlier you'll get feedback from user, the earlier you'll fix them.

The common practice is to log exceptions. Web API allows you to do it in custom way by injecting your own IExceptionLogger implementation:

    public static void Register(HttpConfiguration config)
    {
        // ...
        // ApiNLogLogger is a custom logger, that uses NLog
        config.Services.Add(typeof(IExceptionLogger), new ApiNLogLogger());
    }

score 0 · Answer 2 · answered Oct 30 '15 at 08:23

We know that HandleException will throw the exception again but compiler does not know that. So to satisfy the compiler validation, without changing too much code, you can simply add a throw; inside catch block. Although this throw; will be unreachable but compiler validation will pass and your functionality will work as expected. Example is below:

public List<User> GetAll()
{
    try
    {
        return _businessLogic.GetAll();
    }
    catch (Exception ex)
    {
        ExceptionHelper.HandleException(ex, _logger, ControllerContext);
        throw;//Added throw to remove compilation error.
    }
}

class ExceptionHelper {
    public static void HandleException(Exception ex, ILogger _logger, HttpControllerContext controllerContext) {
        _logger.LogError(ex);
        // If possible handle the exception here
        // Code for handling

        // Throw it again
        throw new HttpResponseException(
            controllerContext.Request.CreateErrorResponse(HttpStatusCode.InternalServerError, errorMessagError)
        );
    }
}

score -1 · Answer 3 · answered Oct 30 '15 at 07:27

-1

try it this way

public List<User> GetAll()
{
    List<User> result = new List<User>();
    try
    {
        result _businessLogic.GetAll();
    }
    catch (Exception ex)
    {
        ExceptionHelper.HandleException(ex, _logger, ControllerContext);
    }
    return result;
}

One entry point. One exit point. Personally i prefer returning empty lists im something bad happens rather than return null.

answered Oct 30 '15 at 07:27

Marco Forberg

2,634
5
22
33

This hides problems at the server side. What for? – Dennis Oct 30 '15 at 07:31
This is just to provide an answer to the "not all code pahs return a value" problem. actually i rarely catch exceptions in DAO or similar classes and let the client code handle them properly – Marco Forberg Oct 30 '15 at 07:37
You don't get it. This - "Personally i prefer returning empty lists if something bad happens" - is a very, very bad idea, regardless on return value. If something bad happens, controller *must* throw exceptions (which leads to HTTP 500 at client side). Otherwise "something bad" won't be fixed until somebody will look into server's log. – Dennis Oct 30 '15 at 07:40
Oh I did get it. But to me something bad reaches from "it's not good but we can carry on with it" to "OMG". So i carefully look whether i catch exceptions and handle them properly or let client code handle them in a more appropriate way. In a case like this i wouldn't even catch it. – Marco Forberg Oct 30 '15 at 07:49

Handling exceptions: What should the controller return?

3 Answers3