A Fraction class in C++

Question

I am trying to learn clear and concise C++. Currently I'm learning classes in school, and to practice I have made a Fraction class.

I'd appreciate any help regarding style, cleanliness or anything else that would be good to know.

(I know using namespace std; is frowned upon, but it's required in our course.)

#include <iostream>
using namespace std;

class Fraction {
    int numerator, denominator;
private:
    int gcd(int a, int b) {
        while (a != b) {
            if (a > b)
                a -= b;
            else
                b -= a;
        }
        return a;
    }
public:
    Fraction (int, int);
    double decimal();
    string fraction();
    Fraction add(Fraction);
    Fraction subtract(Fraction);
    Fraction multiply(Fraction);
    Fraction divide(Fraction);
};

int main() {
    Fraction f = {2,3};
    cout << f.decimal() << endl;
    Fraction k = {1,4};
    Fraction d = {1,3};
    Fraction result = k.add(d).add(f);
    cout << result.fraction() << endl;
    return 0;
}

Fraction::Fraction(int n, int d) {
    numerator = n / gcd(n, d);
    denominator = d / gcd(n, d);

}
string Fraction::fraction() {
    return to_string(numerator) + "/" + to_string(denominator);
}
double Fraction::decimal() {
    return (double)numerator / (double)denominator;
}

Fraction Fraction::add(Fraction f) {
    return Fraction(numerator * f.denominator + f.numerator * denominator, denominator * f.denominator);
}
Fraction Fraction::subtract(Fraction f) {
    return Fraction(numerator * f.denominator - f.numerator * denominator, denominator * f.denominator);
}
Fraction Fraction::multiply(Fraction f) {
    return Fraction(numerator * f.numerator, denominator * f.denominator);
}
Fraction Fraction::divide(Fraction f) {
    return Fraction(numerator * f.denominator, denominator * f.numerator);
}

Answer 1

You wished for some style and cleanliness advice, so here are a few things you may improve in that area:

1. Don't include gcd in you class because it is actually completely unrelated. Instead, write a separate non-class function for it.

2. Name your parameters. Things such as Fraction add(Fraction); usually imply that the parameter is ignored, and even if the user understands that this is not the case here, it remains unclear what your parameter actually represents.

3. Include all the headers that you use. Your code is currently missing a #include <string>.

4. Do some error checking and assure that parameters are valid. For example, what happens when somebody passes 0 for d to the constructor? You will end up in an infinite loop because your gcd function does not verify that its arguments are not 0.

5. Avoid C-style casts. C++ offers a variety of casts with clearly defined purposes, whereas the C cast covers a lot of different conversions, some of which you may not want. In your particular case, don't write (double)numerator; write static_cast<double>(numerator) instead (same for denominator, of course).

6. Instead of having add, subtract, multiply and divide, you should overload the corresponding operators (e.g. operator+ for add). You should also consider adding more operators, such as operator+=, operator-= and so on and so forth.

7. Don't use std::endl where a simple '\n' would suffice (which is almost always). The reason for this is that std::endl also flushes the underlying buffer which can seriously harm performance when doing lots of IO and is seldom required.

8. fraction is a bad name for a function that returns a string representation of your class. You could call it to_string for example, or even choose to implement explicit operator std::string().

9. You can safely omit return 0; at the end of main, the compiler is nice enough to add it automatically.

10. Currently, your constructor calculates gcd(n, d) twice. You should calculate the value once and then divide set numerator = n / gcd_value and denominator = d / gcd_value instead.

11. You should consider adding adding a default value of 1 to the second parameter to the constructor to enable construction of fractions from whole numbers easily, i.e.

    Fraction::Fraction(int n, int d = 1);
    ...
    Fraction f {3}; //f represents the fraction 3/1
    

12. Don't treat single characters as strings when you don't need to. In your case, replace "/" by '/' in fraction. There is an operator+ in std::string that handles single chars.

13. Fix the unnecessary whitespace in Fraction (int, int). Judging by how you defined your other methods, it should instead read Fraction(int, int) (I suppose this is just a typo).

---

Edit: As JPhi1618 pointed out in the comments, it is better to not get into the habit of relying on implicit construction when assigning values to objects. Instead, one should make use of explicit construction and uniform initialization. Accordingly I changed last line of the code sample of point 11 from Fraction f = 3; to its current representation.

Answer 2

Great answer by Ben Staffen, just wanted to add:

1. Prefer const references when passing complicated types. It might not be particularly relevant for this class, but as types become more complicated/more expensive to construct, const-refs make a huge performance impact.
2. Using class makes members private by default, so the first private specifier you have is unnecessary.

Answer 3

Ben Steffan suggests replacing add with operator+ and adding operator+=. That is a good idea, but it's wise to follow a specific pattern to do so: Implement operator+= as Fraction& Fraction::operator+=(Fraction), i.e. a member function, and implement operator+ as a free function

inline Fraction operator+(Fraction lhs, Fraction rhs) {
   lhs += rhs;
   return lhs;
}

Since this relies on the public operator+= it does not need to be a friend.

[Edit] And to expand on that, mvds is correct to wonder about decimal. It's usually spelled operator double. You'll want to make that explicit so you don't get implicit conversions to double. E.g. without explicit, std::cout << Fraction{4} would print 4.0000 because it uses the implicit conversion..

Answer 4

Don't be afraid to introduce local variables when doing calculations. For instance, your add and subtract functions have some pretty long lines. It would be clearer to write them as:

Fraction Fraction::add(Fraction f) {
    int addNumerator = numerator * f.denominator + f.numerator * denominator;
    int addDenomenator = denominator * f.denominator;
    return Fraction(addNumerator, addDenomenator);
}

This does two things. It gives the intermediate values meaningful names so people can follow the calculations easier. When you start to debug programs and step through them line by line, the parts of the calculation are now defined individually so they can be easily inspected. Any modern compiler is going to be smart enough to remove the usage of the local variables once compiled with optimizations.

You have a simple calculation and a short class, so this may seem excessive, but in a more complicated situation it can help quite a bit.

Answer 5

In addition to the other answers,

Use the const keyword to enforce immutability. This can be used on the signatures of functions that don't change the object state eg. double decimal() const; The preferred way of passing objects (that won't be altered by the function) in c++ is const reference eg. Fraction add(const Fraction &f). This is cheaper than passing by value as objects don't need to be copy constructed.

Only const functions can be called on const objects. So by declaring a function const in can then be called in the body of a function like void print(const Fraction &f);

The current implementation of your class is immutable, none of the member functions alter the contents in any way. If this is intended then make numerator and denominator const.

In order to enforce immutability you create a new object on each method call. Unfortunately, object creation isn't free. Instead, I would let the caller decide whether they want a new object or whether to modify the existing one.

Currently simplification of the fraction is handled by the constructor. If the object is immutable this is fine. However, I would create a simplify() method which performs this simplification instead. This allows the logic to be reused in multiple places. The constructor could then become

Fraction(int n, int d) : numerator(n), denominator(d) { simplify(); }

As others have mentioned the c++ way of doing things is to overload operators rather than writing functions such as add(). Define operators +=, -=, *= and /= as member functions and then define non member +, -, * and / in terms of those.

Fraction& operator+=(const Fraction &f)
{
    numerator += f.numerator * denominator;
    denominator += f.denominator * nominator;
    simplify();
    return *this;
}

Fraction operator+(Fraction l, const Fraction &r)
{
    l += r;
    return l;
}

l is copy constructed as it's passed by value leaving the passed object unaltered. The function does not change r so it is also const.