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?

• Tim Schmelter: Where is too much logic in the catch? What's the problem with this method that you don't show us?
• gnat: Possible duplicate of How to clean up nested try/except/else?
• Tim Schmelter: For me it's not obvious why you can't use: try{ KindaLengthyRecursiveRetryMethodDoesSetupAndNetweorkCall(numTimesToTry)} catch (AnyException ex){//log}
• Doc Brown: 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).

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

• David Arno: "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.
• khalid13: 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.
• Robert Harvey: @khalid13: Then you have to play the hand you're dealt.
• Ewan: @RobertHarvey unless you have a decompiler and an indomitable will!