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

My colleague showed me this piece of code, and we both wondered why we couldn't seem to remove duplicated code.

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

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

Here's an alternate representation, to make it a little less wordy:

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

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

Is there a simpler way to write this, without duplicating the !P? If not, is there some unique property about the situation or conditions that makes it impossible to factor out the !P?

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

17 个解决方案

#1

One reason it looks like a lot of code is that it is very repetitive. Use variables to store the parts that are repeated, and that will help with the readability:

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

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

Edit: As Stewart points out in the comments below, if it's possible to compare response.status() and Status.OK directly, then you can eliminate the extraneous calls to .code() and use import static to access the statuses directly:

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

Regarding the question of what to do with the duplication of the payloadAbsent logic, Zachary has provided some good ideas that are compatible with what I have suggested. One more option is to keep the basic structure, but make the reason for checking the payload more explicit. This would make the logic easier to follow and save you from having to use || in the inner if. OTOH, I'm not very keen on this approach myself:

关于如何处理有效载荷重复逻辑的问题,Zachary提供了一些与我建议的内容兼容的好主意。还有一个选择是保留基本结构,但要更明确地检查有效负载。这将使逻辑更容易遵循,并使您不必使用||在内心如果。 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;
}

#2

If you wanted to clarify on the conditional checks and maintain the same logical outcome, the following may be appropriate.

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

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

Or (This was OP preferred)

或者(这是OP首选)

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

Or (I personally prefer this, if you don't mind the switch)

或者(我个人更喜欢这个,如果你不介意开关)

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

You could condense the if-statement by removing braces and moving the single-lined body to the same line as the if-statement. Single-line if-statements, however, can be confusing and out-right bad practice. From what I gather from the comments, your preference would be against this use. While single-line if-statements can condense logic and give the appearance of cleaner code, clarity and code intent should be valued of 'economic' code. To be clear: I personally feel there are situations where single-line if-statements are appropriate, however, as the original conditions are very long, I would strongly advise against this in this case.

您可以通过删除大括号并将单行主体移动到与if语句相同的行来压缩if语句。但是,单行if语句可能会让人感到困惑,也可能是不正确的错误做法。根据我从评论中收集到的内容,您的偏好将不利于此用途。虽然单行if语句可以压缩逻辑并使代码更清晰,但清晰度和代码意图应该被重视为“经济”代码。需要明确的是:我个人觉得有些情况下单行if语句是合适的,但是,由于原始条件很长,在这种情况下我强烈建议不要这样做。

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

As a side node: If the Log(); statement was the only reachable branch in the nested if-statements, you could use the following logical identity to condense the logic (Distributive).

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

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

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

EDIT Significant edit to rearrange content and resolve issues mentioned in comments.

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

#3

Please remember that succinctness is not always the best solution and in most cases local named variables will be optimised out by the compiler.

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

I would probably use:

我可能会用:

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

#4

The code smell to me is that you are asking of the Response object instead of telling it. Ask yourself why the Parse method is external to the Response object instead of being a method of it (or more likely, a super-class of it). Can the Log() method perhaps be called in the Response object constructor instead of its Parse method? At the moment when the properties status().code() and payload().isPresent() are computed in the constructor would it be possible to assign a default parsed object to a private property such that only a simple (and single) if ... else ... remains in Parse()?

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

When one is blessed with being able to write in an object-oriented language with implementation inheritance, every conditional statement (and expression!) should be queried, to see if it is a candidate to be lifted into either the constructor or the method(s) that invoke the constructor. The simplification that can follow for all your class designs is truly immense.

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

#5

The main thing that is actually redundant is the !P (!payload is present). If you wrote it as a boolean expression you have:

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

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

As you've observed, the !P seems to be repeated, and it is needlessly. In boolean algebra you can treat AND sort of like multiplication, and OR sort of like addition. So you can expand those parentheses just like with simple algebra into:

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

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

You can group all the ANDed expressions that have !P together:

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

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

Since all those terms have !P in them you can take it out like multiplication. When you do, you replace it with true (like you would 1, because 1 times anything is itself, true AND anything is itself):

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

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

Notice the "true && true" is one of the OR'd expressions, so that whole grouping is always true, and you can simplify to:

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

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

I'm rusty on the proper notation and terminology here, maybe. But that's the gist of it.

也许,我在这里用正确的符号和术语生锈了。但这就是它的要点。

Back to the code, if you have some complex expressions in an if statement, as others have noted you should stick them in a meaningful variable even if you're just going to use it once and throw it away.

回到代码,如果你在if语句中有一些复杂的表达式,就像其他人已经注意到的那样你应该把它们放在一个有意义的变量中,即使你只是要使用它一次并扔掉它。

So putting those together you get:

所以把这些放在一起你会得到:

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

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

Notice the variable is named "statusIsGood" even though you'll negate it. Because variables with negated names are really confusing.

请注意,即使您将其否定,该变量也会被命名为“statusIsGood”。因为带有否定名称的变量确实令人困惑。

Keep in mind, you can do the above kind of simplification for really complex logic, but you shouldn't always do it. You'll end up with expressions that are technically correct but nobody can tell why by looking at it.

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

In this case, I think the simplification clarifies the intent.

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

#6

IMHO the problem is mainly the repetition and if nesting. Others have suggested using clear variables and util functions which I recommend too but you can also try separating concerns of your validations.

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

Correct me if I am wrong but it seems your code is trying to validate before actually processing the response so here is an alternative way to write your validations:

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

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

You could invert the if statement to make it clearer as in:

您可以反转if语句以使其更清晰,如:

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

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

    return;
}

