0

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?

4
  • 2
    Where is too much logic in the catch? What's the problem with this method that you don't show us? – Tim Schmelter Sep 5 '18 at 14:15
  • 1
    Possible duplicate of How to clean up nested try/except/else? – gnat Sep 5 '18 at 14:15
  • 5
    For me it's not obvious why you can't use: try{ KindaLengthyRecursiveRetryMethodDoesSetupAndNetweorkCall(numTimesToTry)} catch (AnyException ex){//log} – Tim Schmelter Sep 5 '18 at 14:17
  • 2
    For me it's not obvious why your lead dev did not come up with a constructive suggestion how to improve the code (or why you did not ask him/her what he/she meant). – Doc Brown Sep 5 '18 at 17:16
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
            }
        }
    }
}
4

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;
    }
}
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;
        }
4
  • 3
    "Generally if you expect an Exception to happen, it probably shouldn't be an exception". +1 just for that. Never a truer word spoken; folk all too often use exceptions simply for control flow, rather than for exceptional cases. – David Arno Sep 5 '18 at 14:38
  • Unfortunately I'm forced to rely on an external library that throws an exception when I attempt something and doesn't allow me to check validity otherwise. – khalid13 Sep 5 '18 at 14:44
  • @khalid13: Then you have to play the hand you're dealt. – Robert Harvey Sep 5 '18 at 14:45
  • 1
    @RobertHarvey unless you have a decompiler and an indomitable will! – Ewan Sep 5 '18 at 14:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.