Introduce assorted BigDecimal#signum Refaster rules

[Venorcis]

Expands the BigDecimalRules, mainly preventing contrived uses of compareTo(BigDecimal.ZERO) by using signum() instead.
Also adds some rules surrounding the use of signum().

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

[rickie]

Added a commit with some suggestions :).

Suggested commit message (repeating title):

Introduce assorted `BigDecimal#signum` Refaster rules (#812)

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

[Stephan202]

W.r.t. the incorrect cases: I have on a TODO list somewhere the idea to generate unit tests from these Refaster rules, so that this kind of thing would be automatically flagged.

Rebased and added two commits; lemme know what you think.

[Stephan202]

Instead of == 1 and == -1, shall we settle on > 0 and < 0 respectively? That would:

1. Be more consistent with the "Inclusive" rules.
2. (IMHO) be easier to interpret (the two mistakes in the current rule set also hint at that).
3. More clearly communicate that the full range of integers is supported (even though in practice only -1, 0 and 1 will be returned).
4. Maintain consistency with how Comparators should be used.

[Stephan202]

(^ With this change we can also further collapse the rules using @AfterTemplate.)

[Venorcis]

Instead of == 1 and == -1, shall we settle on > 0 and < 0 respectively?

IMO == 1 and == -1 are more readable as meaning 'positive sign' and 'negative sign'. The signum function is also mathematically defined like that (unlike comparators), which Java follows:

[Stephan202]

That's fair, but Java's type system still maps those numbers onto larger domain, with the alternative making clear how such won't-happen output would be treated. I.e., a trade-off between mathematical fact and programming pragmatics 🤷

[Venorcis]

I'm not sure if the type should be leading; such reasoning could be extrapolated to mean you should be prepared for e.g. Collection#size returning something negative 😛 Perhaps this discussion signals that either < 0 or == -1 should be acceptable, but that other variants are not (and ofc. we need to pick a default of how to rewrite those here).

[Stephan202]

such reasoning could be extrapolated to mean you should be prepared for e.g. Collection#size returning something negative

Luckily we can side step that particular case ;)

Perhaps this discussion signals that either < 0 or == -1 should be acceptable, but that other variants are not (and ofc. we need to pick a default of how to rewrite those here).

Indeed, we could also drop some of the cases from the @BeforeTemplate body. Downside of that is that we won't "clean up" mixed-use code-bases. (And it would still require agreement on what the @AfterTemplate should contain 😅)

Perhaps this is a case where a flag could help, though right now Refaster doesn't support that (but the idea was also floated in #654 and #655), while a BugChecker approach would be way more work.

Maybe I'm too a-mathematical on this point; will think about it. Also interested in @rickie's thoughts on this.

NB: Whatever we settle on: we can do the same for {Integer,Long}#signum. 😄

[Stephan202]

Upon further reflection, the signum is often multiplied; an operation that crucially depends on the [-1, 0, 1] domain. That's an argument in favour of just embracing that fact. If we do, however, it does make sense to me that we always compare to these values, so also in favour of the >= 0 and <= 0 alternatives proposed earlier.

I pushed a PR in that direction. WDYT?

[Venorcis]

That aligns with my original proposal, so SGTM 😄

[Stephan202]

NB: Whatever we settle on: we can do the same for {Integer,Long}#signum. 😄

I filed #822 for this :)

[rickie]

I like the argument @Venorcis made here. But then you also came up with the multiplication argument that is strong.

Nice discussion 🚀 !

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

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

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

[Stephan202]

@rickie this PR changed significantly since you last looked at it; feel free to merge if you're happy with the latest changes.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

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