如何简化这组 if 语句？（或者，是什么让它感觉如此尴尬？）答案

【问题标题】：How can I simplify this set of if statements? (Or, what's making it feel so awkward?)如何简化这组 if 语句？（或者，是什么让它感觉如此尴尬？）
【发布时间】：2026-02-02 15:35:01
【问题描述】：

我的同事向我展示了这段代码，我们都想知道为什么我们似乎无法删除重复的代码。

private List<Foo> parseResponse(Response<ByteString> response) {
    if (response.status().code() != Status.OK.code() || !response.payload().isPresent()) {
      if (response.status().code() != Status.NOT_FOUND.code() || !response.payload().isPresent()) {
        LOG.error("Cannot fetch recently played, got status code {}", response.status());
      }
      return Lists.newArrayList();
    }
    // ...
    // ...
    // ...
    doSomeLogic();
    // ...
    // ...
    // ...
    return someOtherList;
}

这是另一种表示形式，以使其不那么冗长：

private void f() {
    if (S != 200 || !P) {
        if (S != 404 || !P) {
            Log();
        }
        return;
    }
    // ...
    // ...
    // ...
    doSomeLogic();
    // ...
    // ...
    // ...
    return;
}

有没有更简单的方法来写这个，而不重复!P？如果不是，是否有一些关于情况或条件的独特属性使得无法分解!P？

【问题讨论】：

我不这么认为，纯粹是因为您要返回然后登录 sub if 语句。
虽然它不一定会使它更简单，但您可以在其他操作之前检查 !P，这将使代码在 P 不存在的情况下更有效，因为其他操作不会需要检查。
如果您不想在S 不是 200 而是 404 时登录，这似乎是一种更短的方法。如果您希望两者都执行相同的操作，您会执行(S != 200 && S != 404) || !P，但事实并非如此
我完全希望分支不存在有效负载来记录不同的错误消息。
啊，让我澄清一下。我关心的不是代码长度。直觉上，感觉就像我们错过了什么，看到 !P 在两个地方重复。

标签： java if-statement logic refactoring boolean-logic

【解决方案1】：

它看起来像很多代码的一个原因是它非常重复。使用变量来存储重复的部分，这将有助于提高可读性：

private List<Foo> parseResponse(Response<ByteString> response) {
    Status status = response.status();
    int code = status.code();
    boolean payloadAbsent = !response.payload().isPresent();

    if (code != Status.OK.code() || payloadAbsent) {
      if (code != Status.NOT_FOUND.code() || payloadAbsent) {
        LOG.error("Cannot fetch recently played, got status code {}", status);
      }
      return Lists.newArrayList();
    }
    // ...
    // ...
    // ...
    return someOtherList;
}

编辑： 正如斯图尔特在下面的 cmets 中指出的那样，如果可以直接比较 response.status() 和 Status.OK，那么您可以消除对 .code() 的无关调用并使用 @987654325 @ 直接访问状态：

import static Namespace.Namespace.Status;

// ...

private List<Foo> parseResponse(Response<ByteString> response) {
    Status status = response.status();
    boolean payloadAbsent = !response.payload().isPresent();

    if (status != OK || payloadAbsent) {
      if (status!= NOT_FOUND || payloadAbsent) {
        LOG.error("Cannot fetch recently played, got status code {}", status);
      }
      return Lists.newArrayList();
    }
    // ...
    // ...
    // ...
    return someOtherList;
}

关于如何处理重复的payloadAbsent 逻辑的问题，Zachary 提供了一些与我的建议兼容的好主意。另一种选择是保留基本结构，但使检查有效负载的原因更加明确。这将使逻辑更易于遵循，并使您不必在内部if 中使用||。 OTOH，我自己对这种方法不是很热衷：

import static Namespace.Namespace.Status;

// ...

private List<Foo> parseResponse(Response<ByteString> response) {
    Status status = response.status();
    boolean failedRequest = status != OK;
    boolean loggableError = failedRequest && status!= NOT_FOUND ||
        !response.payload().isPresent();

    if (loggableError) {
      LOG.error("Cannot fetch recently played, got status code {}", status);
    }
    if (failedRequest || loggableError) {
      return Lists.newArrayList();
    }
    // ...
    // ...
    // ...
    return someOtherList;
}

【讨论】：

这似乎并没有解决我认为的主要问题：函数的逻辑结构以及 !P 重复的事实。
如果Status 是enum，那么您可以进一步将code != Status.OK.code() 缩短为status != Status.OK，并通过使用import static，将其进一步缩短为status != OK
@PeterBingham 是的，因为我没有看到避免使用两次!P 的好方法（或使用P 和!P，我不认为这是一种改进）。取消嵌套 if 语句将使逻辑更容易理解，但 Zachary 已经提出了这一点。我的回答来自 OP：“我们都想知道为什么代码看起来太多了”。它是很多代码，因为它是一堆超出 SO 代码窗口末尾的方法访问。我的回答解决了这个问题。
日志调用正常完成，logableError已经检查了failedRequest，可以吊出。

【解决方案2】：

如果您想澄清条件检查并保持相同的逻辑结果，以下可能是合适的。

if (!P) {
    Log();
    return A;
}
if (S != 200) {
    if (S != 404) {
        Log();
    }
    return A;
}
return B;

或者（这是 OP 的首选）

if (S == 404 && P) {
    return A;
}
if (S != 200 || !P) {
    Log();
    return A;
}
return B;

或者（我个人更喜欢这个，如果你不介意切换的话）

if (P) {
    switch (S) {
        case 200: return B;
        case 404: return A;
    }
}
Log ();
return A;

您可以通过删除大括号并将单行主体移动到与 if 语句相同的行来压缩 if 语句。然而，单行 if 语句可能会令人困惑，而且是完全不好的做法。根据我从 cmets 收集到的信息，您的偏好将反对这种使用。虽然单行 if 语句可以压缩逻辑并使代码看起来更清晰，但应该重视“经济”代码的清晰度和代码意图。需要说明的是：我个人认为在某些情况下单行 if 语句是合适的，但是，由于原始条件很长，我强烈建议不要在这种情况下这样做。

if (S != 200 || !P) {
    if (S != 404 || !P) Log();
    return A;
}
return B;

作为侧节点：如果 Log(); 语句是嵌套 if 语句中唯一可到达的分支，您可以使用以下逻辑标识来压缩逻辑 (Distributive)。

(S != 200 || !P) && (S! = 404 || !P) <=> (S != 200 && S != 404) || !P

编辑重要的编辑，以重新排列内容并解决 cmets 中提到的问题。

【讨论】：

所以，我的意思并不是按字面意思缩短代码。在我工作过（或知道）的大多数地方，单行 if 被禁止，并且您的操作员仅混音只能在代码高尔夫设置中接受（：但是您的最后一个建议还不错！它分开事件的整体后果并重复 return 而不是 !P。我喜欢它。
我倾向于不使用单行，除非条件和行都简洁（即使那样我也很想使用单行大括号）。最后的建议只有在 return 语句是第二个 if 语句之后的唯一代码时才是好的（因为复制代码会变得乏味 - 在这种情况下，您总是可以将其转移到单独的方法）。
@Chris 谢谢！很生气我错过了第一个，虽然已经很晚了。
由于正在添加更多版本，为了让未来的读者清楚，我更喜欢以if (S == 404 && P) return A;开头的版本。
否决了您的第一个示例是极度混淆的代码。遵循嵌套的 if 语句而不用括号括起来已经足够困难了，但不是其他的。我认为绝大多数程序员，包括我自己在内，都会将return A 视为它之前的if 语句的主体，而没有真正检查它并注意到Log();。请记住：代码的阅读频率高于编写频率！垂直空间不贵；使用那些括号！

【解决方案3】：

请记住，简洁并不总是最好的解决方案，在大多数情况下，编译器会优化局部命名变量。

我可能会使用：

    boolean goodPayload = response.status().code() == Status.OK.code() && response.payload().isPresent();
    if (!goodPayload) {
        // Log it if payload not found error or no payload at all.
        boolean logIt = response.status().code() == Status.NOT_FOUND.code()
                || !response.payload().isPresent();
        if (logIt) {
            LOG.error("Cannot fetch recently played, got status code {}", response.status());
        }
        // Use empty.
        return Lists.newArrayList();
    }

【讨论】：

看起来您通过添加有意义的变量名称使代码更具自我描述性。我喜欢你如何将hasPayload 修改为goodPayload，就像我正要评论它一样，哈哈。好的，同意这是一种明确的方法。但是，我更想了解需要重复的基本逻辑。
我想提倡“规则”，其中布尔变量名称以“is”、“has”、“should”或任何其他导致问题答案为“是”的动词开头与否（或真或假）。
@klaar - 我更喜欢指南，您应该使用使人类对代码含义/目的的理解尽可能清晰的名称 .
是的，当然，但是因为布尔变量表达的是或否的答案，所以变量不应该像人类提出问题的方式一样命名吗？我说是！我喜欢你试图把我描绘成一个原教旨主义者的活力，但我非常是一个实用主义者。当然，一切都很好。 ;-)

