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

Question

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

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？

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

编辑: 正如斯图尔特在下面的评论中指出的那样，如果可以直接比较 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 逻辑的问题，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;
}