A minimal Fraction class in C++

Question

I am trying to learn idiomatic Modern C++. I was wondering how you would improve upon this Fraction class. I would really appreciate any help with code organization, cleanliness and anything else that I should keep in mind.

Header:

#ifndef FRACTION_H
#define FRACTION_H

#include <iostream>

class Fraction {
    int numerator{}, denominator{};
public:

    Fraction() = default;
    Fraction(int n, int d):
        numerator(n), denominator(d) {}
    ~Fraction() = default;

    void set_num(int value) { numerator = value; }
    void set_den(int value) { denominator = value; }
    int get_num() const { return numerator; }
    int get_den() const { return denominator; }

    void reduce();
    int calculate_gcd(int, int) const;
    //void display() {}

};

std::istream &operator>>(std::istream &is, Fraction &value);
std::ostream &operator<<(std::ostream &os, const Fraction &value);

Fraction operator+(const Fraction &lhs, const Fraction &rhs);
Fraction operator-(const Fraction &lhs, const Fraction &rhs);
Fraction operator*(const Fraction &lhs, const Fraction &rhs);
Fraction operator/(const Fraction &lhs, const Fraction &rhs);

bool operator> (const Fraction &lhs, const Fraction &rhs);
bool operator< (const Fraction &lhs, const Fraction &rhs);
bool operator==(const Fraction &lhs, const Fraction &rhs);
bool operator!=(const Fraction &lhs, const Fraction &rhs);
bool operator>=(const Fraction &lhs, const Fraction &rhs);
bool operator<=(const Fraction &lhs, const Fraction &rhs);


#endif

Class file:

#include <cmath>
#include "Fraction.h"

void Fraction::reduce() {
    int gcd = calculate_gcd(numerator, denominator);

    numerator = numerator / gcd;
    denominator = denominator / gcd;
}

int Fraction::calculate_gcd(int a, int b) const {
    int temp{};

    a = std::abs(a);
    b = std::abs(b);

    if (b > a) {
        temp = a;
        a = b;
        b = temp;
    }

    while(b != 0) {
        temp = b;
        b = a % b;
        a = temp;
    }

    return a;
}

std::istream &operator>>(std::istream &is, Fraction &value) {
    int numerator{}, denominator{};
    is >> numerator >> denominator;

    value.set_num(numerator);
    value.set_den(denominator);

    if (!is) {
        numerator = 0;
        denominator = 0;
    }
    return is;
}

std::ostream &operator<<(std::ostream &os, const Fraction &value) {
    os << value.get_num() << "/" << value.get_den();
    return os;
}

Fraction operator+(const Fraction &lhs, const Fraction &rhs) {
    Fraction result{};

    result.set_num((lhs.get_num() * rhs.get_den()) + (lhs.get_den() * rhs.get_num()));
    result.set_den(lhs.get_den() * rhs.get_den());

    result.reduce();

    return result;
}

Fraction operator-(const Fraction &lhs, const Fraction &rhs){
    Fraction result{};

    result.set_num((lhs.get_num() * rhs.get_den()) - (lhs.get_den() * rhs.get_num()));
    result.set_den(lhs.get_den() * rhs.get_den());

    result.reduce();

    return result;
}

Fraction operator*(const Fraction &lhs, const Fraction &rhs){
    Fraction result{};

    result.set_num(lhs.get_num() * rhs.get_num());
    result.set_den(lhs.get_den() * rhs.get_den());

    result.reduce();

    return result;
}

Fraction operator/(const Fraction &lhs, const Fraction &rhs){
    Fraction result{};

    result.set_num(lhs.get_num() * rhs.get_den());
    result.set_den(lhs.get_den() * rhs.get_num());

    result.reduce();

    return result;
}

bool operator> (const Fraction &lhs, const Fraction &rhs) {
    return (((lhs.get_num() * rhs.get_den()) - (rhs.get_num() * lhs.get_den())) > 0);
}

bool operator< (const Fraction &lhs, const Fraction &rhs) {
    return (((lhs.get_num() * rhs.get_den()) - (rhs.get_num() * lhs.get_den())) < 0);
}

