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

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

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 4

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 5

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