【解决方案4】：

代码味道对我来说是你询问 Response 对象，而不是告诉它。问问自己为什么 Parse 方法是 Response 对象的外部而不是它的方法（或者更可能是它的超类）。是否可以在 Response 对象构造函数中调用 Log() 方法而不是其 Parse 方法？在构造函数中计算属性 status().code() 和 payload().isPresent() 时，是否可以将 默认解析对象 分配给私有属性，这样只有一个简单（和单个）@987654323 @ 仍然在 Parse() 中？

当一个人有幸能够使用具有实现继承的面向对象语言进行编写时，应该查询每个条件语句（和表达式！），以查看它是否可以提升到构造函数或调用构造函数的方法。您的所有班级设计都可以遵循的简化是非常巨大的。

【讨论】：

是的，我也在想也许应该创建一个新类，将响应作为构造函数参数，因此可以包含验证响应的所有逻辑。
@KerriBrown：很有可能——但很难确定只看这个单一方法的代码。
无论如何，为“告诉不要问”+1
@KerriBrown：是的 - 这是 O-O 和函数式编程中众多最佳实践的核心。
如果您提供一个代码示例来说明您所描述的内容而不仅仅是对它的描述，将会很有帮助。

【解决方案5】：

实际上多余的主要是 !P （存在！有效负载）。如果你把它写成一个布尔表达式，你有：

