[jira] [Comment Edited] (NUMBERS-123) "BigFraction(double)" is unnecessary

["Heinrich Bohne (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/NUMBERS-123?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16876910#comment-16876910 ] 

Heinrich Bohne edited comment on NUMBERS-123 at 7/2/19 9:32 PM:
----------------------------------------------------------------

bq. The sole purpose of the flag is efficiency

Actually, I wasn't concerned about efficiency at all when I raised this issue. I found the design itself a bit awkward, because I think code should not do more than it needs to do; efficiency is only a by-product of this. And because I think that it is indeed the responsibility of the constructor (whether it is private or public) to validate the values to which it initializes the fields, I don't think the flag-approach is a satisfactory solution (of course, one could include a warning in the documentation of the constructor that it must only be called with valid arguments, but then, I really don't see how this would be better than having three separate constructors).

bq. this discussion also is a hint that the initial setup was not clear

Indeed, it was not obvious that the three constructors all reduced the fraction to lowest terms. A comment would probably have been in order. Although I don't see what this has to do with the awkward initialization of the ZERO and ONE constants. As far as I can tell, this is simply a matter of which method/constructor to call – converting the constructors to factory methods should not make a difference in this regard.

In the end, I think it boils down to the question of whether one is willing to entrust the code that converts a double value to a fraction with the responsibility of ensuring that the calculated numerator and denominator fulfill the conditions required by the fields' contracts (i.e. lowest terms, sign in numerator etc.), which is not unreasonable, since the two double constructors/methods accomplish this without code duplication. If yes, then the code would be better of in constructors, like in the original design. If not, then the current design seems the most adequate.


was (Author: schamschi):
bq. The sole purpose of the flag is efficiency

Actually, I wasn't concerned about efficiency at all when I raised this issue. I found the design itself a bit awkward, because I think code should not do more than it needs to do; efficiency is only a by-product of this. And because I think that it is indeed the responsibility of the constructor (whether it is private or public) to validate the values to which it initializes the fields, I don't think the flag-approach is a satisfactory solution (of course, one could include a warning in the documentation of the constructor that it must only be called with valid arguments, but then, I really don't see how this would be better than having three separate constructors).

bq. this discussion also is a hint that the initial setup was not clear

Indeed, it was not obvious that the three constructors all reduced the fraction to lowest terms. A comment would probably have been in order. Although I don't see what this has to do with the awkward initialization of the ZERO and ONE constants. As far as I can tell, this is simply a matter of which method/constructor to call – converting the constructors to factory methods should make a difference there.

In the end, I think it boils down to the question of whether one is willing to entrust the code that converts a double value to a fraction with the responsibility of ensuring that the calculated numerator and denominator fulfill the conditions required by the fields' contracts (i.e. lowest terms, sign in numerator etc.), which is not unreasonable, since the two double constructors/methods accomplish this without code duplication. If yes, then the code would be better of in constructors, like in the original design. If not, then the current design seems the most adequate.

> "BigFraction(double)" is unnecessary
> ------------------------------------
>
>                 Key: NUMBERS-123
>                 URL: https://issues.apache.org/jira/browse/NUMBERS-123
>             Project: Commons Numbers
>          Issue Type: Improvement
>          Components: fraction
>            Reporter: Gilles
>            Assignee: Gilles
>            Priority: Trivial
>             Fix For: 1.0
>
>         Attachments: NUMBERS-123__Javadoc.patch
>
>          Time Spent: 0.5h
>  Remaining Estimate: 0h
>
> Constructor {{BigFraction(double value)}} is only called from the {{from(double value)}} method.
>  Actually, this constructor is misleading as it is indeed primarily a conversion from which appropriate {{numerator}} and {{denominator}} fields are computed; those could be set by
>  the "direct" constructor {{BigFraction(BigInteger num, BigInteger den)}}.
> Moreover, the private field {{ZERO}} goes through this conversion code whereas it could constructed "directly", e.g. using {{of(0)}}. Similarly for field {{ONE}}.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)