Refactor method with multiple return points

Question

**EDIT: There are several options below that would work. Please vote/comment according to your views on the matter.

I'm working on cleaning up and adding functionality to a c# method with the following basic structure:

    public void processStuff()
    {
        Status returnStatus = Status.Success;
        try
        {
            bool step1succeeded = performStep1();
            if (!step1succeeded)
                return Status.Error;

            bool step2suceeded = performStep2();
            if (!step2suceeded)
                return Status.Warning;

            //.. More steps, some of which could change returnStatus..//

            bool step3succeeded = performStep3();
            if (!step3succeeded)
                return Status.Error;
        }
        catch (Exception ex)
        {
            log(ex);
            returnStatus = Status.Error;
        }
        finally
        {
            //some necessary cleanup
        }

        return returnStatus;
    }

There are many steps, and in most cases step x must succeed in order to proceed to step x+1. Now, I need to add some functionality that will always run at the end of the method, but which depends on the return value. I'm looking for recommendations on how to cleanly refactor this for the desired effect. The obvious choice would be to put the functionality that depends on the return value in the calling code, but I'm not able to modify the callers.

One option:

    public void processStuff()
    {
        Status returnStatus = Status.Success;
        try
        {
            bool step1succeeded = performStep1();
            if (!step1succeeded)
            {
                returnStatus =  Status.Error;
                throw new Exception("Error");
            }

            bool step2succeeded = performStep2();
            if (!step2succeeded)
            {
                returnStatus =  Status.Warning;
                throw new Exception("Warning");
            }

            //.. the rest of the steps ..//
        }
        catch (Exception ex)
        {
            log(ex);
        }
        finally
        {
            //some necessary cleanup
        }
        FinalProcessing(returnStatus);
        return returnStatus;
    }

This seems a little ugly to me. Instead, I could throw right from the performStepX() methods. However, this leaves the problem of setting the returnStatus variable appropriately in the catch block of processStuff(). As you may have noticed, the value returned on failure of a processing step depends on which step failed.

    public void processStuff()
    {
        Status returnStatus = Status.Success;
        try
        {
            bool step1succeeded = performStep1(); //throws on failure
            bool step2succeeded = performStep2(); //throws on failure

            //.. the rest of the steps ..//
        }
        catch (Exception ex)
        {
            log(ex);
            returnStatus = Status.Error;  //This is wrong if step 2 fails!
        }
        finally
        {
            //some necessary cleanup
        }
        FinalProcessing(returnStatus);
        return returnStatus;
    }

I would appreciate any suggestions that you might have.

Answer 1

Can you make performStep1 and performStep2 throw different exceptions?

Answer 2

You could refactor the steps into an interface, so that each step, instead of being a method, exposes a Run() method, and a Status property - and you can run them in a loop, until you hit an exception. That way, you can keep the complete information on what step ran, and what status each step had.

Answer 3

Get a tuple class. Then do:

var steps = new List<Tuple<Action, Status>>() {
  Tuple.Create(performStep1, Status.Error),
  Tuple.Create(performStep2, Status.Warning)
};
var status = Status.Success;
foreach (var item in steps) {
  try { item.Item1(); }
  catch (Exception ex) {
    log(ex);
    status = item.Item2;
    break;
  } 
}
// "Finally" code here

Oh yea, you can use anon types for tuples:

var steps = new [] {                        
    { step = (Action)performStep1, status = Status.Error }, 
    { step = (Action)performStep2, status = Status.Error }, 
};                                          
var status = Status.Success          
foreach (var item in steps) {               
  try { item.step(); }                     
  catch (Exception ex) {                    
    log(ex);                                
    status = item.status;                    
    break;                                  
  }                                         
}                                           
// "Finally" code here

Answer 4

You can perform the processing in your finally section. finally is fun in that even if you've returned in your try block the finally block will still get executed before the function actually returns. It will remember what value was being returned, though, so you can then also ditch the return at the very end of your function.

public void processStuff()
{
    Status returnStatus = Status.Success;
    try
    {
        if (!performStep1())
            return returnStatus = Status.Error;

        if (!performStep2())
            return returnStatus = Status.Warning;

        //.. the rest of the steps ..//
    }
    catch (Exception ex)
    {
        log(ex);
        return returnStatus = Status.Exception;
    }
    finally
    {
        //some necessary cleanup

        FinalProcessing(returnStatus);
    }
}

Answer 5