(A || !P) && (B || !P)

正如您所观察到的，!P 似乎被重复了，而且是不必要的。在布尔代数中，您可以处理类似于乘法的 AND 和类似于加法的 OR。因此，您可以像使用简单代数一样将这些括号展开为：

A && B || A && !P || !P && B || !P && !P

您可以将所有具有 !P 的 AND 表达式组合在一起：

A && B || (A && !P || !P && B || !P && !P)

由于所有这些术语都包含 !P，因此您可以像乘法一样将其取出。当你这样做时，你将它替换为 true （就像你会 1，因为 1 次任何事物都是它自己，true AND 任何东西都是它自己）：

A && B || !P && (A && true || B && true || true && true)

请注意，“true && true”是 OR 表达式之一，因此整个分组始终为真，您可以简化为：

A && B || !P && true
-> A && B || !P

我可能对这里的正确符号和术语感到生疏。但这就是它的要点。

回到代码，如果您在 if 语句中有一些复杂的表达式，正如其他人所指出的那样，您应该将它们放在一个有意义的变量中，即使您只是打算使用一次并丢弃它。

所以把它们放在一起你会得到：

boolean statusIsGood = response.status().code() != Status.OK.code() 
  && response.status().code() != Status.NOT_FOUND.code();

if (!statusIsGood || !response.payload().isPresent()) {
  log();
}

请注意，变量名为“statusIsGood”，即使您将其取反。因为带有否定名称的变量真的很混乱。

请记住，您可以对非常复杂的逻辑进行上述简化，但您不应该总是这样做。你最终会得到技术上正确但没有人能通过观察来判断原因的表达式。

在这种情况下，我认为简化说明了意图。

【讨论】：

好观察！我总是从尝试通过使用逻辑规则来简化开始。你是对的，有时它确实让人更难理解，所以它并不总是有用的。在这种情况下，虽然获得相同转换的更简单方法是将常用术语 !P 拉到括号外：(A || !P) && (B || !P) <==> (A && B) || !P。
是的，我可能试图对这个答案过于笼统。它甚至适用于非常复杂的逻辑表达式。但你是对的，在这种情况下你基本上可以看到它，而简化的关键就是给 A、B 和 P 赋予有意义的名称并使用它们。

【解决方案6】：

恕我直言，问题主要是重复和嵌套。其他人建议使用我也推荐的 clear 变量和 util 函数，但您也可以尝试分离验证的关注点。

如果我错了，请纠正我，但您的代码似乎在实际处理响应之前尝试验证，所以这是编写验证的另一种方法：

private List<Foo> parseResponse(Response<ByteString> response)
{
    if (!response.payload.isPresent()) {
        LOG.error("Response payload not present");
        return Lists.newArrayList();
    }
    Status status = response.status();
    if (status != Status.OK || status != Status.NOT_FOUND) {
        LOG.error("Cannot fetch recently played, got status code {}", status);
        return Lists.newArrayList();
    }
    // ...
    // ...
    // ...
    return someOtherList;
}

【讨论】：

【解决方案7】：

您可以反转 if 语句以使其更清晰，如下所示：

private void f() {
    if (S == 200 && P) {
        return;
    }

    if (S != 404 || !P) {
        Log();
    }

    return;
}

然后您可以使用有意义的方法名称重构条件，例如“responseIsValid()”和“responseIsInvalid()”。

【讨论】：

【解决方案8】：

辅助函数可以简化嵌套条件。

private List<Foo> parseResponse(Response<ByteString> response) {
    if (!isGoodResponse(response)) {
        return handleBadResponse(response);
    }
    // ...
    // ...
    // ...
    return someOtherList;
}

【讨论】：

【解决方案9】：

最尴尬的部分是像response.status() 这样的getter 在逻辑似乎需要一个单一的、一致的值时被多次调用。推测它可以工作，因为 getter 被保证总是返回相同的值，但它错误地表达了代码的意图，使其更容易受到当前假设的影响。

要解决这个问题，代码应该得到一次response.status()，

var responseStatus = response.status();

，然后只需使用responseStatus。应该对其他 getter 值重复此操作，这些值在每次获取时都假定为相同的值。

此外，如果此代码稍后可能在更动态的上下文中重构为线程安全的实现，那么所有这些获取理想情况下都将在同一顺序点完成。要点是您的意思是在某个特定时间点获取 response 的值，因此代码的关键部分应该在一个同步过程中获取这些值。

通常，正确指定预期的数据流会使代码更具弹性和可维护性。然后，如果有人需要为 getter 添加副作用或使 response 成为抽象数据类型，则更有可能继续按预期工作。

【讨论】：

【解决方案10】：

免责声明：我不会质疑所提供函数的签名，也不会质疑功能。

对我来说，这感觉很尴尬，因为这个函数自己做了很多工作，而不是委托它。

在这种情况下，我建议将 validation 部分去掉：

