具有多个返回点的重构方法答案

【问题标题】：Refactor method with multiple return points具有多个返回点的重构方法
【发布时间】：2009-10-29 17:50:02
【问题描述】：

**编辑：下面有几个可以工作的选项。请根据您对此事的看法投票/评论。

我正在清理并为具有以下基本结构的 c# 方法添加功能：

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

有很多步骤，在大多数情况下，步骤 x 必须成功才能继续执行步骤 x+1。现在，我需要添加一些始终在方法结束时运行的功能，但这取决于返回值。我正在寻找有关如何干净地重构它以获得预期效果的建议。显而易见的选择是将依赖于返回值的功能放在调用代码中，但我无法修改调用者。

一个选项：

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

这对我来说似乎有点难看。相反，我可以直接从 performStepX() 方法中抛出。然而，这留下了在 processStuff() 的 catch 块中适当地设置 returnStatus 变量的问题。您可能已经注意到，处理步骤失败时返回的值取决于哪个步骤失败。

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

如果您有任何建议，我将不胜感激。

【问题讨论】：

标签： c# refactoring

【解决方案1】：

不要使用异常来控制程序的流程。就个人而言，我会保留代码原样。要在最后添加新功能，您可以这样做：

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

【讨论】：

我绝对同意你回答的前半部分。这里有一些很好的答案可以在不违反该原则的情况下提高可读性。
事实上，它的行为并不符合要求。我需要在方法结束时根据返回值做一些事情。而且，正如我所说，我无法更改调用代码。
抱歉@Odrade 我忘了您需要更改功能。看看我更新的答案是否对你有帮助。
+1 指出异常处理不应该是执行的控制流
这似乎是一个非常干净的方法。谢谢。

【解决方案2】：

您可以将这些步骤重构为一个接口，以便每个步骤（而不是一个方法）公开一个 Run() 方法和一个 Status 属性 - 您可以循环运行它们，直到遇到异常。这样，您就可以保留有关运行的步骤以及每个步骤的状态的完整信息。

【讨论】：

稍作修改，接口 Run() 方法可以返回 true 或 false，允许仅查询失败步骤的状态字段。如果状态为错误，则可能会在循环中引发异常。这允许循环继续警告。
这实际上取决于这些步骤是否实际上属于它们自己的类。如果不是这样，为每个步骤创建类、创建接口然后跟踪所有对象可能会变得过于繁重。
@jasonh 是的，但是可以使用工厂处理步骤和执行顺序的管理。如果这个方案实施得当，即使是增加步骤或者改变执行顺序也可以在不改变代码和重新编译的情况下完成。
@jasonh：首先，所有步骤共享一个共同的功能（执行和状态），并且该过程已经被描述为连续的“步骤”所以对我来说这里有一个接口的论点似乎强的。然后，如果真的只有 3 个步骤并且设计不会改变，那么这可能是矫枉过正 - 但除此之外，这是一种相当灵活且可维护的方法。

【解决方案3】：

您可以在finally 部分执行处理。 finally 很有趣，即使您在 try 块中返回，finally 块仍将在函数实际返回之前执行。不过，它会记住返回的值，因此您也可以在函数的最后放弃 return。

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

【讨论】：

【解决方案4】：

获取一个元组类。然后做：

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

哦，是的，您可以对元组使用匿名类型：

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

【讨论】：

这将不允许在 performStep1 出现警告的情况下继续执行代码。
另外，该方法不遵循我显示 %100 的模式。有些步骤会获取必须传递给后续步骤的数据。
@jheddings 如我所见，任何异常都会触发处理结束；只是状态改变。 @odrade，原来的只是 performStep1/2/3()... :)
抽象的好工作......太糟糕了 Actions 不返回 bool 或者这可以完美地工作。
完美...我以前没见过那种通用的。 +1

【解决方案5】：

扩展上面的接口方法：

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

【讨论】：

看到您在处理步骤之间传递数据的更改，这可能无法正常工作。希望它能帮助您入门。

【解决方案6】：

你能让performStep1和performStep2抛出不同的异常吗？

【讨论】：

这确实发生在我身上。其他人如何看待这种方法？
我会抛出有意义的异常。
你的意思是用自定义类型抛出异常，命名清楚地反映它们的目的吗？
我认为抛出和捕获 2 种不同类型的异常是个好主意。此外，像您在示例代码中所做的那样只捕获一个异常类型是一个坏主意。如果您的 performStep1 代码中有一些意外异常，您想告诉用户/程序员出了点问题，而这不仅仅是一个失败的步骤。
抛出异常将不允许继续执行以获得警告。至少没有几个大的 try ... catch 块。

【解决方案7】：

您可以反转您的状态设置。在调用异常抛出方法之前设置错误状态。最后，无异常设置成功。

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 {
 // ...
}

【讨论】：

“默认失败”？这使他可以根据需要从每个步骤中抛出异常，同时确保设置了每个步骤的失败。没有额外的条件或更多代码。当涉及到如此小的重构时，C# 的能力有限。

【解决方案8】：

不知道你的逻辑要求是什么，我会先创建一个抽象类作为基础对象来执行特定步骤并返回执行状态。它应该具有可覆盖的方法来实现任务执行、成功时的操作和失败时的操作。还处理逻辑把逻辑放在这个类中来处理任务的成功或失败：

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

创建任务类来执行步骤后，按执行顺序将它们添加到 SortedList，然后在任务完成时遍历它们检查状态：

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

我留在错误处理中，因为我不确定您是在任务执行时捕获错误还是只是在寻找您在特定步骤失败时抛出的异常。

【讨论】：

【解决方案9】：

我建议将步骤重构为单独的类，毕竟，无论如何，您的类应该只有一个责任。我认为这听起来像是应该控制步骤的运行。

【讨论】：

【解决方案10】：

嵌套 if 呢？

能用也不能用

它会删除每一个回报，只留下一个

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

【讨论】：

这可能行得通，但它会导致一个根深蒂固的怪物。