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
