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

if-statement boolean-logic java refactoring logic

2022-09-01 10:06:29

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

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

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

编辑：正如Stewart在下面的评论中指出的那样，如果可以直接比较，那么您可以消除对状态的无关调用，并直接用于访问状态：response.status()Status.OK.code()import static

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

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

答案 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 语句可能会令人困惑且不合时宜的不良做法。从我从评论中收集到的信息来看，您的偏好是反对这种使用。虽然单行 if 语句可以压缩逻辑并给出更清晰的代码外观，但清晰度和代码意图应该被视为“经济”代码。需要明确的是：我个人认为在某些情况下，单行if语句是合适的，但是，由于原始条件很长，因此在这种情况下，我强烈建议不要这样做。

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

作为端节点：如果语句是嵌套的 if 语句中唯一可访问的分支，则可以使用以下逻辑标识来压缩逻辑（Distributive）。Log();

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

编辑进行重大编辑以重新排列内容并解决评论中提到的问题。