How much error handling is too much?

Question

I have the following C# code (.NET 6):

public async Task<IEnumerable<Foobar>> RetrieveFoobar(string accessToken, CancellationToken cancellationToken)
{
    try
    {
        using var httpClient = new HttpClient(new HttpClientHandler())
        {
            BaseAddress = new Uri(_baseUrl)
        };

        httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", accessToken);

        var response = await httpClient.GetAsync("/path", cancellationToken);
        
        if (!response.IsSuccessStatusCode)
        {
            Log.Error($"Foobar failed: {response.StatusCode} - {await response.Content.ReadAsStringAsync(cancellationToken)}");
            return null;
        }

        var results = await JsonSerializer.DeserializeAsync<FooBarWrapper>(await response.Content.ReadAsStreamAsync(cancellationToken), cancellationToken: cancellationToken);

        if (results == null)
        {
            Log.Error("Json deserialisation failed when parsing Foobar results");
            return null;
        }
        
        if (!results.Success)
        {
            Log.Error("Foobar response was marked as unsuccessful.");
            return null;
        }

        return results.Records;
    }
    catch (Exception e)
    {
        Log.Error(e, "Foobar retrieval failed");
        return null;
    }
}

It's a fairly simple function that retrieves some results from a REST call.

The problem is, only around 8 of these 40 lines of code provide the core functionality, the rest is essentially error handling. Most of the code is like this, when reading through it I have to wade through blocks of error handling.

I'm writing this code in a service in which reliability and robustness are extremely important.

Is this normal? Or am I going overboard? Is this a good thing? Should readability be sacrificed in the name of robustness? Is there a better way of doing this?

Answer 1

Let's start with a code review. Your style is pleasant with good naming and indentation. About the only thing I would suggest is that method RetrieveFoobar could be named RetrieveFoobarAsync.

The reason you log things is to help you diagnose problems after the fact. If an exception is thrown, you log "Foobar retrieval failed". Is that log message by itself sufficient for someone to take corrective action? I would suggest you would also want to log the the actual exception in the catch block. Keep in mind it could be a composite exception, so you may need to unpeel it a bit more.

I think it is proper to have all the extra code to log the non-exceptional issues (!response.IsSuccessStatusCode, results == null, and !results.Success). This enables customized, very specific messages that should direct the person reading the log to a more immediate remedy than a vague, generic message.

Answer 2

If the function is central to your application then it's perfectly normal to have extensive error handling, compared to the rest of the code.

In practice the amount of error handling is dictated by the nature of the operation that you are performing. If you want to fetch a URL, the likelihood of failure is fairly high. We know many things can go wrong here: network connectivity issues, DNS resolution failure, JSON parsing errors etc. And we actually expect problems to occur. Writing data to a file can fail too, for lack of disk space, file in use or insufficient permissions. Again we know this is a sensitive operation and anticipate issues.

On the other hand a function that concatenates two strings is much less likely to fail. It does not need the same kind of attention. For unexpected conditions you can let a generic, application-wide exception handler take over (handler of last resort). There is no need to have try/catch blocks everywhere, use them in the sections where you expect problems will occur, and when you want to recover from the error gracefully.

I would encourage you to perform unit testing on this particular function and try different scenarios eg invalid JSON or empty output.

The code can be further simplified or shortened but this is a matter of personal preference. Instead of checking the status code in this block:

if (!response.IsSuccessStatusCode)
{
    Log.Error($"Foobar failed: {response.StatusCode} - {await response.Content.ReadAsStringAsync(cancellationToken)}");
    return null;
}

You could use the EnsureSuccessStatusCode method, which will:

Throw an exception if the IsSuccessStatusCode property for the HTTP response is false

Answer 3

Reduce your error handling to a single line.

When you have any code where you are following the same basic format a lot like this to the point it makes your code too verbose for your own personal comfort, I find it is best to use a simple function to simplify it, even if you think it is already relatively simple.

logError(results == null, "Json deserialization failed when parsing Foobar results");

is 80% fewer lines of code than

if (results == null)
{
    Log.Error("Json deserialisation failed when parsing Foobar results");
    return null;
}

So if you have a page that is 1000 lines long, but 75% error handling, a simple function like this can reduce your code down to just about 400 lines. You can also train your eye to ignore lines that start in "logError" when looking for bugs because you know those are just your exception handling lines, but if() statements look too much like your other code to just ignore. Between these two factors, a simple function like this can make it much easier to skim through your code later and find what you are actually looking for.

Alternatively, sometimes it is worth it to break standard formatting when its just filling up a bunch of needless space. These if() statements are so repetitive and simple that it would not necessarily make your code harder to read to put it all on 1 line.

if (results == null) { Log.Error("Json deserialisation failed when parsing Foobar results"); return null;}

The advantage here is that other developers will not need to look up the specs of your function to understand exactly what they are looking at. That said, make sure you understand how dedicated your employer is to standard formatting before doing this since some developers/managers/companies are very strict about standard formatting.