// Returns empty if valid, or a List if invalid.
private Optional<List<Foo>> validateResponse(Response<ByteString> response) {
    var status = response.status();
    if (status.code() != Status.NOT_FOUND.code() || !response.payload().isPresent()) {
        LOG.error("Cannot fetch recently played, got status code {}", status);
        return Optional.of(Lists.newArrayList());
    }

    if (status.code() != Status.OK.code()) {
        return Optional.of(Lists.newArrayList());
    }

    return Optional.empty();
}

请注意，我更喜欢重复 return 语句而不是嵌套条件。这使代码平坦，降低了圈复杂度。另外，不能保证您总是希望为所有错误代码返回相同的结果。

之后，parseResponse 变得简单：

private List<Foo> parseResponse(Response<ByteString> response) {
    var error = validateResponse(response);
    if (error.isPresent()) {
        return error.get();
    }

    // ...
    // ...
    // ...
    return someOtherList;
}

您可以改为使用函数式样式。

/// Returns an instance of ... if valid.
private Optional<...> isValid(Response<ByteString> response) {
    var status = response.status();
    if (status.code() != Status.NOT_FOUND.code() || !response.payload().isPresent()) {
        LOG.error("Cannot fetch recently played, got status code {}", status);
        return Optional.empty();
    }

    if (status.code() != Status.OK.code()) {
        return Optional.empty();
    }

    return Optional.of(...);
}

private List<Foo> parseResponse(Response<ByteString> response) {
    return isValid(response)
        .map((...) -> {
            // ...
            // ...
            // ...
            return someOtherList;
        })
        .orElse(Lists.newArrayList());
}

虽然我个人觉得额外的嵌套有点烦人。

【讨论】：

【解决方案11】：

我认为以下是等价的。但正如其他人所指出的，代码透明度可能比“简单”代码更重要。

if (not ({200,404}.contains(S) && P)){
    log();
    return;
}
if (S !=200){
    return;
}
// other stuff

【讨论】：

【解决方案12】：

您可以拥有一组要记录的代码，并且有效负载的常见情况不存在

Set<Code> codes = {200, 404};
if(!codes.contains(S) && !P){
    log();
}
return new ArrayList<>();

状态修正。

【讨论】：

啊，对不起。这是我的错。在if 之后会发生更多的事情还不是很清楚。让我修改一下我的问题。（我不是反对你的人。）
哦。我假设你想减少条件。一种解决方案可能是提取条件并提供有意义的名称。但是，如果您在每个块中执行代码行，那么您可以根据优先级遵循条件的优先级。

【解决方案13】：

通过将整数与一组有限的显式值进行比较来对整数进行分支最好由switch 处理：

if (P) {
    switch (S) {
        case 200: return B;
        case 404: return A;
    }
}

Log();
return A;

【讨论】：

此代码记录每个案例，与原始代码不同。但是，它确实提供了使用 switch 的直通机制来避免重复代码的想法。
@AndrewCheong 没有失败，因为return。
不，我的意思是你的代码不像我的原始代码那样做。但是有办法使用switch，这样就可以了。

【解决方案14】：

只需使用像 JLRishe 的答案这样的变量。但我认为代码清晰比不重复简单的布尔检查更重要。您可以使用提前返回语句使这一点更清晰：

private List<Foo> parseResponse(Response<ByteString> response) {

    if (response.status().code() == Status.NOT_FOUND.code() && !response.payload().isPresent()) // valid state, return empty list
        return Lists.newArrayList();

    if (response.status().code() != Status.OK.code()) // status code says something went wrong
    {
        LOG.error("Cannot fetch recently played, got status code {}", response.status());
        return Lists.newArrayList();
    }

    if (!response.payload().isPresent()) // we have an OK status code, but no payload! should this even be possible?
    {
        LOG.error("Cannot fetch recently played, got status code {}, but payload is not present!", response.status());
        return Lists.newArrayList();
    }

    // ... got ok and payload! do stuff!

    return someOtherList;
}

【讨论】：

【解决方案15】：

