Rule java:S2111 flags any use of the BigDecimal(double) constructor because of the potential for hideously long (and inaccurate) numbers caused by inherent floating point imprecision. These are major bugs (used to be critical).
While the recommended replacements in the rule description (e.g., BigDecimal.valueOf() ) are probably the best approach overall, some “second-best” approaches are still getting flagged.
Java at some point (I think 1.5) added a constructor (double, MathContext) which lets you specify the precision and roundoff. Use of this constructor avoids the inaccuracy problem (assuming a reasonable value to the MathContext constructor). But SonarLint still flags:
(The blue squiggle is from SonarLint in CodeReady). It also flags the rule if the constructor call is followed by a call to setScale:
This is probably a “third-rate” solution because it could involve creating the long number and then fixing it, unless the compiler(s) optimize and just make the BigDecimal constant up front.
But either way, the situation in the rule description won’t occur here. So I would make exceptions to the rule for when 1) the (double, MathContext) constructor is used, and 2) the plain (double) constructor is chained with a setScale() method call. (Maybe there could be a separate smell rule for those cases since I agree they’re still “smelly.”)
Looking at the JavaDocs for the BigDecimal(double, MathContext) constructor, it comes with the same warning as the constructor without MathContext. As such, it can occur using this constructor as well and the rule description is accurate. Even if you spend some thought on rounding yourself, you may still be surprised by some unintuitive floating point rounding error.
While we can always discuss severities and rules often capture a number of cases with varying severities, I don’t think the most valuable solution would be to introduce a new code smell rule here at this moment.
Well, a key phrase in my comment was “assuming a reasonable value to the MathContext constructor” – one can still put in an unreasonable value so I guess the Java folks felt they needed to cover all bases.
When you say “rules often capture a number of cases with varying severities” do you mean some rules will cause different issues to be reported with different severities?
Anyway, I don’t like the rule’s default recommendation to use valueOf() as much as I said I did before. We still see naive coders do something like BigDecimal.valueOf(0.10000) as if the trailing zeroes make a difference (they don’t), so I’ve been telling people to use either the two-arg form of valueOf() or the String constructor, since both give you exact control over the precision/scale without having to chain additional methods.
I think choosing a reasonable value for the MathContext may not be trivial here. The value of the float passed as an argument may depend on the JVM implementation. Even if we have a good idea of what this value might be, it requires some good understanding of floating-point inaccuracies by those who write and those who maintain the code. For many, it can still yield unexpected results, certainly more so than using the method accepting a String argument, which does exactly what you would expect.
Well I agree with you to a point. If someone uses, say, 10, I’m either going to head over to a whiteboard, or start typing search queries. But in our case, nobody does that. It’s 2, or 4, or, at worst, 6. So one could probably pick ranges of reasonable, unreasonable, or not sure. Or just make a parameter.
In our case, we have some legacy code. Now, my urge is just to fix it (maybe I was a beaver in a past life?) but then it would have to go through regression etc. as this is “mission critical” code. So, to get a lot of nothingburgers out of the stats, I scripted something that automatically went through and demoted some of them to code smells.
If the double literal can be represented EXACTLY, like anything 0, call it an info smell.
Otherwise, if the constructor is followed by a call to setScale and the scale value is 6 or less, call it a critical smell. (My reasoning there is that you will still waste CPU cycles by turning a literal into a 60-digit monstrosity, only to clean it up with a reasonable call to setScale. So it’s not really a bug anymore, but definitely “smellier” than case 1. (SQ tends to call things it regards as perf hits as either smells or bugs according to how bad the perf cost is.)
