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

Question

My colleague showed me this piece of code, and we both wondered why we couldn't seem to remove duplicated code. private List parseResponse(Response 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`?

Answer 1

There will almost always be a ton of ways to write any piece of code, and this piece here is no exception. I like that you wrote another version without the wordiness, so I will use that version in my explanation.

if(S != 200 || !P){

if(S != 404 || !P){

//Do something here

}

}

So in order to get to the "Do something here" line, we need one of two things to happen, either !P has to be true, or S has to be some value that is NOT 200 or 404. The main difference here, is that S has two things it must check (not being 200 AND not being 404), while !P just has one check, which is to be True. Because they have a different amount of checks, we have repeated pieces of code. Imagine if S also couldn't be 150, we would have this:

if(S != 200 || !P){

if(S != 404 || !P){

if(S != 150 || !P){

//Do something here

}

}

}

As we progress past two conditions for S, it becomes obvious that there should be a better way to compress this, and in fact, there is! One such option, would be to just combine all the S checks into a single check:

if((S != 200 && S != 404) || !P){

//Do something here

}

This would be my preferred version of the code, but there are numerous ways. You could also separate the checks altogether. First we see if !P is True, and only if it's not do we even worry about the values of S:

if(!P){

//Do something here

} else if(S != 200 && S != 404){

//Do something here

} else {

//If both are false, do the last thing

}

In both of these examples we can see that combining the S checks into a single check makes it much simpler. If you had more that two S values to check against, say 20, you could just make an entire method to just check S values and return a boolean.

Hope this helps!