可以排除重复的 P 检验。以下（伪代码）在逻辑上等同于您问题中的代码。

private List<Foo> f() {
    List<Foo> list(); /*whatever construction*/
    if (P) {
        if (S==200) {
            // ...
            // ...
            // ...
            list.update(/*whatever*/);
        }
        else if (S!=404) {
           Log();
        }
    }
    else {
       Log();
    }
    return list;
}

就可读性而言，我会使用以下内容（再次是伪代码）：

private bool write_log() {
    return (S!=200 && S!=404) || !P
}
private bool is_good_response() {
    return S==200 && P
}
private List<Foo> f() {
    List<Foo> list(); /*whatever construction*/
    if (write_log()) {
       Log();
    }
    if (is_good_response()) {
        // ...
        // ...
        // ...
        list.update(/*whatever*/);
    }
    return list;
}

也许有更恰当命名的函数。

【讨论】：

【解决方案16】：

在这组条件下，我认为没有办法解决一些重复问题。但是，我更愿意将我的条件尽可能合理地分开，并在必要时复制其他区域。

如果我写这个，保持当前的风格，它会是这样的：

    private void f() {
        if(!P) {
            Log();          // duplicating Log() & return but keeping conditions separate
            return;
        } else if (S != 200) {
            if (S != 404) {
                Log();
            }
            return;
        }

        // ...
        // ...
        // ...
        return;
    }

代码的简单性有一些主观因素，可读性是非常主观的。鉴于此，如果我要从头开始编写此方法，这就是我的偏见。

private static final String ERR_TAG = "Cannot fetch recently played, got status code {}";

private List<Foo> parseResponse(Response<ByteString> response) {
    List<Foo> list = Lists.newArrayList();

    // similar to @JLRishe keep local variables rather than fetch a duplicate value multiple times
    Status status = response.status();
    int statusCode = status.code();
    boolean hasPayload = response.payload().isPresent();

    if(!hasPayload) {
        // If we have a major error that stomps on the rest of the party no matter
        //      anything else, take care of it 1st.
        LOG.error(ERR_TAG, status);
    } else if (statusCode == Status.OK.code()){
        // Now, let's celebrate our successes early.
        // Especially in this case where success is narrowly defined (1 status code)
        // ...
        // ...
        // ...
        list = someOtherList;
    } else {
        // Now we're just left with the normal, everyday failures.
        // Log them if we can
        if(statusCode != Status.NOT_FOUND.code()) {
            LOG.error(ERR_TAG, status);
        }
    }
    return list;        // One of my biases is trying to keep 1 return statement
                        // It was fairly easy here.
                        // I won't jump through too many hoops to do it though.
}

如果我删除我的 cmets，这仍然会使代码行数几乎翻倍。有些人会争辩说，这不能使代码更简单。对我来说，确实如此。

【讨论】：

【解决方案17】：

我不确定代码试图做什么：老实说，只记录 404 状态并在代码不是 200 时返回一个空列表感觉就像您试图避免 NPE...

我认为这样更好：

private boolean isResponseValid(Response<ByteString> response){
    if(response == null){
        LOG.error("Invalid reponse!");
        return false;
    }

    if(response.status().code() != Status.OK.code()){
        LOG.error("Invalid status: {}", response.status());
        return false;
    }

    if(!response.payload().isPresent()){
        LOG.error("No payload found for response!");
        return false;
    }
    return true;
}

private List<Foo> parseResponse(Response<ByteString> response) throws InvalidResponseException{
    if(!isResponseValid(response)){
        throw InvalidResponseException("Response is not OK!");
    }

    // logic
}

如果由于任何原因无法更改 if 逻辑，我无论如何都会将验证移到单独的函数中。

另外，尝试使用 Java 命名约定：

LOG.error("")    // should be log.error("")
Status.OK.code() // why couldn't it be a constant like Status.OK, or Status.getOkCode()?

【讨论】：