You could then refactor the conditionals using meaningful method names like "responseIsValid()" and "responseIsInvalid()".

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

#8

Helper functions can simplify the nested conditionals.

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

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

#9

The most awkward part is the getters like response.status() getting called many times when the logic appears to require a single, consistent value. Presumably it works because the getters are guaranteed to always return the same value, but it misexpresses the intent of the code and makes it more fragile to the current assumptions.

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

To fix this, the code should get response.status() once,

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

var responseStatus = response.status();

, then just use responseStatus thereafter. This should be repeated for the other getter values that are assumed to be the same value on each getting.

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

Additionally, all of these gettings would ideally be done at the same sequential point if this code might later be refactored into a thread-safe implementation in a more dynamic context. The gist is that you mean to get the values of the response at some particular point in time, so the critical section of code should get those values in one synchronous process.

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

In general, correctly specifying the intended data flow makes the code more resilient and maintainable. Then if someone needs to add a side-effect to a getter or make response an abstract data type, it'll be far more likely to continue working-as-intended.

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

#10

Disclaimer: I will not question the signature of the presented function, nor the functionality.

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

It feels awkward, to me, because the function is going a lot of work by itself, rather than delegating it.

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

In this case, I would suggest hoisting out the validation part:

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

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

Note that I prefer repeating the return statement rather than nest conditions. This keeps the code flat, reducing cyclomatic complexity. Plus, there's no guarantee that you will always want to return the same result for all error codes.

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

Afterward, parseResponse becomes easy:

之后,parseResponse变得容易:

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

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

---

You can, instead, use a functional style.

相反,您可以使用功能样式。

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

Though personally I find the extra nesting a tad annoying.

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

#11

I think the following is equivalent. But as others have pointed out, code transparency can be more important that "simple" code.

我认为以下内容是等效的。但正如其他人所指出的那样,代码透明度对于“简单”代码来说更为重要。

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

#12

you can have a set of code which you want to log and the common condition of payload not present

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

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

Correction in condition.

条件更正。

#13

Branching on an integer by comparing it to a finite set of explicit values is best handled by a switch:

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

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

Log();
return A;

#14

Just use a variable like JLRishe's answer. But I'd argue that code clarity is far more important than not duplicating a simple boolean check. You can use early return statements to make this a lot clearer:

只需使用像JLRishe的答案这样的变量。但我认为代码清晰度比不复制简单的布尔检查要重要得多。您可以使用早期的return语句来使它更清晰:

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

It is possible to factor out the repeated P test. The following (pseudo code) is logically equivalent to the code in your question.

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

In terms of readability I would go with the following (again pseudo code):

在可读性方面,我将使用以下(再次伪代码):

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

with perhaps more appropriately named functions.

可能更适合命名的功能。

#16

With this set of conditions, I don't think there's a way around some duplication. However, I prefer to keep my conditions separated as much as reasonable & duplicate other areas when it's necessary.

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

If I were writing this, keeping with the current style, it would be something like this:

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

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

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

Simplicity of code has some subjective elements and readability is hugely subjective. Given that, if I were going to write this method from scratch, this is what I would have given my biases.

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

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

If I remove my comments, this still almost doubles the lines of code. Some would argue that this could not make the code simpler. For me, it does.

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

#17

I'm not sure what the code tries to do: honestly, logging only 404 status and returning an empty list when code is not 200 feels like your're trying to avoid a NPE...

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

I think it's way better something like:

我认为这样的方式更好:

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 for any reason the if logic cannot be changed, i will anyway move the validation into a separate function.

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

Also, try to use Java naming conventions:

另外,尝试使用Java命名约定:

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