Answer 4

Well, method is good enough, but there is always room for improvements. Let me add some minor details.

1. Firstly, using DI (Dependency Injection) can help you make your code more testable and readable at the same time.
   For example, instead of creating HttpClient in place, you can register it through DI like this

services.AddHttpClient<IYourService, YourService>(client =>
  client.BaseAddress = new Uri(baseUrl));

and then just inject it in a service constructor

public YourService(
       HttpClient client,
       //...

This approach is called typed clients.

After doing this all you have in the method to run HTTP request is this single line

var response = await _httpClient.GetAsync("/path", cancellationToken);

2. Secondly, you method returns a collection and it can be not a very expected behavior for someone if they see null in the result of such public method. I'd suggest returning empty collection instead. Or at least making the result nullable in an obvious way (IEnumerable<Foobar>?). By the way, you can read about Nullable reference types. It's about robustness.
3. Next, you can create some kind of deserialization helper and hide related details inside. It's hard for me to suggest something specific here, because I can't see your types, but it shouldn't be a big deal. This way you can collapse your code related to deserialization and result checking into one or two lines.
   You can make this class/method generic if your DTOs are similar and then use the same helper in other places.
4. And finally, I believe you already know about catching generic exceptions :)

After all that your method can look something like this

public async Task<IEnumerable<Foobar>> RetrieveFoobar(CancellationToken cancellationToken)
{
    try
    {
        var response = await _httpClient.GetAsync("/path", cancellationToken);

        if (!response.IsSuccessStatusCode)
        {
            Log.Error($"Foobar failed: {response.StatusCode} - {await response.Content.ReadAsStringAsync(cancellationToken)}");
            return Array.Empty<Foobar>();
        }

        var stream = await response.Content.ReadAsStreamAsync(cancellationToken);

        return await DeserializeHelper<Foobar>(stream, cancellationToken);
    }
    catch (Exception e)
    {
        Log.Error(e, "Foobar retrieval failed");
        throw;
    }
}

I still don't like this line

Log.Error($"Foobar failed: {response.StatusCode} - {await response.Content.ReadAsStringAsync(cancellationToken)}");

But let's keep some room for further improvements :)

Answer 5

An alternative style is to assign your objects from helper (or phi) functions that only return on success, and throw an exception on error. That moves all error handling out of the expected path. It also means the response to an error you don’t catch is to stop trying to run the operation, and report the first error you got, not to keep going even though the OS told you something went wrong and fail for no apparent reason later.

You mention in the comments a concern that there might be local state that doesn’t get logged. This doesn’t have to be the case: you can create an exception with a formatted string that says whatever you want it to.

This doesn’t mean I think there’s anything wrong with your coding style!

Answer 6

It's difficult to review it without a broader context but in addition to other reviews here's another approach that shows some more options to consider:

• Rename the method to clearly express the intention of it like FetchSuccessfulFoobarOrEmpty.
• Use positive conditions for the happy-path. This would put all the good stuff first and all the bad stuff at the end. This way you would avoid spagetti code.
• Use patthern matching like is { Success: true } wrapper to create inline variables inside ifs.
• Add an id to the log entry about a wrapper marked as unsuccessful - if you have one. Without such an id the log is not really useful as you cannot pin it to any particular wrapper.
• Log the unsuccessful wrapper with the Warn level as you recover from it and virtually ignore such a request so I guess it's not a big issue.
• I would also remove all other logs as without additional information about the request, ids, etc. they do not provide any useful information to the reader.
• Let the caller handle any exceptions, especially the http related ones. This would allow it to retry if necessary. You even might want to throw your own exception.

I suggest something like this:

    public async Task<IEnumerable<Foobar>> FetchSuccessfulFoobarOrEmpty(string accessToken, CancellationToken cancellationToken)
    {
        using var httpClient = new HttpClient(new HttpClientHandler())
        {
            BaseAddress = new Uri(_baseUrl),
        };

        httpClient.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", accessToken);
        
        var response = await httpClient.GetAsync("/path", cancellationToken);

        if (response.IsSuccessStatusCode)
        {
            var responseStream = await response.Content.ReadAsStreamAsync(cancellationToken);

            if (await JsonSerializer.DeserializeAsync<FooBarWrapper>(responseStream, cancellationToken) is { Success: true } wrapper)
            {
                return wrapper.Records;
            }
            else
            {
                Log.Error($"Foobar {wrapper.Id} response was marked as unsuccessful.");
                return Enumerable.Empty<Foobar>();
            }
        }
        else
        {
            // you might want to throw an exception here if the caller should try again
            throw new FoobarRequestException(response.StatusCode, await response.Content.ReadAsStringAsync(cancellationToken));
        }
    }
```

Answer 7

I like the code. I never understood why the new way of coding is going to shorten source as much as possible. For me it makes Code not more clean if you handle everything with exceptions. Your code is easy to read and understand.