CODEWITHSUNDEEP
javasonarqubeboolean-expression
Gabriel
Gabriel

Reputation: 306

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?

Upvotes: 4

Views: 7485

Answers (3)

G. Ann - SonarSource Team
G. Ann - SonarSource Team

Reputation: 22804

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.

Upvotes: 7

GhostCat
GhostCat

Reputation: 140465

Even when the code is correct; seriously, it makes my eyes hurt. Thing is: it is hard to read. Such kind of nested conditions is something that one should not be writing in the first place.

If you can't avoid it; at least refactor it into something like

private boolean areActualAnedExpectedBothNull(args ...) {
  return actual == null && expectedGlobalRule == null;
}

And please note; you can dramatically simply your code:

if (areActualAnedExpectedBothNull(actual, expected)) {
  return true;
}
if (actual == null) {
  return false;
}

if (expected == null) {
  return false;
}

do your thing ...

and use such methods in your other code. And of course, you do a lot of unit testing; probably with coverage measurements; just to make sure that your tests really test all possible paths through this maze.

But as said; you better step back and think if there are ways to avoid writing such code in the first place.

The typical answer to booleans, and if/else chains in OO programming is polymorphism. So instead of asking something about its state; you turn to interfaces/abstract classes; and have different implementations of those. And then you have a factory giving you that implementation you need; and then you just call methods on that; without further need for if/else/whatever.

If you don't know what I am talking about - watch these videos; especially the second one!

Upvotes: 3

nhouser9
nhouser9

Reputation: 6780

The problem is with SonarQube.

See this article for more info on ignoring that issue: https://www.bsi-software.com/en/scout-blog/article/ignore-issues-on-multiple-criteria-in-sonarqube.html

You can just set it up to ignore that error within that file.

The gist of it is

Open the Settings (SonarQube General Settings or project Settings) and select the Exclusions category. Switch to the Issues Exclusions and scroll down to “Ignore Issues on Multiple Criteria”. Set squid:S00112 as Rule Key Pattern and **/*Activator.java as File Path Pattern.

You will need to change the rule key pattern to the pattern associated with the rule that is being violated for your code and the file pattern as the path of your .java file.

Upvotes: -3

Related Questions