Too much logic in a nested catch block

Question

My lead dev complained that I have too much logic in a nested catch block. My code goes something like this:

try
{
    // some setup and a network call
}
catch (CustomEx ex)
{
    try
    {
        KindaLengthyRecursiveRetryMethodThatHandlesCustomEx(numTimesToTry);
    }
    catch (AnyException ex2)
    {
        // Log Exception
        throw;
    }
}

KindaLengthyRecursiveRetryMethodThatHandlesCustomEx() makes the same network call as the parent try. This is necessary because we have no way of knowing CustomEx will occur until the network call fails, and if there are multiple things wrong with the request, the network call only returns one error at a time (hence the retry).

Does anyone have feedback on how to improve this code structure?

Answer 1

If you're basically doing the same in the catch x-times, why you don't do it in the inital try?

try
{
    NetworkCall(numTimesToTry);
}
catch (Exception ex)
{
    // log this
}

So something like this:

void NetworkCall(int maxRetries)
{
    if (!SetupDone())
        DoSetup();

    int retryCount = 0;
    while (retryCount < maxRetries)
    {
        try
        {
            NetworkCall();
        }
        catch (CustomEx ex)
        {
            retryCount++;
            if(retryCount == maxRetries)
            {  
               // log this too
            }
        }
    }
}

Answer 2

By "too much logic", my guess is that your lead developer wasn't referring to the length of the method precisely, but rather the fact that you have a nested try/catch in your catch clause.

A nested try/catch isn't really inefficient, but it is a little ugly to look at. Consider pulling the entire try/catch into its own method:

try
{
    // some setup and a network call
}
catch (CustomEx ex)
{
    HandleError(ex);
}

...

public void HandleError(CustomEx ex) 
{
    try
    {
        KindaLengthyRecursiveRetryMethodThatHandlesCustomEx(numTimesToTry);
    }
    catch (AnyException ex2)
    {
        // Log Exception
        throw;
    }
}

Answer 3

IdentityModel has a similar problem with Refresh Tokens

here they early return if good, rather than retry in a catch:

Generally if you expect an Exception to happen, it probably shouldn't be an exception.

        var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false);

        if (response.StatusCode != HttpStatusCode.Unauthorized)
        {
            return response;
        }

        if (await RefreshTokensAsync(cancellationToken) == false)
        {
            return response;
        }