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).

rewrite-static-analysis/src/main/java/org/openrewrite/staticanalysis/BigDecimalDoubleConstructor.java

Lines 24 to 42 in 88f3531

	@RecipeDescriptor(
	name = "`new BigDecimal(double)` should not be used",
	description = "Use of `new BigDecimal(double)` constructor can lead to loss of precision. Use `BigDecimal.valueOf(double)` instead.\n" +
	"For example writing `new BigDecimal(0.1)` does not create a `BigDecimal` which is exactly equal to `0.1`, " +
	"but it is equal to `0.1000000000000000055511151231257827021181583404541015625`. " +
	"This is because `0.1` cannot be represented exactly as a double (or, for that matter, as a binary fraction of any finite length).",
	tags = {"RSPEC-2111"}
	)
	public class BigDecimalDoubleConstructor {
	@BeforeTemplate
	BigDecimal bigDecimalDoubleConstructor(double d) {
	return new BigDecimal(d);
	}
	@AfterTemplate
	BigDecimal bigDecimalValueOf(double d) {
	return BigDecimal.valueOf(d);
	}

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

rewrite-static-analysis/src/test/java/org/openrewrite/staticanalysis/PrimitiveWrapperClassConstructorToValueOfTest.java

Lines 87 to 116 in 88f3531

	java(
	"""
	class A {
	Boolean bool = new Boolean(true);
	Byte b = new Byte("1");
	Character c = new Character('c');
	Double d = new Double(1.0);
	Float f = new Float(1.1f);
	Long l = new Long(1);
	Short sh = new Short("12");
	short s3 = 3;
	Short sh3 = new Short(s3);
	Integer i = new Integer(1);
	}
	""",
	"""
	class A {
	Boolean bool = Boolean.valueOf(true);
	Byte b = Byte.valueOf("1");
	Character c = Character.valueOf('c');
	Double d = Double.valueOf(1.0);
	Float f = Float.valueOf(1.1f);
	Long l = Long.valueOf(1);
	Short sh = Short.valueOf("12");
	short s3 = 3;
	Short sh3 = Short.valueOf(s3);
	Integer i = Integer.valueOf(1);
	}
	"""
	)

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 😃

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