Prefer `BigDecimal.valueOf(double)` over `new BigDecimal(double)` #394

Venorcis · 2022-12-06T21:17:15Z

A very nice rule to rewrite new BigDecimal(long/int) to BigDecimal.valueOf(long/int) already existed. As using new BigDecimal(double) is quite discouraged, this PR extends the existing rule to rewrite new BigDecimal(double/long/int) to BigDecimal.valueOf(double/long/int)

Venorcis · 2022-12-06T21:33:48Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BigDecimalRules.java

-  /** Prefer {@link BigDecimal#valueOf(long)} over the associated constructor. */
-  // XXX: Ideally we'd also rewrite `BigDecimal.valueOf("<some-integer-value>")`, but it doesn't
+  /** Prefer {@link BigDecimal#valueOf(double)} over the associated constructor. */
+  // XXX: Ideally we'd also rewrite `new BigDecimal("<some-integer-value>")`, but it doesn't


think this was meant here; not sure if this is a nice comment to keep around anyway, but I'll leave that up to you 😉

Hmm, I wonder whether the comment meant a bit of both: if "<some-integer-value>" represents a value that can be expressed as a long, then passing in a long/int is nicer than a string. (But such a change can't be done using Refaster; it'd require a BugChecker.)

Ah, I see that BigDecimal.valueOf doesn't exist. So certainly that was not meant 😄

github-actions · 2022-12-06T21:34:54Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Venorcis · 2022-12-06T21:38:21Z

...ontrib/src/test/resources/tech/picnic/errorprone/refasterrules/BigDecimalRulesTestInput.java

@@ -18,6 +18,6 @@ ImmutableSet<BigDecimal> testBigDecimalTen() {
  }

  ImmutableSet<BigDecimal> testBigDecimalFactoryMethod() {
-    return ImmutableSet.of(new BigDecimal(0), new BigDecimal(0L));
+    return ImmutableSet.of(new BigDecimal(2), new BigDecimal(2L), new BigDecimal(2.0));


felt 2 was a nicer value to use here as there is a rule above to rewrite 0 to ZERO as well

also did not include 2F because there is no explicit BigDecimal(float) constructor, whilst there are separate ones for int, long and double; in this sense you could actually consider dropping the integer valueOfs in the existing tests above because they just map to the valueOf(long) anyway (there is no explicit int version).

Stephan202

Nice one @Venorcis! I'll circle back later for a full review :)

Stephan202 · 2022-12-07T06:25:07Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BigDecimalRules.java

-  /** Prefer {@link BigDecimal#valueOf(long)} over the associated constructor. */
-  // XXX: Ideally we'd also rewrite `BigDecimal.valueOf("<some-integer-value>")`, but it doesn't
+  /** Prefer {@link BigDecimal#valueOf(double)} over the associated constructor. */
+  // XXX: Ideally we'd also rewrite `new BigDecimal("<some-integer-value>")`, but it doesn't


Hmm, I wonder whether the comment meant a bit of both: if "<some-integer-value>" represents a value that can be expressed as a long, then passing in a long/int is nicer than a string. (But such a change can't be done using Refaster; it'd require a BugChecker.)

Stephan202

Added a small commit. Have a meeting now; will finalize review afterwards :)

Stephan202 · 2022-12-07T08:49:50Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BigDecimalRules.java

-  /** Prefer {@link BigDecimal#valueOf(long)} over the associated constructor. */
-  // XXX: Ideally we'd also rewrite `BigDecimal.valueOf("<some-integer-value>")`, but it doesn't
+  /** Prefer {@link BigDecimal#valueOf(double)} over the associated constructor. */
+  // XXX: Ideally we'd also rewrite `new BigDecimal("<some-integer-value>")`, but it doesn't


Ah, I see that BigDecimal.valueOf doesn't exist. So certainly that was not meant 😄

github-actions · 2022-12-07T08:54:44Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202

Added one more commit. Suggested commit message:

