[boost] Re: Submission: Rational numbers

From: Ed Brey (brey_at_[hidden])
Date: 1999-12-10 09:53:19

"Moore, Paul" wrote:
> 
> Here is the complete version of my rational numbers class. I believe that it
> now covers everything which has been discussed here, as well as including
> some documentation. The supplied "example.cpp" doubles as sample code and a
> simple test case.

First, one interface issue:

Should there be an assign function, similar to what std::string has. 
This would allow you to reassign a rational without creating a
temporary.  For example, in the read from stream operator, code would
change from
        r = rational<Int>(n, d);
to
        r.assign(n, d);

Seconds, several issues with the code:

1. Int lcm(Int n, Int m)
   {
      return (n * m) / gcd<Int>(n, m);
   }

The <Int> for gcd seems unnecessary, since it is deducible from n and
m.

2. The destructor defined in bad_rational is superfluous.  As an
example, std::domain_error defines no destructor.

3. Of the base classes of rational, only less_than_comparable is
public; all others are private.

4. "normalise" is misspelled, should be "normalize".

5. You comment that default assignment is fine, but say nothing about
default copy constructor being fine.  I'd recommend dropping the
commented out operator= line and changing the comment to "Default copy
constructor and assignment are fine".

6. For indenting, a combination of spaces and tabs are used, which
assumes a tab size of 8 spaces.  I would prefer using just spaces or
just tabs, as the current representation causes problems if the tab
size is set to other than 8.

7.  bool operator== (const rational& r) const
    {
        return ((num == r.num) && (den = r.den));
    }

= should be ==.

8.  const rational operator++(int)
    {
        rational was(*this);
        operator++();
        return was;
    }

Clever variable naming in the use of "was".  I really like it.
BTW, the postfix operators tend to always be boring code (except for
creative variable names) that ends up just making a temporary and
calling the prefix version.  Is there any way to add this to
operators.hpp?

9. abs, operator+/- (unary), and rational_cast take parameters by
value, where as all others take parameters by reference.  Why?

10. In noticed that abs ended up being a friend, even though the last
post I saw led me to believe that it wouldn't be.  As I mentioned
earlier, my understanding is that it should be a friend, so that it
can access private data, if it should need to.  To me, it seems that
it should have equal access rights as, say operator*= of any other
tightly coupled function.  I'd be interested in hearing reasons for
the belief that abs should not be a friend (discounting broken
compiler work-arounds).

11. As the comment for abs states, abs calls std::abs on the
denominator, even though it doesn't need to.  The compiler, will
rarely know that this call is unnecessary.  Rather than take the
performance hit, it would be better to take the std::abs off the
denominator, and move the comment about abs's assumption up to the
beginning of the document where someone who would change whether the
function is normalized will see it.  Even more secure, you could
create a normalized trait and do a compile-time check, although for a
class this small, it doesn't seem worth it; however, a normalized
trait may be useful for other reasons.

12. You include <cstdlib> twice.  Also, there is an blank line break
between inclusion of operators and config.