SonarQube - boolean logic correctness -

Question

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 1

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.