重构方法的最佳实践

Question

我想知道重构类似这样的代码的最佳实践：

应该在哪里设置退出标准，什么是最佳实践？

    private static bool Foo()
    {
        bool result = false;

        if (DoMehod1())
        {
            if (DoMehod2())
            {
                if (DoMethod3())
                {
                    result = true;
                }
                else
                {
                    Console.WriteLine("DoMethod3 Failed");
                }
            }
            else
            {
                Console.WriteLine("DoMethod2 Failed");
            }
        }
        else
        {
            Console.WriteLine("DoMethod1 Failed");
        }
        return result;
    }

谢谢

- user829174

---

请考虑在http://codereview.stackexchange.com/上发布此内容。 - finnw

如果您解释一下DoMethod方法实际上是做什么的，以及为什么它们在失败时通过布尔结果返回错误代码，而不是抛出异常，那将会很有帮助。 - Merlyn Morgan-Graham

Answer 1

不改变代码功能的情况下，最佳结构是这样的：

private static bool Foo()
{
    if (!DoMethod1())
    {
        Console.WriteLine("DoMethod1 Failed");
        return false;
    }

    if (!DoMethod2())
    {
        Console.WriteLine("DoMethod2 Failed");
        return false;
    }

    if (!DoMethod3())
    {
        Console.WriteLine("DoMethod3 Failed");
        return false;
    }

    return true;
}

- mqp

---

这不会输出所有结果。 - Ufuk Hacıoğulları

我不知道“不输出所有结果”的含义，但这段代码在功能上与帖子中的代码完全相同。 - mqp

@UfukHacıoğulları：为什么这不会输出所有结果？如果DoMethod1返回true并且DoMethod2返回false，那么在最终返回之前执行的唯一一行将是Console.WriteLine("DoMethod2 Failed.");。 - Joshua

+1；这不是一个坏的方法使用给定的代码。尽管我认为（请参见我的答案），这些函数的签名可能从一开始就是错误的。默认情况下，错误处理不应该是合作的 - 它应该通过异常强制执行，因为否则未处理的错误将意外地被忽略。对于这种情况很难判断，因为函数名称DoMethod没有意义。它们是动词，这意味着当它们无法“执行方法”时应该抛出异常，但我不知道它们是否应该这样做... - Merlyn Morgan-Graham

我同意这段代码可能是错误的，但试图阅读没有上下文的名为Foo的方法调用名为DoMethod1的函数的意图并修复设计，有点像在黑暗中射击。 - mqp

Answer 2

我认为最佳实践应该是：

private static void Foo()
{
    DoMehod1();
    DoMehod2();
    DoMehod3();
}

// ...

try
{
    Foo();
}
catch(Exception e)
{
    Console.WriteLine(e.ToString());
    throw;
}

如果您无法遵守具有基于动词命名约定的 DoMethod() 所暗示的承诺，就抛出异常。

如果您绝对不想抛出异常，请使用 mquander 建议的方法，将您的方法重命名为 TryDoMethod1 等。

如果您需要确保所有方法都运行（在原始代码中没有这样做），可以尝试以下方法之一：

• 构建一个要调用的方法列表（作为委托），并循环调用这些方法，内置 try/catch，并使用 使用 AggregateException 组合所有子异常以进行最终 throw。
• 使用 任务并行库 进行类似操作。

- Merlyn Morgan-Graham

---

我想这可能可行，但我通常不喜欢使用异常来控制程序流程。特别是如果在响应故障发生的另一个方法正在执行的情况下。 - Joshua

@Joshua：原始设计滥用控制流来实现错误处理。如果他们正确地使用异常处理，就不需要控制流了。虽然这个例子完全是假设的，但很难通过它们的名称判断这些方法是否应该抛出异常，或者是否应该预期它们会默默地/合作地失败。我认为他们的签名是错误的，因为我很少以那种方式编码。所以我选择了我喜欢的那一面 :) - Merlyn Morgan-Graham

Answer 3

阅读这篇经典文章“箭头代码”http://www.codinghorror.com/blog/2006/01/flattening-arrow-code.html，了解如何通过尽早返回错误来“平坦化”它。

可以在“代码大全”中找到关于“箭头代码”的章节：http://c2.com/cgi/wiki?ArrowAntiPattern @mquander的答案是理想的。

重点是摆脱单一出口的习惯，这在需要管理内存的代码中是必需的，因为您需要一个单一的地方来释放它（类似于您可以使用finally块）。由于.NET管理您的内存，您可以尽早离开方法而不必担心内存泄漏并使方法更易读。

- StuperUser

---

我认为应该使用异常，除非有强烈的理由通过代码返回错误（例如Try方法），因此我不认为mquander的解决方案一定是理想的。但是，原始代码相当难以理解，所以很难判断。 - Merlyn Morgan-Graham

Answer 4

这样做有什么问题：

return (DoMethod1() && DoMethod2() && DoMethod3())

...在调用代码中直接使用，完全不需要Foo方法

- SpaceBison

---

很可能 Console.WriteLines 表达了一些副作用，这些副作用需要根据 DoMethod1、DoMethod2 或 DoMethod3 的失败情况进行不同的处理，这在像这样的一行代码中很难表达清楚。 - mqp

是的，我想那是一个公正的评论。它也会始终评估所有3个 - DoMethod3可能会隐藏许多你不想每次运行的工作，但我想我会提出一个极简主义的建议 :) - SpaceBison

实际上不会。在 C# 中，&& 是短路运算符，所以如果 DoMethod1 失败，DoMethod2 和 DoMethod3 将永远不会运行。 - mqp

Answer 5

我认为第一目标应该是易于理解的代码。如果不知道底层功能并且没有适当命名，很难说什么是最佳代码结构。

在我看来，没有一种通用的最佳代码结构，这完全取决于您试图通过代码讲述的故事。

- Dylan Smith

Answer 6

你可以在这里尝试事务模式。

- oleksii