Rational Type - Feedback appreciated

[bugbyte]

Hi there,

I wrote a Rational class, which allows calculating with Rationals/Fractions.
I have the code on github: https://github.com/Raincode/Rational

If you happen to feel like skimming it, and you notice anything that could be improved or even find a bug, please let me know :)

I don't necessarily recommend using it in a serious project, since it probably still has some bugs.

I am posting this here, because I have yet much to learn in C++, and I want to see how much of the stuff I have read and learnt I am actually capable of using and of course improve my programming skills.

Kind Regards
bugbyte

[ne555]

    void inverse() { std::swap(num, denom); }
    /**< Turns Rational into its reciprocal */

    Rational inverse() const { return Rational(denom, num); }
    /**< Returns the reciprocal of a Rational */

that's confusing.
you could not do bar = foo.inverse(); if `foo' were not constant
also, the final state would be dependant on the type, making the code hard to follow.

I would recommend to conserve only the const version. If you think that foo = foo.inverse(); is quite a common operation, then provide a `invert()' method.

    friend double to_double(const Rational&);
    /**< Converts a Rational to double */

prefer member functions to non-member friends.
(but prefer non-member non-friend to member)

    static Rational make_exact(double);
    /**< Returns the exact representation of a double as Rational */

exact is quite a big word.


Given that you provide a `safe_add()' operation, I was expecting that += would not be "safe"

[bugbyte]

Hey
@ne555 thanks for your reply. Until I am more sure about the usage, I will rename the inverse() function, which actually inverts the Rational to 'invert'. I'll keep in mind removing it, if it deems itself not very useful.

prefer member functions to non-member friends.
(but prefer non-member non-friend to member)

I am afraid I wasn't quite able to grasp what you mean (my english is kinda rusty, sorry).
I believe it means, that my design choice making to_double a friend function is not optimal?
I was thinking about declaring it like so:
static double to_double(const Rational&)
Would that be the preferred choice? Or did you mean something different? It's just, if I make it non-friend non-member, I will habe to provide facilities to access num and denom. I could just make them public, but I am not sure that is the right way to go.

I added the whole "safe_..." and "..._is_safe", because I want my Rational class to detect overflows (or do I? Think so...). Even if it is something that is unlikely to occur, especially after I changed the double to Rational constructor to use an approximation, I think if it ever happened it shouldn't go unnoticed.
If I understand correctly, are you suggesting I made safe_add, ... private? Would make sense, since they are meant for implementation only. Or, I could leave the regular '+=' "unsafe", making it more efficient, and keep the safe_add public, so one can use it, if they fear of overflows...

exact is quite a big word.

indeed. I just looked into that, and it turns out something is flawed with my implementation. Feeding 123.123456789 gives a numerator of 123123457, which is not how I imagined it to be, but (123123456789 / 1000000000) which would be rather exact (if I feed it to python it spits out 123.123456789), however I believe maybe the limitation of using integers as numerator and denominator puts an end to that. Or maybe something gets lost somewhere during all those conversions...

I am really considering removing that...

After doing more checks, I believe I must overlook my approximation method, too. It does not work properly for small tolerances.

Anyhow, thank you very much for looking into my code, I really appreciate it.

[ne555]

here's the rationale http://www.gotw.ca/gotw/084.htm

you have friend double to_double(const Rational&); and you make it friend because you need access to the internals (`denum', `num'), ¿why don't make it a member function then?

You propose then static double to_double(const Rational&), that is a class member function.
But you are also asking for an object as a parameter ¿why make it static then?.
Consider the use.

Rational foo;
Rational::to_double( foo ); //as an class (static) member function
foo.to_double(); //as an object (non-static) member function 

> if I make it non-friend non-member, I will habe to provide facilities to access num and denom.
Please no. Your `Rational' class is the only one that understand the meaning of `num' and `denom' and how should they be handled (like that denom cannot be 0)

The idea of non-frien non-member function (free functions) is that you can add functionality to the class without having to have any knowledge of the internals. You would only use the interface of the class, and if the internals ever change, your function should still work without modification.


> Or, I could leave the regular '+=' "unsafe", making it more efficient,
> and keep the safe_add public, so one can use it, if they fear of overflows...
yes, that's what I mean. Like in the case of std::vector that provides operator[] to access an element, and .at() that would also check for index validity.


> Or maybe something gets lost somewhere during all those conversions...
I did not find how to adjust the precision of to_string, you may use http://www.cplusplus.com/reference/iomanip/setprecision/ with an stringstream

[bugbyte]

Hey,
once again thanks for replying @ne555:
I will now make the to_double function like so:
double to_double() const;
That is, its a member function which returns the value of a Rational as double. I hope this is what you where thinking about. If not, I am really embarassed, because I can't understand what someone is trying to explain for the 2. time :D

