什么都不做的 If 分支是代码气味或良好实践吗？答案

【问题标题】：Is an If branch that does nothing a code smell or good practice?什么都不做的 If 分支是代码气味或良好实践吗？
【发布时间】：2011-04-26 03:50:54
【问题描述】：

我在这里（或至少commented）回复了包含此类代码的答案，但我想知道用一个（或多个）编写一系列if 分支是好是坏) 的分支在其中什么都不做，通常是为了消除在每个分支中检查 null。

一个例子（C#代码）：

if (str == null) { /* Do nothing */ }
else if (str == "SomeSpecialValue")
{
    // ...
}
else if (str.Length > 1)
{
    // ...
}

代替：

if (str != null && str == "SomeSpecialValue")
{
    // ...
}
else if (str != null && str.Length > 1)
{
    // ...
}

当然，这只是一个示例，因为我倾向于将这些用于更大、更复杂的类。在大多数情况下，null 值表示什么都不做。

对我来说，这减少了我的代码的复杂性，并且在我看到它时很有意义。那么，这种形式是好是坏（甚至是代码味道）？

【问题讨论】：

我认为如果它使您的代码更具可读性并避免令人费解的条件就可以了。不过，投票结束，“这是好是坏”太主观了。
所有这些检查都需要“以防万一”值为空的事实本身就是一种气味，恕我直言。这样做是在隐藏错误。
@SimonJ：好点。至少在这种情况下，如果不期望 null 值，他应该抛出异常。
我会把 '{ /* do nothing--dodge 'else-ifs' */ }' 放在它自己的行上，而不是放在第一个 'if' 的行上。
这是错误的形式。编码时最好的经验法则之一是“做某事，再也不做”。这适用于大规模（通过将通用功能重构为单个包或类）和小规模（通过仅检查一次条件值）。您唯一可以摆脱这种情况的是在 XOR 场景中（即，“if (x == null && y == null) {...} else if (x != null && y != null) {. ..}") 逻辑会强制您多次进行类似的检查。

标签： conditional complexity-theory

【解决方案1】：

我更喜欢这样——

if (str != null) 
{
 if (str == "[NULL]")
 {
    // ...
 }
 else if (str.Length > 1)
 {
    // ...
 }
}

我认为您始终可以将带有空主体的if“改写”为带有主体的否定，这样看起来更好，更有意义。

【讨论】：

我想我讨厌太多的缩进。它还为我节省了一点垂直空间，至少在 C# 中，我确信编译器会很好地处理它。
编译器会很好地处理它，但恕我直言，你所说的逻辑很尴尬。为什么要说“如果……那么什么都不做”，而您可以说“如果不……那就做……”关于标识，您可以减少制表符占用的空格数，这至少会有所帮助。
不想正确格式化代码并不是反对正确编写代码的好理由。
@palswim Anthony Morelli 是对的。缩进是为了人类读者而不是编译器的利益。
+1 做显而易见、合乎逻辑和惯用的事情最有可能产生明显、合乎逻辑和惯用的代码。

【解决方案2】：

我通常会将return 或类似的东西放在第一个if 中：

void Foo()
{
  if (str == null) { return; }
  if (str == "SomeSpecialValue")
  {
      // ...
  }
  else if (str.Length > 1)
  {
      // ...
  }
}

如果你不能这样做，因为函数在if/else 之后做了其他事情，我想说是时候重构了，并将if/else 部分拆分为一个单独的函数，您可以从中提前返回。

【讨论】：

我不喜欢你的缩进风格，但是这段代码是解决 OP 问题的最好方法，恕我直言。 +1
@rmeador：有趣；你喜欢哪种缩进样式？
这不是真正的“我的”缩进风格，我只是试图坚持使用 OP。 :)

【解决方案3】：

避免以下情况确实很好，因为它不必要地重新检查其中一个条件（编译器将优化这一点的事实是不切实际的——它可能会做更多的工作对于试图阅读您的代码的人）：

if (str != null && str == "SomeSpecialValue")
{
    // ...
}
else if (str != null && str.Length > 1)
{
    // ...
}

但是按照你的建议去做也很奇怪，如下：

if (str == null) { /* Do nothing */ }
else if (str == "SomeSpecialValue")
{
    // ...
}
else if (str.Length > 1)
{
    // ...
}

我说这很奇怪，因为它混淆了你的意图并且违背了读者的期望。如果你检查一个条件，人们希望你在它满足的情况下做某事——但你没有。这是因为您的 intent 并不是要真正处理 null 条件，而是要在检查您实际的两个条件时避免空指针em> 感兴趣。实际上，不是有两个概念状态要处理，而是有一个健全的规定（非空输入），它读起来就像你有三个概念状态要处理。 在计算上，您可以说存在三种这样的状态这一事实是不切实际的——它不太清楚。

