commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Eric Barnhill <ericbarnh...@gmail.com>
Subject Re: [numbers] Making fractions VALJOs
Date Mon, 29 Oct 2018 20:05:17 GMT
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 <scolebourne@joda.org>
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 <gilles@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 <gilles@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
> > >> <gilles@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-unsubscribe@commons.apache.org
> > For additional commands, e-mail: dev-help@commons.apache.org
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message