bool operator==(const Fraction &lhs, const Fraction &rhs) {
    return (((lhs.get_num() * rhs.get_den()) - (rhs.get_num() * lhs.get_den())) == 0);
}

bool operator!=(const Fraction &lhs, const Fraction &rhs) {
    return !(lhs == rhs);
}

bool operator>=(const Fraction &lhs, const Fraction &rhs) {
    return (lhs > rhs) or (lhs == rhs);
}

bool operator<=(const Fraction &lhs, const Fraction &rhs) {
    return (lhs < rhs) or (lhs == rhs);
}

main file:

#include <iostream>
#include "Fraction.h"

int main() {

    Fraction a{1,2}, b{3,4};

    std::cout << a << " + " << b << " = " << a + b << "\n";
    std::cout << a << " - " << b << " = " << a - b << "\n";
    std::cout << a << " * " << b << " = " << a * b << "\n";
    std::cout << a << " / " << b << " = " << a / b << "\n";

    std::cout << a << " == " << b << " = " << (a == b) << "\n";
    std::cout << a << " < " << b << " = " << (a < b) << "\n";
    std::cout << a << " > " << b << " = " << (a > b) << "\n";

    std::cout << a << " != " << b << " = " << (a != b) << "\n";
    std::cout << a << " <= " << b << " = " << (a <= b) << "\n";
    std::cout << a << " >= " << b << " = " << (a >= b) << "\n";

    Fraction c(a);
    std::cout << "c: " << c << "\n";

    Fraction d{};

    std::cout << "d: " << d << "\n";
    d = b;
    std::cout << "d: " << d << "\n";

    return 0;
}

makefile:

CXX=g++
CXXFLAGS = -c -std=c++11 -Wall -Wpedantic

fraction : main.o Fraction.o
    $(CXX) -o fraction main.o Fraction.o

main.o : main.cpp
    $(CXX) $(CXXFLAGS) main.cpp

Fraction.o : Fraction.cpp Fraction.h
    $(CXX) $(CXXFLAGS) Fraction.cpp Fraction.h

clean:
    rm fraction main.o Fraction.o Fraction.h.gch

Answer 1

Don't include <iostream> just to declare types

Your header can be leaner if it replaces the big include with

#include <iosfwd>

which is provided for exactly this purpose.

You will then need to include <iostream> in your implementation file, of course.

Think about default values and implicit conversions

Look at this:

    int numerator{}, denominator{};
public:
    Fraction() = default;

A default-constructed Fraction will be 0/0, which isn't very useful. A better version has

    int numerator{0};
    int denominator{1};

Now, the default is 0/1, which conforms to the guidance to "act like the integers".

ALso,

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

It can be helpful to allow implicit conversions from integers (e.g. so that you can perform mixed Fraction and int arithmetic: 2 + Fraction(1,4)). The two constructors can be replaced by a single one with defaults:

    Fraction(int n = 0, int d = 1):
        numerator(n), denominator(d)
    {
        if (!d) throw std::out_of_range("d");
    }

You don't need to declare the default destructor

Just omit this, and let the compiler do the right thing unmolested

    ~Fraction() = default;

