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




java refactor if else (12)

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

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();
    }
    // ...
    // ...
    // ...
    return someOtherList;
}

这是一个替代表示,使它不那么罗嗦:

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

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


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

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


免责声明:我不会质疑所提供功能的签名,也不会质疑功能。

对我来说,这感觉很尴尬,因为这个功能本身就要做很多工作,而不是委托它。

在这种情况下,我建议吊起验证部分:

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

虽然我个人觉得额外的筑巢有点烦人。


可以将重复的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;
}

可能更适合命名的功能。


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

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(); statement是嵌套if语句中唯一可到达的分支,您可以使用以下逻辑标识来压缩逻辑( Distributive )。

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

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


实际上多余的主要是!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'd表达式之一,因此整个分组始终为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”。 因为带有否定名称的变量确实令人困惑。

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

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


恕我直言,问题主要是重复和嵌套。 其他人建议使用我推荐的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;
}

您可以拥有一组要记录的代码以及有效负载的常见条件

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

条件更正。


我不确定代码试图做什么:老实说,只记录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()?

最尴尬的部分是当逻辑看起来需要单个一致的值时,像response.status()这样的getter被多次调用。 据推测它是有效的,因为getter保证总是返回相同的值,但是它会严重地表达代码的意图并使其对当前的假设更加脆弱。

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

var responseStatus = response.status();

,然后再使用responseStatus 。 对于每次获取时假定为相同值的其他getter值,应重复此操作。

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

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


有了这些条件,我认为没有办法解决一些重复问题。 但是,我更愿意在必要时将条件与合理分开并复制其他区域。

如果我写这个,保持当前的风格,它将是这样的:

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

如果我删除我的评论,这仍然几乎使代码行加倍。 有些人认为这不能使代码更简单。 对我来说,确实如此。


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

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

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

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

Log();
return A;






boolean-logic