2

我在别人的代码中遇到了以下表达式。我认为这是一个糟糕的代码，原因有很多（尤其是因为它没有考虑到 bool.TrueString 和 bool.FalseString），但我很好奇编译器将如何评估它。

private bool GetBoolValue(string value)
{
    return value != null ? value.ToUpper() == "ON" ? true : false : false;
}

编辑 顺便说一句，表达式不是从内向外评估的吗？在这种情况下，在调用 value.ToUpper() 之后检查 value != null 的意义何在，这将引发空引用异常？

我认为以下是正确的（故意）冗长的版本（我永远不会这样 :D ）：

if (value != null)
{
    if (value.ToUpper() == "ON") 
    {
        return true;
    }
    else        // this else is actually pointless
    {
        return false;
    }
}
else
{
    return false;
}

可以缩短为：

return value != null && value.ToUpper == "ON";

这是对表达式的正确重写吗？

4

7 回答 7

4

看起来该方法旨在处理来自checkboxHTML 元素的值。如果未为复选框指定值，则"on"默认使用该值。如果未选中该复选框，则表单数据中根本没有任何值，因此从中读取键Request.Form会给出空引用。

在这种情况下，该方法是正确的，尽管由于使用了if-condition-then-true-else-false反模式，它非常可怕。此外，它应该被赋予一个更适合其特定用途的名称，例如GetCheckboxValue.

您对方法的重写是正确且合理的。由于该值不依赖于文化，因此将值转换为大写不应使用当前文化。因此，比您建议的更好的重写将是：

return value != null && value.ToUpperInvariant == "ON";

（文化独立的方法也比使用特定文化的方法快一点，所以没有理由不使用它们。）

顺便说一句，表达式不是从内向外计算的吗？

如果是方法调用，以便实际评估所有表达式，它们会，因为必须进行内部调用来评估外部调用的参数。

但是，条件表达式的第二个和第三个操作数只有在使用时才会被计算，因此表达式是从外部和内部计算的。首先评估最外面的条件以确定它将评估哪些操作数。

于 2009-04-20T14:17:38.017 回答
3

你是正确的，在你的重写和你断言这种简洁的尝试是不好的，因为它会导致混乱。

于 2009-04-20T13:52:27.470 回答
2

那么第一个是双嵌套的三元运算符

return (value != null) ? [[[value.ToUpper() == "ON" ? true : false]]] : false;

[[[ ]]] 中的位是三元表达式的第一个结果，当第一个条件为真时评估它，因此您正在阅读/断言它是正确的

但它的丑陋如地狱，在当前状态下非常不可读/不可维护。我肯定会把它改成你最后的建议

边注：

做的人

if(X == true) 
   return true;
else
   return false;

代替

 return X;

应该取出并射击;-)

于 2009-04-20T13:57:11.313 回答
1

您是否正在寻找速度或可读性和组织？执行速度，您缩短的示例可能是最好的方法。

多花几毫秒，您可以将此实用程序方法重写为扩展方法，如下所示：

public static bool ToBoolean(this string value)
{

    // Exit now if no value is set
    if (string.IsNullOrEmpty(value)) return false;

    switch (value.ToUpperInvariant())
    {
        case "ON":
        case "TRUE":
            return true;
    }

    return false;

}

...然后您将按如下方式使用它：

public static void TestMethod()
{
    bool s = "Test".ToBoolean();
}

编辑：实际上，我错了......快速性能测试表明扩展方法比内联方法更快。我的测试来源如下，以及我电脑上的输出。

[Test]
public void Perf()
{

    var testValues = new string[] {"true", "On", "test", "FaLsE", "Bogus", ""};
    var rdm = new Random();
    int RunCount = 100000;
    bool b;
    string s;

    Stopwatch sw = Stopwatch.StartNew();
    for (var i=0; i<RunCount; i++)
    {
        s = testValues[rdm.Next(0, testValues.Length - 1)];
        b = s.ToBoolean();
    }
    Console.Out.WriteLine("Method 1: {0}ms", sw.ElapsedMilliseconds);

    sw = Stopwatch.StartNew();
    for (var i = 0; i < RunCount; i++)
    {
        s = testValues[rdm.Next(0, testValues.Length - 1)];
        b = s != null ? s.ToUpperInvariant() == "ON" ? true : s.ToUpperInvariant() == "TRUE" ? true : false : false;
    }
    Console.Out.WriteLine("Method 2: {0}ms", sw.ElapsedMilliseconds);


}

输出：

Method 1: 21ms
Method 2: 30ms
于 2009-04-20T14:10:21.657 回答
0

我以与您相同的方式阅读原始表达。所以我认为你缩短的表达是正确的。如果 value 为 null 它永远不会到达第二个条件，所以对我来说它看起来很安全。

于 2009-04-20T13:55:14.000 回答
0

我也讨厌这样的结构：

if (value.ToUpper() == "ON") 
{
    return true;
}
else        // this else is actually pointless
{
    return false;
}

正如您所注意到的，这是一种冗长而复杂（不是说愚蠢）的写作方式：

return value.ToUpper() == "ON";

你的提议很好，简短而正确。

于 2009-04-20T13:58:16.817 回答
0

另一种选择：

return string.Equals( value, "ON", StringComparison.OrdinalIgnoreCase );
于 2009-04-20T14:24:06.270 回答