Don't use exceptions to control the flow of your program. Personally, I would leave the code as it is. To add the new functionality at the end you could do:

    public void processStuff()
    {
        Status returnStatus = Status.Success;
        try
        {
            if (!performStep1())
                returnStatus = Status.Error;
            else if (!performStep2())
                returnStatus = Status.Warning;

            //.. More steps, some of which could change returnStatus..//

            else if (!performStep3())
                returnStatus = Status.Error;
        }
        catch (Exception ex)
        {
            log(ex);
            returnStatus = Status.Error;
        }
        finally
        {
            //some necessary cleanup
        }

        // Do your FinalProcessing(returnStatus);

        return returnStatus;
    }

Answer 6

You could reverse your status setting. Set the error status before calling the exception throwing method. At the end, set it success if no exceptions.

Status status = Status.Error;
try {
  var res1 = performStep1(); 
  status = Status.Warning;
  performStep2(res1);
  status = Status.Whatever
  performStep3();
  status = Status.Success; // no exceptions thrown
} catch (Exception ex) {
  log(ex);
} finally {
 // ...
}

Answer 7

what about nested if?

could work and could not work

it would remove every return to leave only one

if(performStep1())
{
  if(performStep2())
  {
      //..........
  }
  else 
    returnStatus = Status.Warning;
}
else
  returnStatus = Status.Error;

Answer 8

Not knowing to much of what you're logical requirements are, I'd start with creating an abstract class to act as a base object to execute a specific step and return the Status of the execution. It should have overrideable methods to implement task execution, actions on success, and actions on failure. also handle the logic Putting the logic in this class to handle success or failure of the task:

abstract class TaskBase
{
    public Status ExecuteTask()
    {
        if(ExecuteTaskInternal())
            return HandleSuccess();
        else
            return HandleFailure();
    }

    // overide this to implement the task execution logic
    private virtual bool ExecuteTaskInternal()
    {
        return true;
    }

    // overide this to implement logic for successful completion
    // and return the appropriate success code
    private virtual Status HandleSuccess()
    {
        return Status.Success;
    }

    // overide this to implement the task execution logic
    // and return the appropriate failure code
    private virtual Status HandleFailure()
    {
        return Status.Error;
    }
}

After you've created Task classes to execute your steps, add them to a SortedList in order of execution, then iterate through them checking the staus when the task has completed:

public void processStuff()
{
    Status returnStatus 
    SortedList<int, TaskBase> list = new SortedList<int, TaskBase>();
    // code or method call to load task list, sorted in order of execution.
    try
    {
        foreach(KeyValuePair<int, TaskBase> task in list)
        {
            Status returnStatus task.Value.ExecuteTask();
            if(returnStatus != Status.Success)
            {
                break;
            }
        }
    }
    catch (Exception ex)
    {
        log(ex);
        returnStatus = Status.Error;
    }
    finally
    {
        //some necessary cleanup
    }

    return returnStatus;
}

I left in the error handling as I'm not sure if your trapping errors on task execution or just looking for the exception you were throwing on a particular step failing.

Answer 9

Expanding on the interface approach above:

public enum Status { OK, Error, Warning, Fatal }

public interface IProcessStage {
    String Error { get; }
    Status Status { get; }
    bool Run();
}

public class SuccessfulStage : IProcessStage {
    public String Error { get { return null; } }
    public Status Status { get { return Status.OK; } }
    public bool Run() { return performStep1(); }
}

public class WarningStage : IProcessStage {
    public String Error { get { return "Warning"; } }
    public Status Status { get { return Status.Warning; } }
    public bool Run() { return performStep2(); }
}

public class ErrorStage : IProcessStage {
    public String Error { get { return "Error"; } }
    public Status Status { get { return Status.Error; } }
    public bool Run() { return performStep3(); }
}

class Program {
    static Status RunAll(List<IProcessStage> stages) {
        Status stat = Status.OK;
        foreach (IProcessStage stage in stages) {
            if (stage.Run() == false) {
                stat = stage.Status;
                if (stat.Equals(Status.Error)) {
                    break;
                }
            }
        }

        // perform final processing
        return stat;
    }

    static void Main(string[] args) {
        List<IProcessStage> stages = new List<IProcessStage> {
            new SuccessfulStage(),
            new WarningStage(),
            new ErrorStage()
        };

        Status stat = Status.OK;
        try {
            stat = RunAll(stages);
        } catch (Exception e) {
            // log exception
            stat = Status.Fatal;
        } finally {
            // cleanup
        }
    }
}

Answer 10

I'd advise on some refactoring of the steps into seperate classes, after all, your class should only have a single responsibility anyway. I think it sounds like it's responsibility should controlling the running of the steps.