The only time you need to write = default for a constructor is when you're writing a class that's designed to be inherited from (in which case, you'd declare it virtual).

A bug in operator<<()

If streaming into the temporaries fails, we still write to value:

std::istream &operator>>(std::istream &is, Fraction &value) {
    int numerator{}, denominator{};
    is >> numerator >> denominator;

    value.set_num(numerator);
    value.set_den(denominator);

    if (!is) {
        # These assignments are useless
        numerator = 0;
        denominator = 0;
    }
    return is;
}

To be more consistent with standard library, we should leave value untouched if the read fails:

std::istream &operator>>(std::istream &is, Fraction &value) {
    int numerator, denominator;
    if (is >> numerator >> denominator) {
        value = {numerator, denominator};
    }
    return is;
}

Add constexpr where appropriate

The arithmetic and comparison operators are pure functions of their arguments, so you can decorate them with constexpr to allow the compiler to shift more work away from run time.

Use Make's built-in rules and variables

Make knows how to create object files, given source files, and how to link the object files to make a binary:

CXX = g++
CXXFLAGS = -std=c++11 -Wall -Wextra -Weffc++ -Wpedantic

fraction: main.o Fraction.o
    $(LINK.cpp) $(OUTPUT_OPTION) $^

main.o: main.cpp Fraction.h
Fraction.o: Fraction.cpp Fraction.h

clean:
    $(RM) fraction *.o

This differs from the original in these ways:

• I removed -c from CXXFLAGS (that's part of the built-in COMPILE.cpp rule)
• I omitted the commands for the object files, and let Make provide sensible built-in ones
• I added some extra warnings you might like
• I used $(LINK.cpp) to link - this is useful when you want to add libraries (you can just add to LDFLAGS and LDLIBS as required)
• I added the missing dependency of main.o on Fraction.h
• I'm using the standard RM command (which includes -f so it doesn't report failure if the files don't exist)

(I didn't rename Fraction.* to fraction.*, but I recommend that you do: inconsistent capitalization of file names can be frustrating, particularly in locales such as the standard C locale, where it affects sorting. Just use lower-case everywhere for your source files and save yourself much annoyance!)

---

Future ideas

I know this is intended to be a simple class, but if you want to take it further, consider these extensions:

• Make it a template parameterised on the numeric type (instead of always int).
• Provide a user-defined literal for user convenience.
• Provide an explicit narrowing conversion to int (or to the numerator type if you've moved to a template).
• Replace the streaming operators with templated ones that will work on any kind of std::basic_istream<Char,Traits> and std::basic_ostream<Char,Traits>.

---

Further reading

Be sure to look through questions tagged [rational numbers] [c++].

In particular, one that I've previously reviewed is Implementation of a Rational Number class in C++.

Answer 2

• Unrestricted public getters and setters defeat the privacy of numerator and denominator. I recommend dropping getters and setters altogether, and let the class methods access the data fields directly.

• The constructor should call reduce. This way the fraction is always kept in the reduced form, and the operator== can be simplified to

  bool operator==(const Fraction &lhs, const Fraction &rhs) {
      return (lhs.numerator == rhs.numerator) && (lhs.denominator == rhs.denominator);
  }
  

• It is recommended to implement operator> in terms of operator<:

  bool operator> (const Fraction &lhs, const Fraction &rhs) {
      return rhs < lhs;
  }
  

  Notice that it is possible (and sometimes desirable) to express operator== also in terms of operator<:

  bool operator==(const Fraction &lhs, const Fraction &rhs) {
      return !(lhs < hrs) && !(rhs < lhs);
  }
  

• There is no reason to publicly expose calculate_gcd and reduce.

• It might be a matter of taste, but I advise against the precompiled headers. In the long run, they create more problems than they solve.

Answer 3

Adding to the other answers:

• For this particular case (Fraction), make it a value-type. That is, do not allow functions to modify an instance [in particular, remove setters].

• Consider the standard members. You can add copy-constructor, move-constructor, copy-assignment, move-assignment. Use = default or = delete when appropriate.

• Consider conversions to and from other types. For example explicit Fraction(int n);.

• For Fraction your probably do not need a destructor (i.e. = default or omit it).

• Consider if and where constexpr may be appropriate.

• In your code there is a redundancy with the sign of the fraction. You might want to keep the sign in the numerator, and keep the denominator positive...

• Unfortunately, the denominator of fractions grows very quickly out of bounds. For general use, large- or unlimited-precision integers are necessary.

Answer 4

I have only two minor suggestions.

Use std::swap to swap values

In Fraction::calculate_gcd you can then write:

if (b > a)
    std::swap(a, b);

Use character literals to print single characters

When you perform a formatted stream output operation that effectively prints a single character it would be better to use a character literal '/' instead of a string literal "/" since it likely has less overhead:

os << value.get_num() << '/' << value.get_den();
// ...
std::cout << a << " != " << b << " = " << (a != b) << '\n';

However, this may not matter too much because the compiler may decide to perform this optimisation anyway^{[citation needed]}.