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 <scolebou...@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 <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 > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > >
