On 27/09/13 12:37, Dicebot wrote:
As usual, volunteering to act as review manager. Once you are ready to
championship this module, add it to review queue in wiki and ping me (preferably
via e-mail)
Thanks very much! :-)
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. I think it should be "numerator" and
"denominator". This matches Boost.Rational but I don't know if David was naming
these methods by analogy to some other library/language. In any case I should
probably make a detailed comparison to Boost.Rational; I don't know if this was
David's model or whether this is a clean-room implementation.
There are also probably some Ddoc tweaks that need to be made, not everything
seems to be perfectly in order on my system.
Anyway, if anyone else wants to take a look through and give me any comments,
they'd be welcome. (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:-)
Best wishes,
-- Joe