Prefer `BigDecimal.valueOf(double)` over `new BigDecimal(double)` (#394)

See https://rules.sonarsource.com/java/RSPEC-2111

Stephan202 · 2022-12-07T12:50:30Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/refasterrules/BigDecimalRules.java

+  /** Prefer {@link BigDecimal#valueOf(double)} over the associated constructor. */
+  // XXX: Ideally we also rewrite `new BigDecimal("<some-integer-value>")` in cases where the
+  // specified number can be represented as an `int` or `long`, but that requires a custom
+  // `BugChecker`.
  static final class BigDecimalFactoryMethod {


While we're here, let's rename this rule in line with our current naming strategy.

Suggested change

static final class BigDecimalFactoryMethod {

static final class BigDecimalValueOf {

github-actions · 2022-12-07T12:53:28Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2022-12-07T12:55:34Z

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Nice catch @Venorcis 🚀 !

Venorcis force-pushed the vkoeman/extend-bigdecimal-rules branch from cc81eb8 to 6f11b4c Compare December 6, 2022 21:26

This comment was marked as outdated.

Sign in to view

rickie requested review from Stephan202 and rickie December 6, 2022 21:28

rickie added the new feature label Dec 6, 2022

Venorcis force-pushed the vkoeman/extend-bigdecimal-rules branch from 6f11b4c to 346407b Compare December 6, 2022 21:33

Venorcis commented Dec 6, 2022

View reviewed changes

Stephan202 reviewed Dec 7, 2022

View reviewed changes

Stephan202 approved these changes Dec 7, 2022

View reviewed changes

Venorcis and others added 3 commits December 7, 2022 13:53

Extend the refaster rule for new BigDecimal(...) to doubles

25ca321

Tweak comment

6b473b6

Rename rule

dce6db6

Stephan202 force-pushed the vkoeman/extend-bigdecimal-rules branch from cc3af0f to dce6db6 Compare December 7, 2022 12:53

Stephan202 added this to the 0.6.0 milestone Dec 7, 2022

rickie approved these changes Dec 7, 2022

View reviewed changes

rickie changed the title ~~Extend the refaster rule for new BigDecimal(...) to doubles~~ Prefer BigDecimal.valueOf(double) over new BigDecimal(double) Dec 7, 2022

rickie merged commit bc1f204 into master Dec 7, 2022

rickie deleted the vkoeman/extend-bigdecimal-rules branch December 7, 2022 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefer `BigDecimal.valueOf(double)` over `new BigDecimal(double)` #394

Prefer `BigDecimal.valueOf(double)` over `new BigDecimal(double)` #394

Venorcis commented Dec 6, 2022 •

edited

Loading

This comment was marked as outdated.

Venorcis Dec 6, 2022 •

edited

Loading

Stephan202 Dec 7, 2022

Stephan202 Dec 7, 2022

github-actions bot commented Dec 6, 2022

Venorcis Dec 6, 2022 •

edited

Loading

Stephan202 left a comment

Stephan202 Dec 7, 2022

Stephan202 left a comment

Stephan202 Dec 7, 2022

github-actions bot commented Dec 7, 2022

Stephan202 left a comment

Stephan202 Dec 7, 2022

github-actions bot commented Dec 7, 2022

github-actions bot commented Dec 7, 2022

rickie left a comment

	static final class BigDecimalFactoryMethod {
	static final class BigDecimalValueOf {

Prefer BigDecimal.valueOf(double) over new BigDecimal(double) #394

Prefer BigDecimal.valueOf(double) over new BigDecimal(double) #394

Conversation

Venorcis commented Dec 6, 2022 • edited Loading

This comment was marked as outdated.

Venorcis Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Stephan202 Dec 7, 2022

Choose a reason for hiding this comment

Stephan202 Dec 7, 2022

Choose a reason for hiding this comment

github-actions bot commented Dec 6, 2022

Venorcis Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Dec 7, 2022

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Dec 7, 2022

Choose a reason for hiding this comment

github-actions bot commented Dec 7, 2022

Stephan202 left a comment

Choose a reason for hiding this comment

Stephan202 Dec 7, 2022

Choose a reason for hiding this comment

github-actions bot commented Dec 7, 2022

github-actions bot commented Dec 7, 2022

rickie left a comment

Choose a reason for hiding this comment

Prefer `BigDecimal.valueOf(double)` over `new BigDecimal(double)` #394

Prefer `BigDecimal.valueOf(double)` over `new BigDecimal(double)` #394

Venorcis commented Dec 6, 2022 •

edited

Loading

Venorcis Dec 6, 2022 •

edited

Loading

Venorcis Dec 6, 2022 •

edited

Loading