Gabriel Amyot Gabriel Amyot - 3 months ago 14
Java Question

SonarQube - boolean logic correctness -

I have a problem with the logic expression on my method matches1().

Problem



SonarQube is telling me there is an error:
(expectedGlobalRule == null && actual != null)



SonarQube:
Change this condition so that it does not always evaluate to
"true".
Conditions should not unconditionally evaluate to "TRUE" or to "FALSE"


I'm essentially doing this logic to avoid a NPE on my "Block to be executed".

My code



matches1()

private boolean matches1(GbRule actual, GbRule expected) {
if(actual == null && expected == null) {
return true;
} else if((expected == null && actual != null) || (expected != null && actual == null)) {
return false;
} else {
//Block to be executed
}
}


I inverted the logic in to see what SonarQube would tell me and he doesn't complain about it.
matches2()

private boolean matches2(GbRule actual, GbRule expected) {
if(actual == null && expected == null) {
return true;
} else if(expected != null && actual != null) {
//Block to be executed
} else {
return false;
}
}


Question




  1. Do the problem is in my boolean logic or it's SonarQube that lost
    his mind?

  2. If the problem is within sonarQube, how could I resolve it?


Answer

The problem is in your logic. Let's take it piece by piece:

 if(actual == null && expected == null) {
    return true;

At this point if both vars are null then we're no longer in the method. So if we get any further, then at least one of them is non-null.

The viable options at this point are:

  • actual = null, expected = non-null

  • actual = non-null, expected = null

  • actual = non-null, expected = non-null

Now, let's look at the next bit of code:

 } else if((expected == null && actual != null) 

We already know that both variables can't be null, so as soon as we know expected == null, there's no need to test whether actual != null. That has already been proven by the fact that we got this far. So actual != null is always true, which is why an issue is raised.

Edit

This means that your code could be boiled down to:

private boolean matches1(GbRule actual, GbRule expected) {
  if(actual == null && expected == null) {
    return true;
  } else if(actual == null || expected == null) {
    return false;
  } 

  //Block to be executed
}

Note that the else isn't needed & dropping it makes the code easier to read.