在这种情况下，通常的 case 方法是 Oren A 建议的——检查 null，然后检查结果块中的其他条件：

if (str != null) 
{
 if (str == "SomeSpecialValue")
 {
    // ...
 }
 else if (str.Length > 1)
 {
    // ...
 }
}

这只不过是提高可读性的风格问题，而不是代码异味的问题。

编辑：但是，如果您设置为无所事事的条件，我做非常喜欢您包含“无所事事”的评论。否则，人们可能会认为您只是忘记完成代码。

【讨论】：

查看编辑 - 我把那个（“[NULL]”字符串）去掉了，因为人们已经挂断了。
@palswim 好的。但请注意，这并不是我回答的重点。（但与此同时，该代码来自某个地方——如果它是生产代码，那几乎肯定是一件坏事）。
@user359996：不；我不想复制和粘贴我的生产代码。我试图在现场想一个例子。
我认为可以公平地说我的大脑确实有代码味道。每当我尝试在此站点上提供示例时，都会有人指出示例中的不良做法，通常是关于我选择的示例数据。

【解决方案4】：

在这种特殊情况下，我会提前返回，这样代码更容易阅读

if (string.IsNullOrEmpty(str)) { return; }

我喜欢明确的返回语句。

【讨论】：

只有在函数除了 if...else 块之外什么都不做的情况下才有效。
这真的取决于问题的语义。空参数很少表示“什么都不做”。很多时候，这意味着有人犯了错误，而默默地返回，只会加剧问题。如果不是生命支持或军队，那么快速失败通常是个好主意。
我完全同意你的观点，而且我总是很快失败。我只是回答这个特殊的问题，无论出于何种原因想要返回的使用。这通常发生在批处理中，而不是失败，您只是跳过并可能记录跳过的实体。

【解决方案5】：

是的这是一种代码味道。

一个迹象是你想问这个问题。

另一个迹象是代码看起来不完整——好像有些东西应该属于那里。它可能是可读的，但感觉不对。

在阅读该代码时，局外人必须停下来一秒钟，并使用脑力来确定该代码是否有效/完整/正确/符合预期/形容词。

user359996 一针见血：

我说这很奇怪，因为它混淆了你的意图并违背了读者的期望。

【讨论】：

我不认为“思考问题”一定是一个不好的指标。我看到的几乎所有代码，即使是“好代码”的经典示例，对我来说都很好。例如，几乎每个设计模式对我来说都像是一种代码味道。几乎每一个程序代码的例子都让我“停下来动动脑筋”来弄清楚它的意图。
@Ken 一般来说，“想问”可能不是代码异味的迹象，但在这种情况下，我说它是。如果没有代码对您来说看起来不错，那么您要么阅读了错误的源代码，要么出于某种原因不喜欢代码的外观。（很多）设计模式不是代码气味……可能是语言中的弱点暴露。对于上面的示例，与看起来不完整的简单代码相比，有更好的事情可以“停止”并消耗脑力。很抱歉，程序代码给您带来了麻烦。
instanceofTom：很抱歉，一对没有声明的大括号给您带来了麻烦。
@Ken，在这里插入诙谐的回应

【解决方案6】：

你的第一个例子对我来说是完全可读的——一点味道都没有。

【讨论】：

【解决方案7】：

这一切都取决于上下文。如果放一个空的 if 语句会使代码更具可读性，那就去吧。

【讨论】：

是的，但是当你这样做的时候一定要记录下来。或者，如果您像我一样尝试编写不需要 cmets 的代码，并且您没有使用一种强制所有内容都在一个类中的语言，请调用一个名为 Nothing() 的方法。
我习惯于使用 Python，其中包含“pass”（什么都不做的声明）。

【解决方案8】：

它是可读的，它是好是坏取决于你想要达到的目标 - 通常长嵌套的“goes-on-forever”类型if 语句是不好的。不要忘记框架中的静态字符串方法：string.IsNullOrEmpty() 和 string.IsNullOrWhiteSpace()。

您的if (str == null) { /* Do nothing */ } 行很不寻常，但确实有一个积极的意义：它让其他开发人员预先知道您在这种情况下故意不做任何事情，如果您使用长长的 if/else if 结构，您的意图可能会变得不清楚你把它改成

if (str != null) 
{ 
    /* carry on with the rest of the tests */
}

【讨论】：