I will implement safe_sub and safe_div, and then change everything as proposed (it makes sense to me, integers and std::complex dont throw an exception either if you make them overflow)

The whole double_to_Rational thing is still currently a little problem, but I am working on various methods and testing them. The current method is pretty broken...^^

I have now decided to remove 'make_exact' (which btw was my naive original method for double-to-Rational).

Thanks very much. I am of course open to further suggestions anytime.

I will read the link you posted in 3, 2, 1, ...
Ok, so after reading the article, I feel I should just add

int getNum() const;
int getDenom() const;

Or something similar, and implement all those functions (IOoperator, safe_xxx, to_double, ...) as non-member non-friend functions?

[ne555]

Please no. Your `Rational' class is the only one that understand the meaning of `num' and `denom' and how should they be handled (like that denom cannot be 0)

http://www.javaworld.com/article/2073723/core-java/why-getter-and-setter-methods-are-evil.html


When creating the interface think on the operations that you want to do. Like arithmetic operations.
Because those operations need to access the internals of the class, they need to be member functions.

[bugbyte]

@ne555
Read the article. Thanks again for a helpful link. I am now 101% certain I don't want getNum() and getDenom()!

[JLBorges]

> I am now 101% certain I don't want getNum() and getDenom()!

Are you?
What about the open-closed principle? https://en.wikipedia.org/wiki/Open/closed_principle

For instance:
I have written a program that uses your Rational class.
In the next version of my program, I want to use a library which uses boost::rational in its interface
I can't afford the inaccuracies that would creep in if I use Rational => to_double() => double => boost::rational.

Or I need to convert from Rational to a fixed precision type with 100 decimal digits precision; say boost::multiprecision::cpp_dec_float_100

What do you expect me to do?
Write your Rational to a std::ostringstream, extract a copy of the std::string from it, and then parse the string to get the numerator and denominator?

Would providing int numerator() const ; and int denominator() const ; violate encapsulation any more than std::ostream& operator<< ( std::ostream&, const Rational& ) ;

Isn't good design, over and above everything else, pragmatic design?

[bugbyte]

@JLBorges
Well, the reason I would and even had provided numerator() and denominator(), ist to completely eliminate a few friend functions. Every design choice as a good and bad side.

The other article shows the downside in the first few paragraphs under "data abstraction".
It also shows, why get... functions would violate encapsulation more than the output operator (in practice at least).

I think you wouldn't want to mix Rational and boost::rational in a program, why not just stick to one or the other?

Would to_double() -> cpp_dec_float_100 also be too inaccurate? Wouldn't the result of to_double() not be accurate by 100 decimals anyway?

edit: I wonder though, say I want to represent a Rational graphically, e.g. render it with a Text Object... I would have no way of finding the actual numerator or denominator...
Would it make sense to return a pair<int, int> ? I guess that doesn't really help encapsulation either.

Tricky thing... boost::rational supplies the access to num and denom. But I don't know, if boost is known for having superior Design.

Additionally, what if someone wanted to implement an operation or some formula besides +-*/ etc. ?

[JLBorges]

> I think you wouldn't want to mix Rational and boost::rational in a program, why not just stick to one or the other?

Even if I don't use anything other than Rational, there would be be a lot of other people and libraries using boost::rational. I definitely want the freedom to easily use libraries written by other people in my code.

Just kidding. I would use boost::rational as well. Its design has been refined through peer reviews by more than one expert programmer. And it does not paint itself into a corner; it has the two member functions numerator() and denominator(); "These functions allow user code to implement any additional required functionality."


> But I don't know, if boost is known for having superior Design.

From 'Boost Background Information' http://www.boost.org/users/index.html

What do others say about Boost?

"...one of the most highly regarded and expertly designed C++ library projects in the world."
— Herb Sutter and Andrei Alexandrescu, C++ Coding Standards

"Item 55: Familiarize yourself with Boost." — Scott Meyers, Effective C++, 3rd Ed.

"The obvious solution for most programmers is to use a library that provides an elegant and efficient platform independent to needed services. Examples are BOOST..."
— Bjarne Stroustrup, Abstraction, libraries, and efficiency in C++

> Would to_double() -> cpp_dec_float_100 also be too inaccurate?
> Wouldn't the result of to_double() not be accurate by 100 decimals anyway?

No. About 15 decimal digits of precision is typical.
Check it out on your implementation with: std::numeric_limits<double>::digits10
http://en.cppreference.com/w/cpp/types/numeric_limits/digits10

[bugbyte]

All right, after thinking about this, and also asking on another forum, I will probably provide the access functions. Thank you to all for your help with my project and advice, I appreciate it very much. Of course I am always open to further suggestions.