15

We are checking the quality of our code using Sonar, and Sonar found code which compares a float or double for equality with a constant value like this:

if (x == 0.0) { … }

The value the variable is compared with (0.0) is constant, and in case the variable can be equal to this value, the value also isn't computed but only set via a constant. This is typically used to check whether a variable hasn't been set yet or is still at initialization state, e. g. -1.0 might be used for "not yet set" in cases where the value can only be positive.

So, since these values are never computed but only set from constants, the Sonar complaint is not useful for us. Only for computed values (or fractured ones which are not precisely representable as floats or doubles) a complaint about a test for equality makes sense.

The question I have now is: What is the best practice to change the code so that Sonar does not complain about this anymore?

I see several options:

  1. Extract the "test-for-unset" into a special test function; but that would only reduce the number of occurrences (to 1), not the issue in general.
  2. Mark the code for Sonar to ignore it with a special decorator. But we would like to avoid using such decorators.
  3. Hide the comparison behind sth like (0.0 <= x && x <= 0.0) or !(x != 0.0) (which currently seems to be okay for Sonar).
  4. Calling Double.doubleToRawLongBits() to compare the bits of the values like this: (Double.doubleToRawLongBits(x) != Double.doubleToRawLongBits(0.0)).
  5. Other ideas?

None of these solutions I really like and I thought, maybe, there is a better one out there I can't think of.

Alfe
  • 56,346
  • 20
  • 107
  • 159
  • If these values are only set from constants, why are they `double`? Why not `int`, or better yet, `enum`? – Kevin Krumwiede Jun 19 '15 at 15:38
  • The values are, e. g. heights of physical objects. These are doubles by nature. But in some cases the provider of the values wants to signal that this values hasn't been set yet, then it would return a -1.0 instead. A clean solution would be to have an additional flag field stating whether the value has been set or not. But introducing lots of flags now seems like an overkill. – Alfe Jun 19 '15 at 15:47
  • I would avoid using zero to indicate an uninitialized value. It probably makes sense to make the testable value a java.lang.Double rather than a double, and compare it to null, since null's purpose is to indicate the absence of a value. – VGR Jun 19 '15 at 19:12
  • Using `Double` instead of `double` increases the memory consumption in an unacceptable way. Like adding an extra field as an "unset"-flag. – Alfe Jun 23 '15 at 12:20

7 Answers7

18

I would go with your second option:

Mark the code for Sonar to ignore it with a special decorator.

Don't be a slave to static code analysis tools. They're not perfect, and there's nothing wrong with telling them to shut up. My personal practice when using annotations like @SuppressLint is to include a comment explaining why I'm using it.

That said, I would create a constant so the code is more self-explanatory:

private static final double UNINITIALIZED = 0.0;
if (x == UNINITIALIZED) { … }
Kevin Krumwiede
  • 9,868
  • 4
  • 34
  • 82
  • Actually, that constant we already have. I just didn't want to complicate things in my question. – Alfe Jun 23 '15 at 12:07
  • 7
    That: "*Don't be a slave to static code analysis tools*" :-) – assylias Jun 24 '15 at 08:18
  • Isn't the warning is about http://stackoverflow.com/questions/3832592/test-for-floating-point-equality-fe-floating-point-equality and http://stackoverflow.com/questions/8819738/why-does-double-nan-double-nan-return-false . – Jayan Jun 27 '15 at 03:35
  • 2
    @Jayan Probably, but those concerns do not apply in this case. As the OP explained, "these values are never computed but only set from constants." Sonar doesn't understand that, so it emits a false warning. – Kevin Krumwiede Jun 27 '15 at 03:47
  • @KevinKrumwiede best answer ever. – crigore Jun 30 '15 at 08:49
  • If you compare two floating point with the == you will generate a critical bug. – felix at housecat Mar 06 '20 at 16:37
  • @felixathousecat Comparing floats with `==` is not always wrong. In the asker's case it's exactly what's needed. – Kevin Krumwiede Mar 07 '20 at 16:44
5

The best option here is to mark the issue as false positive and to leave a comment. This way the issue and associated technical debt will disappear from your SonarQube instance, without polluting your code with annotations.

Mithfindel
  • 4,553
  • 1
  • 23
  • 32
2

If you only use these constants and comparisons for uninitialized values, one option is to set the fields to Double.NaN and use Double.isNaN() for comparison. E.g.:

double notYetInitialized = Double.NaN;
if (Double.isNaN(notYetInitialized)) {
        // handle uninitialized value
}

This makes (some) sense when reading the code - an unintialized value could possibly be said to "not be a number". And I can not imagine Sonar would have a problem with it.

K Erlandsson
  • 13,408
  • 6
  • 51
  • 67
1

There is also the correct way of doing this:

