Hi.
On Sun, 28 Oct 2018 08:52:01 +0000, Stephen Colebourne wrote:
As the author of the blog and term VALJO, here are some comments on
Fraction:
Thanks for the review and comments.
You should use `of()` (overloading allowed) when the factory normally
succeeds.
You should use `from` (overloading allowed) when the factory methods
are performing a conversion and have a reasonable chance of failure.
The `int` methods should use `of`. The `double` methods could use
either, it is a judgement call as top whether it is a conversion or a
construction (does it normally succeed or not).
I'm not convinced that it is useful to make a difference between
construction and conversion...
Looking at this code
https://git-wip-us.apache.org/repos/asf?p=commons-numbers.git;a=blob;f=commons-numbers-fraction/src/main/java/org/apache/commons/numbers/fraction/Fraction.java;h=308f93033853ca8815663c576f7c38e6770dc3c3;hb=HEAD
In the `abs()` method, there is no need for a local variable - just
return from each branch of the if statement or use a ternary.
+1
The method order in the class is strange.
It seems to be very old Commons Math code...
I would recommend putting
the getters first. I would also recommend grouping the methods
compareTo, equals, hashCode and toString in that order at the end of
the class. See `LocalDate` for example.
The order of the static constants is also strange - I'm sure a more
logical order could be chosen.
The method `getReducedFraction` is not a getter, so it should not
start with `get`. Maybe `ofReduced()` ? Alternatively, have an
instance method `reduced()` that can be called on any fraction, so
users do `of(2, 4).reduce()`.
At first sight, "reduce" should be private.
[I've added a note in JIRA that this class should be reviewed
before the release.]
The recommended naming approach for methods on immutable VALJO
classes
is to use the past tense:
multiply -> multipliedBy
divide -> dividedBy
add -> plus
subtract -> minus
negate -> negated
Where is this convention used/defined?
What is the rationale/advantage?
No doubt this would apply widely in the project
Sure, but we should you then raise the issue of adoption by *all*
Commons components.
Best,
Gilles
HTH
Stephen
On Thu, 18 Oct 2018 at 11:45, Gilles <gil...@harfang.homelinux.org>
wrote:
On Wed, 17 Oct 2018 16:33:58 -0700, Eric Barnhill wrote:
> Oh right, that is the convention. I knew there was something off.
>
> As far as you understand, is to within VALJO standards to overload
> factory
> methods,
I don't think that it is ValJO-related; method overload is a
feature, so better use it rather than duplicate what the compiler
can do by itself. ;-)
Gilles
> so long as they are not private constructors? All that is
> specified on the page is that VALJOs must have all constructors
> private. So
> I am not sure whether it is in the spirit of VALJOs to overload,
but
> coming
> up with elaborate names for each constructor doesn't seem like a
very
> streamlined coding practice.
>
> On Tue, Oct 16, 2018 at 5:56 PM Gilles
<gil...@harfang.homelinux.org>
> wrote:
>
>> On Tue, 16 Oct 2018 16:55:02 -0700, Eric Barnhill wrote:
>> > The Fraction class is IMO looking good (in better shape than
>> Complex
>> > was
>> > in) and is already quite close to fulfilling the standards for
a
>> > VALJO.
>> > Equals() and CompareTo() are well designed and consistent. I
see
>> two
>> > missing steps. The easy one is a parse() method which mirrors
the
>> > toString() method. The harder one is the wide range of public
>> > constructors.
>> >
>> > To be a VALJO all constructors must be private and accessed
with
>> > static
>> > factory methods. If these factory methods themselves can be
>> > overloaded, I
>> > think a decent schema emerges:
>> >
>> > current constructor -> proposed factory method
>> > --------------------------------------------------------
>> > public Fraction(double value) -> public fromDouble(double
value)
>> > public Fraction(double value, double epsilon, int
maxIterations)
>> ->
>> > public
>> > fromDouble(double value, double epsilon, int maxIterations)
>> > public Fraction(double value,int maxDenominator) -> public
>> > fromDouble
>> > (double value,int maxDenominator)
>> > public Fraction(int value) -> public fromInt(int value)
>> > public Fraction(int num, int denom) -> public fromInt(int num,
int
>> > denom)
>>
>> Why not call them all "of(...)" ?
>>
>> Gilles
>>
>> >
>> > so this is what I propose to go with.
>> >
>> > If disambiguation in the double cases is still a problem, the
>> second
>> > and
>> > third of the double constructors could be fromDoubleEpsMaxInt
and
>> > fromDoubleMaxDenom .
>> >
>> > Eric
>> >
>> >
>> > On Thu, Oct 11, 2018 at 7:00 AM Gilles
>> <gil...@harfang.homelinux.org>
>> > wrote:
>> >
>> >> On Wed, 10 Oct 2018 16:18:50 -0700, Eric Barnhill wrote:
>> >> > I am interested in moving forward on making the Fraction
>> classes
>> >> > VALJOs
>> >> > [NUMBERS-75].
>> >> >
>> >> > Just a preliminary question for now, are we otherwise happy
>> with
>> >> the
>> >> > Fraction class in the context of commons-numbers? Or should
I
>> look
>> >> > around
>> >> > for any odd behaviors leftover from commons-math (Complex
had a
>> >> lot
>> >> > of
>> >> > those) that might also be improved?
>> >>
>> >> AFAIK, there was no in-depth review as was done for "Complex".
>> >> So it would indeed be quite useful to check what the Javadoc
>> >> states, whether it seems acceptable (wrt what other libraries
>> >> do), and whether the unit tests validate everything.
>> >>
>> >> Side note: Unless I'm overlooking something, completing this
>> >> task will result in getting rid of all the formatting and
>> >> "Locale"-related classes (as for "Complex").
>> >>
>> >> Best,
>> >> Gilles
>> >>
>> >> >
>> >> > Eric
>> >>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
