[numbers] Making fractions VALJOs

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[numbers] Making fractions VALJOs

Eric Barnhill
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?

Eric
Reply | Threaded
Open this post in threaded view
|

Re: [numbers] Making fractions VALJOs

Gilles Sadowski
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [numbers] Making fractions VALJOs

Eric Barnhill
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)

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 <[hidden email]> 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [numbers] Making fractions VALJOs

Gilles Sadowski
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 <[hidden email]>
> 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [numbers] Making fractions VALJOs

Eric Barnhill
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, 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 <[hidden email]> 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 <[hidden email]>
> > 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [numbers] Making fractions VALJOs

Gilles Sadowski
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 <[hidden email]>
> 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
>> <[hidden email]>
>> > 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [numbers] Making fractions VALJOs

jodastephen
As the author of the blog and term VALJO, here are some comments on Fraction:

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

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.

The method order in the class is strange. 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()`.

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
No doubt this would apply widely in the project

HTH
Stephen


On Thu, 18 Oct 2018 at 11:45, Gilles <[hidden email]> 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 <[hidden email]>
> > 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
> >> <[hidden email]>
> >> > 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [numbers] Making fractions VALJOs

Eric Barnhill
You are indeed and thank you very much for this feedback!

I gave up on pushing fraction through GitHub and have pushed my first round
of changes via Apache. They are available on the branch fraction-dev for
inspection. I will incorporate the rest of your suggestions on  my next
iteration.

Eric

On Sun, Oct 28, 2018 at 1:52 AM Stephen Colebourne <[hidden email]>
wrote:

> As the author of the blog and term VALJO, here are some comments on
> Fraction:
>
> 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).
>
> 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.
>
> The method order in the class is strange. 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()`.
>
> 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
> No doubt this would apply widely in the project
>
> HTH
> Stephen
>
>
> On Thu, 18 Oct 2018 at 11:45, Gilles <[hidden email]> 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 <[hidden email]>
> > > 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
> > >> <[hidden email]>
> > >> > 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: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [numbers] Making fractions VALJOs

Gilles Sadowski
In reply to this post by jodastephen
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 <[hidden email]>
> 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
>> <[hidden email]>
>> > 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
>> >> <[hidden email]>
>> >> > 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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [numbers] Making fractions VALJOs

Eric Barnhill
In reply to this post by jodastephen
On Sun, Oct 28, 2018 at 1:52 AM Stephen Colebourne <[hidden email]>
wrote:

>
> The order of the static constants is also strange - I'm sure a more
> logical order could be chosen.
>

I have rearranged the constants into the order 0, 1, 1/2, 1/3, 2/3, 1/4,
3/4 . I propose dropping the constants
for fifths and negative one. One needs to draw the line somewhere
reasonable. Any objections?
Reply | Threaded
Open this post in threaded view
|

Re: [numbers] Making fractions VALJOs

Eric Barnhill
Second question, is there any reason not to use syntactic sugar for
distinctions between "multiply" and "
multpliedBy"? With the former being the suger, if the latter is a better
standard.

It seems like both sides of the fence can be easily bridged here.

On Fri, Nov 2, 2018 at 4:57 PM Eric Barnhill <[hidden email]> wrote:

>
>
> On Sun, Oct 28, 2018 at 1:52 AM Stephen Colebourne <[hidden email]>
> wrote:
>
>>
>> The order of the static constants is also strange - I'm sure a more
>> logical order could be chosen.
>>
>
> I have rearranged the constants into the order 0, 1, 1/2, 1/3, 2/3, 1/4,
> 3/4 . I propose dropping the constants
> for fifths and negative one. One needs to draw the line somewhere
> reasonable. Any objections?
>
Reply | Threaded
Open this post in threaded view
|

Re: [numbers] Making fractions VALJOs

Gilles Sadowski
In reply to this post by Eric Barnhill
On Fri, 2 Nov 2018 16:57:43 -0700, Eric Barnhill wrote:

> On Sun, Oct 28, 2018 at 1:52 AM Stephen Colebourne
> <[hidden email]>
> wrote:
>
>>
>> The order of the static constants is also strange - I'm sure a more
>> logical order could be chosen.
>>
>
> I have rearranged the constants into the order 0, 1, 1/2, 1/3, 2/3,
> 1/4,
> 3/4 . I propose dropping the constants
> for fifths and negative one. One needs to draw the line somewhere
> reasonable. Any objections?

I think that only "zero" and "one" should be kept: The rest
has no particular use or meaning within the component; better
leave them to the application layer, and keep this API lean.

Did you have a look at the updated description:
  https://issues.apache.org/jira/browse/NUMBERS-74
?

Regards,
Gilles


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [numbers] Making fractions VALJOs

Gilles Sadowski
In reply to this post by Eric Barnhill
On Fri, 2 Nov 2018 17:00:19 -0700, Eric Barnhill wrote:
> Second question, is there any reason not to use syntactic sugar for
> distinctions between "multiply" and "
> multpliedBy"? With the former being the suger, if the latter is a
> better
> standard.

Statement needs some reference before we embark on changing names
for longer ones.  Cf. my previous post:
   https://markmail.org/message/r5hxy5f4eduxp2vn

[If the rationale is "correct English", fine, but then all the
modules must be modified accordingly.]

> It seems like both sides of the fence can be easily bridged here.

IMO, syntactic sugar should be justified by more than sparing
4 keystrokes, especially if we want to induce usage of proper
spelling.

Regards,
Gilles

> On Fri, Nov 2, 2018 at 4:57 PM Eric Barnhill <[hidden email]>
> wrote:
>
>>
>>
>> On Sun, Oct 28, 2018 at 1:52 AM Stephen Colebourne
>> <[hidden email]>
>> wrote:
>>
>>>
>>> The order of the static constants is also strange - I'm sure a more
>>> logical order could be chosen.
>>>
>>
>> I have rearranged the constants into the order 0, 1, 1/2, 1/3, 2/3,
>> 1/4,
>> 3/4 . I propose dropping the constants
>> for fifths and negative one. One needs to draw the line somewhere
>> reasonable. Any objections?
>>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]