JavaMoney / jsr354-api

JSR 354 - Money and Currency API
http://javamoney.org
Apache License 2.0
357 stars 79 forks source link

Clarify MonetaryAmount#plus #99

Closed marschall closed 4 years ago

marschall commented 6 years ago

For all the implementations in the RI MonetaryAmount#plus seems to be a no-op but the comment is confusing for several reasons:

We find the documentation of [BigDecimal#plus()](https://docs.oracle.com/javase/10/docs/api/java/math/BigDecimal.html#plus()) much clearer.

keilw commented 6 years ago

@marschall I sent you an invitation to groups.io, did you get it? Since you are quite active with issues or PRs here, I guess it could be good to also follow the mailing list.

marschall commented 6 years ago

Yes, I just got it and subscribed.

keilw commented 4 years ago

Applied a clarification from BigDecimal, that'll have to do.

marschall commented 4 years ago

Now I find the documentation contradictory. On one hand it says:

This method, which simply returns this {@code MonetaryAmount} is included for symmetry with the unary minus method {@link #negate()}.

on the other hand it says

@throws ArithmeticException if rounding fails.

How can this happen if it is just this {@code MonetaryAmount}?

@return {@code this}, rounded as necessary. A zero result will have a scale of 0.

The Javadoc now in place says this {@code MonetaryAmount} but then talks about rounding and changing the scale.

and

with rounding according to the context settings.

So rounded or just this {@code MonetaryAmount}?

keilw commented 4 years ago

It says "this rounded as necessary", it depends on the concrete implementation, if rounding is necessary or not. This was always a synchronization with BigDecimal, we can revisit it in a future version, for the MR there are more important issues like the TCK.

keilw commented 4 years ago

There have been a few minor adjustments, especially one line that mostly belongs to negate() or another method moved there. That should do for the MR.

marschall commented 4 years ago

So when is rounding necessary when the comment just says this {@code MonetaryAmount} like BigDecimal? BigDecimal does not round in #plus() only in #plus(MathContext) and only talks about the necessity for rounding in #plus(MathContext). I don't fee that synchronization with BigDecimal has been achieved.

What about the comment regarding changing the scale to 0?

keilw commented 4 years ago

That's been there ever since, also in BigDecimal, but as there is no scale directly on MonetaryAmount adjusting where possible to the context.

marschall commented 4 years ago

BigDecimal#plus() does not say anything about scale 0. Only BigDecimal#plus(MatchContext) mentions scale 0. BigDecimal#plus() uses the same MathContext therefore there is no need to. If we assume MonetaryContext is analog of MathContext then #plus() should use the same MonetaryContext and therefore not rounding or adjustment of the scale should be necessary.

Could it be that we copied from the wrong BigDecimal method?

What does it matter since how long the method exists. Its contract should be clearly specified.

keilw commented 4 years ago

No it was not the wrong method, but again MonetaryAmount closely reassembles BigDecimal but it is not an exact clone for good reasons, therefore e.g. a simple scale() method does not exist. The current JavaDoc sorts that out and it has to do for the MR. We're not supposed to make any substantial changes to the API and there is nothing for 1.1, everything else has to wait till we get a chance to file a new JSR.