Refactoring exceptions for workflow

Question

I am currently refactoring an application which uses exceptions for logic flow. The code is difficult to read and maintain and makes a S.O.L.I.D fanboy like myself cry when reading it (not to mention the longest catch block I've ever seen in my career).

My question is what pattern(s) could you use to make it easier to maintain or how would you go about refactoring?

public void CallExternalServices(ICriteria criteria)
{
    try
    {
     someResult = ExternalService1.SomeMethod(criteria);
    }
    catch (Service1Exception service1Exception)
    {
     if (criteria.SomeValue == "1")
     {
      if (service1Exception.InnerException != null 
       && service1Exception.InnerException.InnerException != null
       && service1Exception.InnerException.InnerException is TargetSystemException)
      {
       TargetSystemException targetSystemException = (TargetSystemException)service1Exception.InnerException.InnerException;
       if (targetSystemException.ErrorStatus.IsServiceDownForMaintenance())
       {
        // Call internal method to perform some action
        SendNotification("Service down for maintenance.")
       }
      }
     }
     else if (criteria.SomeValue == "2")
     {
      if (service1Exception.InnerException != null 
       && service1Exception.InnerException.InnerException != null
       && service1Exception.InnerException.InnerException is TargetSystemException)
      {
       TargetSystemException tx = (TargetSystemException)service1Exception.InnerException.InnerException;
       if (targetSystemException.ErrorStatus.IsBusy())
       {
        // Call to some internal method to perform an action
        SendDifferentNotification()

        criteria.SetSomeFlagToBe = true;

        try
        {
         someResult = ExternalService2.SomeOtherMethod(criteria);
        }
        catch (Service2Exception service2Exception)
        {
         if (service2Exception.InnerException != null 
          && service2Exception.InnerException.InnerException != null
          && service2Exception.InnerException.InnerException is TargetSystemException)
         {
          TargetSystemException tx = (TargetSystemException)service1Exception.InnerException.InnerException;
          if (targetSystemException.ErrorStatus.HasNumberOfDailyTransactionsBeenExceeded())
          {
           // Call internal method to perform some action
           SendNotification("Number of daily transactions exceeded.")
          }
         }
         else if (service2Exception.InnerException != null
          && service2Exception.InnerException.InnerException != null
          && service2Exception.InnerException.InnerException is FaultException)
         {
          FaultException faultException = service2Exception.InnerException.InnerException as FaultException;

          if (faultException.Detail != null
           && faultException.Detail.FaultMessage.Equals("SomeValueToCheckAgainst", StringComparison.OrdinalIgnoreCase))
          {
           return someResult;
          }
          else
          {
           throw service2Exception;
          }
         }
         else
         {
          throw service2Exception;
         }
        }

        if (someResult != null)
        {
         // perform another action
         SomeActionInvoker.ExecuteAcitonAgainst(someResult);
        }
       }
      }
      else if (service1Exception.InnerException != null
         && service1Exception.InnerException.InnerException != null
         && service1Exception.InnerException.InnerException is FaultException)
      {
       FaultException faultException = service1Exception.InnerException.InnerException as FaultException;

       if (faultException.Detail != null
        && faultException.Detail.FaultMessage.Equals("SomeOtherValueToCheckAgainst", StringComparison.OrdinalIgnoreCase))
       {
        return someResult;
       }
       else
       {
        throw service1Exception;
       }
      }
      else
      {
       throw service1Exception;
      }
     }
    }
}

Answer 1

Start by refactoring the method into multiple methods. You've got waaaaaay too many levels of indentation going on.

After that, consider if some of the new methods could be extracted into other objects, and use an IoC-style approach rather than a procedural one.

That's kind of a high level answer, but I'm tired and don't have the energy to actually rework the code myself :)

Answer 2

All in all I think you would be helped by breaking up some stuff into some helper methods. For example, you could extract your checks that look like this

if (<exception-instance>.InnerException != null && 
    <exception-instance>.InnerException.InnerException != null && 
    <exception-instance>.InnerException.InnerException is <exception-type>)

Into a boolean method; you call code like this at least 3 times by my cursory glance.

Also, I would recommend extracting that second top-level case into an error-handling method; and perhaps its nested if statements.

Answer 3

Check out Working Effectively with Legacy Code by Michael Feathers, particularly Chapter 22 (I Need to Change a Monster Method and I Can't Write Tests for It). There are a lot of great techniques in there for situations like yours. Personally, in cases like this I usually end up extracting methods from sections of the longer methods, and getting rid of local variables that are used throughout the method; these are almost always trouble.