CODEWITHSUNDEEP
javaif-statementlogicrefactoringboolean-logic
Andrew Cheong
Andrew Cheong

Reputation: 30313

How can I simplify this set of if statements? (Or, what's making it feel so awkward?)

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

Upvotes: 31

Views: 6151

Answers (17)

emont01
emont01

Reputation: 3230

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.

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

Upvotes: 5

Matteo A
Matteo A

Reputation: 358

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

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.

Also, try to use Java naming conventions:

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

Upvotes: 0

JLRishe
JLRishe

Reputation: 101778

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:

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:

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

Upvotes: 29

Acccumulation
Acccumulation

Reputation: 3591

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

Upvotes: 2

Gary99
Gary99

Reputation: 1770

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.

Upvotes: 1

Vectorjohn
Vectorjohn

Reputation: 411

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

(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:

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

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

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):

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:

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.

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.

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.

Upvotes: 10

Zachary
Zachary

Reputation: 1703

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)

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 (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).

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

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

Upvotes: 21

Jibbity jobby
Jibbity jobby

Reputation: 1325

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

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.

Upvotes: 1

user673679
user673679

Reputation: 1366

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:

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

Upvotes: 1

Matthieu M.
Matthieu M.

Reputation: 300439

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.

Afterward, parseResponse becomes easy:

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.

Upvotes: 2

Alexander
Alexander

Reputation: 63399

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;

Upvotes: 1

Pieter Geerkens
Pieter Geerkens

Reputation: 11893

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()?

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.

Upvotes: 14

Nat
Nat

Reputation: 1096

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.

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

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.

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.

Upvotes: 3

Kerri Brown
Kerri Brown

Reputation: 1187

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

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()".

Upvotes: 4

jxh
jxh

Reputation: 70502

Helper functions can simplify the nested conditionals.

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

Upvotes: 3

Java Samurai
Java Samurai

Reputation: 98

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.

Upvotes: 1

OldCurmudgeon
OldCurmudgeon

Reputation: 65889

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

Upvotes: 13

Related Questions