如何简化这组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;
}

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

作为辅助节点:如果Log();语句是嵌套if语句中唯一可到达的分支,则可以使用以下逻辑标识来压缩逻辑(Distributive).

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

编辑重要编辑以重新排列内容并解决评论中提到的问题.

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

Answer 4

该代码味道对我来说,你是问 Response对象,而不是告诉它.问问自己为什么Parse方法在Response对象的外部而不是它的方法(或者更可能是它的超类).是否可以在Response对象构造函数中调用Log()方法而不是其Parse方法？在属性status().code()和payload().isPresent()在构造函数中计算的那一刻,是否可以将默认解析对象分配给私有属性,使得只有一个简单(和单个)if ... else ...保留在Parse()中？

如果有人能够使用实现继承来编写面向对象的语言,那么应该查询每个条件语句(和表达式!),以查看它是否被提升到构造函数或方法中(s) )调用构造函数.所有课堂设计的简化都是非常重要的.

Answer 5

实际上多余的主要是!P(有效载荷存在).如果你把它写成一个布尔表达式,你有:

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

正如你所观察到的那样,!P似乎在重复,而且它是不必要的.在布尔代数中,您可以将AND处理为类似乘法,而OR类似于加法.因此,您可以像使用简单代数一样将这些括号扩展为:

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

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

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

由于所有这些术语都有!P,你可以像乘法一样把它拿出来.当你这样做时,你将它替换为true(就像你将1一样,因为任何东西本身都是1次,真实且任何东西本身):

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

请注意,"true && true"是OR表达式之一,因此整个分组始终为true,您可以简化为:

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".因为带有否定名称的变量确实令人困惑.

请记住,您可以对真正复杂的逻辑进行上述简化,但您不应该总是这样做.你最终会得到技术上正确的表达,但没有人能通过观察它来说明原因.

在这种情况下,我认为简化澄清了意图.

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