Standard usage of signum is <, >, ==. Not direct comparison with -1, 0, 1

[henri-tremblay]

The compareTo in Java was made to match the mental comparison we would do with numbers. x - y < 0, x - y == 0, x - y > 0 would transfer to x.compareTo(y) < 0, x.compareTo(y) == 0, x.compareTo(y) > 0. We never directly compare to -1, 0, 1 to keep the lisibility.

When using signum, it's the same thing. You would do x.signum() < 0 making it clear it's a negative number. When doing x.signum() == -1, you need to think about it. "What do -1 mean?" "Ah! It means negative".

I would highly suggest that error prone follows these guidelines. To be sure it wasn't only me, I asked some my favorite AI what it prefers. Here is the answer.

Recommendation

Use bigDecimal.signum() > 0 because:

1. It clearly communicates that you want to check if the number is positive
2. It's more maintainable and less brittle
3. It follows the principle of expressing intent rather than implementation details
4. It's the same pattern you'd use for other numeric types (number > 0)

[timtebeek]

We'll likely want to make the matching changes to this expected output file:

error-prone-support/error-prone-contrib/src/test/resources/tech/picnic/errorprone/refasterrules/BigDecimalRulesTestOutput.java

Lines 32 to 54 in 62077da

	ImmutableSet<Boolean> testBigDecimalSignumIsPositive() {
	return ImmutableSet.of(
	BigDecimal.valueOf(1).signum() == 1,
	BigDecimal.valueOf(2).signum() == 1,
	BigDecimal.valueOf(3).signum() == 1,
	BigDecimal.valueOf(4).signum() == 1,
	BigDecimal.valueOf(5).signum() != 1,
	BigDecimal.valueOf(6).signum() != 1,
	BigDecimal.valueOf(7).signum() != 1,
	BigDecimal.valueOf(8).signum() != 1);
	}
	ImmutableSet<Boolean> testBigDecimalSignumIsNegative() {
	return ImmutableSet.of(
	BigDecimal.valueOf(1).signum() == -1,
	BigDecimal.valueOf(2).signum() == -1,
	BigDecimal.valueOf(3).signum() == -1,
	BigDecimal.valueOf(4).signum() == -1,
	BigDecimal.valueOf(5).signum() != -1,
	BigDecimal.valueOf(6).signum() != -1,
	BigDecimal.valueOf(7).signum() != -1,
	BigDecimal.valueOf(8).signum() != -1);
	}

[Stephan202]

Thanks for reporting this @henri-tremblay! There's some history behind the current setup, but indeed we may want to revisit this. I'll have a closer look at the PR later ~today. (Don't worry about fixing the tests; we can take care of that.) In the interim I'll CC @Venorcis, as he also had opinions.

[henri-tremblay]

Thanks. I've fixed the test. Not sure why I did noticed. The change is from 2023 by @Venorcis. From #812.

[henri-tremblay]

Reading the comments from #812 , you were saying the same thing as me @Stephan202 , then changed your mind. I liked the way you thought originally :-)

[Venorcis]

In the interim I'll CC @Venorcis, as he also had opinions.

Thanks, I do indeed 😄 As highlighted here when introducing this check, unlike the comparator, the signum is mathematically defined to have one of three values. This fact even has relevant implications, such as being able to use the signum for multiplication (and other more fancy derivations). The Wikipedia article on the Sign function also nicely summarizes this:

The signum function denotes which unique category a number falls into by mapping it to one of the values −1, +1 or 0, which can then be used in mathematical expressions or further calculations.

This makes it way more significant than what AI apparently proclaims to be an 'implementation detail'. I thus think it's good to keep this fact explicit, and not conflate the signum of a number with the number itself (which is what this change would do IMO).

Something I could definitely get behind is allowing (i.e. not rewriting) the use of compareTo, as that's the operation intended to represent the 'traditional' comparison operators for BigDecimals. A quick search also shows that to be the most widely suggested.

[github-actions]

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.