BigDecimalDoubleConstructorRecipe should also convert BigDecimal.valueOf(doubleLiteral) -> new BigDecimal(stringLiteral)

[timo-abele]

What problem are you trying to solve?

The String constructor of BigDecimal is much nicer than BigDecimal.valueOf(doubleLiteral) because

• two chars shorter (even shorter if the double literal has trailing zeroes)
• more obvious that a new object is created
• performance gain because valueOf is converted to String constructor anyway during compilation (unlikely to be noticable).

Describe the solution you'd like

Per API BigDecimal.valueOf(double)

Translates a double into a BigDecimal, using the double's canonical string representation provided by the Double.toString(double) method.

In addition to transforming new Bigdecimal(existingDouble) to valueOf, BigDecimalDoubleConstructorRecipe should convert BigDecimal.valueOf(doubleLiteral) to new BigDecimal(doubleString) where doubleString = Double.toString(doubleLiteral).toString(). E.g. BigDecimal.valueOf(1.00) -> new BigDecimal("1.0")

Have you considered any alternatives or workarounds?

Additional context

Are you interested in contributing this feature to OpenRewrite?

Iff the idea is approved.

[timtebeek]

Thanks for the suggestion @timo-abele ! The current recipe is implemented as a refaster style recipe, which replaces new BigDecimal(d) with BigDecimal.valueOf(d).

https://github.com/openrewrite/rewrite-static-analysis/blob/88f3531b9a5fad4ebb97d0222b38c969012969b2/src/main/java/org/openrewrite/staticanalysis/BigDecimalDoubleConstructor.java#L24-L42

Do I understand correctly that you're looking to then go from BigDecimal.valueOf(1.00) to new BigDecimal("1.0") with an explicit recipe?

I'm not entirely sure that's what folks would expect these days, especially as we're moving other number constructors over to valueOf in https://docs.openrewrite.org/recipes/staticanalysis/primitivewrapperclassconstructortovalueof

https://github.com/openrewrite/rewrite-static-analysis/blob/88f3531b9a5fad4ebb97d0222b38c969012969b2/src/test/java/org/openrewrite/staticanalysis/PrimitiveWrapperClassConstructorToValueOfTest.java#L87-L116

Reintroducing explicit constructors for BigDecimal might then be confusing; any thoughts on that?

[timo-abele]

Hi, in my project the scale of a BigDecimal is relevant, and my takeaway is that initialization with a double literal should be avoided all together. Take this snippet:

List.of(
        new BigDecimal(1.00),
        BigDecimal.valueOf(1.00),
        new BigDecimal("1.00")
).forEach(bd -> System.out.printf("value: %-4s, scale: %s%n", bd, bd.scale()));

that prints

value: 1   , scale: 0
value: 1.0 , scale: 1
value: 1.00, scale: 2

Only the (argument of the) string constructor clearly indicates the BigDecimal value that is created.
I authored this issue before I refactored all BigDecimal initializations in my project, so that argument wasn't as clear to me then and is missing in my original description. Having refactored every BigDecimal initialization from a literal in my repo, I believe it is best to always use the string constructor. (and the int constructor is allowed as a shorter form iff a scale of 0 is intended¹)

Right now (v1.3.1) BigDecimalDoubleConstructorRecipe is already changing semantics when it rewrites new BigDecimal(1.00) -> BigDecimal.valueOf(1.00). It assumes that a dev who wrote new BigDecimal(1.00) which equals new BigDecimal("1") really meant² to write BigDecimal.valueOf(1.00) which equals new BigDecimal("1.0"). That's OK, IntelliJ suggests the same.

Similarly, I believe, that a dev who writes new BigDecimal(1.00) or BigDecimal.valueOf(1.00) expects new BigDecimal("1.00") to happen but doesn't know better. And reviewers and debuggers will be less confused too if the created precision matches the literal.

¹ There is also long, but we never use long literal to create a BigDecimal in my project. I personally prefer a constructor call with new to valueOf because then it is more obvious that a new object is created. And I believe that an official recommendation to prefer valueOf on a wrapper type does not carry an implication to prefer BigDecimal#valueOf, as BigDecimal doesn't wrap anything. But those opinions ultimately reduce to "I think this looks better", I'm open to compromise here :smiley:

² That's an assumption on my part of course. At the very least however it assumes that the semantics are worth changing.