On Fri, Sep 27, 2013 at 03:47:20PM +0200, Joseph Rushton Wakeling wrote: [...] > I've made a fork <https://github.com/WebDrake/Rational> and tweaked > the code to be in line with current D/Phobos style (David Simcha's > last update is from over 2 years ago so it's out of date in that > respect). I did this manually, line by line, so I could have a > proper read-through of the code. > > On the basis of that read-through it looks pretty comprehensive and > well-thought-out. There are various workarounds for bugs which > probably need to be re-examined depending on whether those bugs have > been fixed or not. In any case it compiles fine with current DMD, > and the unittests are thorough and have 94% code coverage -- good > job, David! > > One thing I don't like: David uses "num" and "denom" for the public > methods to get numerator and denominator.
For the record, I'm OK with it. I don't like "numerator" and "denominator" as it's needlessly verbose. It's already clear what "num" and "denom" mean in a rational number library, no need to spell it all out. But, others may have a different opinion on this. As far as the review is concerned, I don't mind either way. [...] > Anyway, if anyone else wants to take a look through and give me any > comments, they'd be welcome. I'll see if I can take a look at it sometime -- no guarantees, though, as I'm busy with other things these days. > (Bearophile, I will add your requested Fraction, but I'd like to get > more info on what everyone else thinks of the code as it is before > making any additions:-) [...] Honestly, I don't think that's necessary. It's just a one-line alias, trivial to add in user code. For Phobos code, I think writing Rational!BigInt is better, to make it clear what underlying number type is being used to instantiate the template. T -- One Word to write them all, One Access to find them, One Excel to count them all, And thus to Windows bind them. -- Mike Champion