private static final double EPSILON = 0.000000000000001;
if (Math.abs(x) < EPSILON) {

}
ACV
  • 9,964
  • 5
  • 76
  • 81
  • Hmm. You want me to compare a variable to a constant value like `-1.0` which indicates a state like *uninitialized* by using an epsilon? That doesn't sound sound. – Alfe Oct 20 '17 at 10:28
  • Doesn't sound good. Yes it's overkill, but it is the only way to do it without Sonar complaining. – ACV Oct 20 '17 at 22:09
  • @Alfe, `-1.0` should never mean uninitialized. For uninitialized return Double `null`. – ACV Jan 17 '18 at 11:18
  • 1
    I elaborated already that using `Double` instead of `double` is no option because we are using huge amounts of them and the overhead introduced by the proper object is unacceptable. So we are using `double` outside of its original scope, in a slightly hacky and unclean (but not unusual) way which could be called a specific optimization. – Alfe Jan 17 '18 at 12:37
  • @Alfe, I see. In that case, just ignore that specific Sonar issue – ACV Jan 17 '18 at 16:00
1

I know this is a really old post, but I faced the same problem, and solved it using the Equals method instead of the "==" operator.

if (x.Equals(0.0)) { … }

In this way SonarQube will not complain.

Bye.

Alfe
  • 56,346
  • 20
  • 107
  • 159
  • Good finding! But I would expect that `a.Equals(b)` should be treated like `a == b` in all cases, so as a Sonar developer I would adjust my product so that it does this as soon as I hear that it doesn't do it yet. So good luck with your solution, but it might be a temporary one. It would fit in my option 3, btw ;-) – Alfe Sep 30 '19 at 08:58
0

Suggestion : One of the good way would be using BigDecimal for checking equality/non-equality to 0:

BigDecimal balance = pojo.getBalance();//get your BigDecimal obj

0 != balance.compareTo(BigDecimal.ZERO)

Explanation :

The compareTo() function compares this BigDecimal with the specified BigDecimal. Two BigDecimal objects that are equal in value but have a different scale (like 2.0 and 2.00) are considered equal by this method. This method is provided in preference to individual methods for each of the six boolean comparison operators (<, ==, >, >=, !=, <=). The suggested idiom for performing these comparisons is: (x.compareTo(y) <op> 0), where is one of the six comparison operators.

(Thanks to SonarQube documentation)

Floating point math is imprecise because of the challenges of storing such values in a binary representation. Even worse, floating point math is not associative; push a float or a double through a series of simple mathematical operations and the answer will be different based on the order of those operation because of the rounding that takes place at each step.

Even simple floating point assignments are not simple:

float f = 0.1; // 0.100000001490116119384765625
double d = 0.1; // 0.1000000000000000055511151231257827021181583404541015625

(Results will vary based on compiler and compiler settings);

Therefore, the use of the equality (==) and inequality (!=) operators on float or double values is almost always an error. Instead the best course is to avoid floating point comparisons altogether. When that is not possible, you should consider using one of Java's float-handling Numbers such as BigDecimal which can properly handle floating point comparisons. A third option is to look not for equality but for whether the value is close enough. I.e. compare the absolute value of the difference between the stored value and the expected value against a margin of acceptable error. Note that this does not cover all cases (NaN and Infinity for instance).

Vivek
  • 895
  • 11
  • 23
  • Unfortunately, in my situation, replacing the `double` used here by a `BigDecimal` would mean to increase memory consumption (because `BigDecimal` is an object) and all involved computation times (because `double` as a primitive can be optimized better) dramatically, so this is not an option. And it seems not to be the "right thing". A `-1` or a `0` is perfectly representable as a `double` without any imprecisions as you mention them for `0.1`. – Alfe Feb 07 '18 at 23:09
  • 1
    @Alfe - I read through, got your point . As you've many of them so not advisable to turn to `BigDecimal` . Also agree in your case because of -1, 0 things can be achieved perfectly via `double` . For me , it was only once and a very dynamic double value and I encountered this question and series of answers and thought to put the world `BigDecimal` brings; As search for sonar and double comparison brings many to this web page :) ! – Vivek Feb 08 '18 at 23:04
-1

How about casting to (int) ?! That should have the same result as you want to check if it's zero.

If you have a calculation :

double a = c - b 
boolean x = (int)(a*100) == 0
amstegraf
  • 597
  • 1
  • 5
  • 11
  • Thanks for your answer but: No. Casting to `int` would reduce a `0.2` to `0` and the comparison would return `true` which it definitely should not do for a value like `0.2`. – Alfe Feb 02 '17 at 12:37
  • "So, since these values are never computed but only set from constants". Why would you set it to 0.2? Why would you make it a double anyways? If indeed is a false possive just suppress warnings. If you have a calculation double a = c - b you could also do boolean x = (int)(a*100) == 0 – amstegraf Feb 07 '17 at 13:40
  • The variable can either hold a computed value (a very common case) or one of a set of constant values which it never will reach via a computation (an initial value case). The phrase "these values are never computed" referred to these ones I want my variable to compare with. You are right, the other meaning you understood would not make sense. – Alfe Feb 07 '17 at 16:34
  • Ok. Take into consideration that the above edited answer, although simple, works with the required scenarios and doesn't throw a Sonar error, i've tested it. – amstegraf Feb 08 '17 at 13:07
  • How about not falsely detecting a computed value like 1e-10? – Alfe Feb 08 '17 at 16:17