A simple reusable class for rational numbers

Question

I created a class for fractions (rational numbers). It contains basic operators for arithmetics, comparisons, and input and output.

I often write classes for fractions, 2D or 3D points, and log probabilities for my projects. I even use templates for underlying types sometimes. For example, 2d points could be based on int, float, or Fraction.

It always takes quite some time to program those classes without bugs. I am creating a class containing all the basic operators that are needed. In the future, I will start by copying that into my project. It will contain

• constructors
• comparison operators
• arithmetic operators
• input and output operators

I think arithmetic operators are the most difficult part. They are often the most complex and I implement both assignment operator+= and usual operator+ operators. I always implement them as member functions and non-member functions respectively. I also often have problems with the input operator >>. Sometimes I use regex for that.

Would you implement the same operators for the class? Would you add or remove anything? Can something be simplified? I would also like to know all C++-related issues. Are there any better or easier ways to do things? I have used the same Makefile for all small C++ scripts I make. I would love to hear opinions about that.

Here is main.cpp:

#include <cassert>
#include <iostream>
#include <compare>
#include <ios>
#include <numeric>


class Fraction {
  public:
    constexpr Fraction(): Fraction(0) {}

    constexpr explicit Fraction(int numerator, int denominator = 1)
      : numerator(numerator), denominator(denominator)
    {
      assert(denominator != 0);
      reduce();
    }

    constexpr bool operator==(const Fraction& other) const = default;

    constexpr std::strong_ordering operator<=>(const Fraction& other) const {
      return numerator*other.denominator <=> other.numerator*denominator;
    }

    constexpr Fraction operator-() const { return Fraction { -numerator, denominator }; }

    constexpr Fraction& operator+=(const Fraction& other) {
      numerator = numerator*other.denominator + other.numerator*denominator;
      denominator *= other.denominator;
      reduce();
      return *this;
    }

    constexpr Fraction& operator-=(const Fraction& other) {
      operator+=(-other);
      return *this;
    }

    constexpr Fraction& operator*=(const Fraction& other) {
      numerator *= other.numerator;
      denominator *= other.denominator;
      reduce();
      return *this;
    }

    constexpr Fraction& operator/=(const Fraction& other) {
      operator*=(Fraction { other.denominator, other.numerator });
      return *this;
    }

    friend std::ostream& operator<<(std::ostream& out, const Fraction& fraction);
    friend std::istream& operator>>(std::istream& in, Fraction& fraction);

  private:
    constexpr void reduce() {
      int gcd = std::gcd(numerator, denominator);
      if (denominator < 0) gcd *= -1;
      numerator /= gcd;
      denominator /= gcd;
    }

    int numerator, denominator;
};


constexpr Fraction operator+(Fraction left, const Fraction& right) { return left += right; }
constexpr Fraction operator-(Fraction left, const Fraction& right) { return left -= right; }
constexpr Fraction operator*(Fraction left, const Fraction& right) { return left *= right; }
constexpr Fraction operator/(Fraction left, const Fraction& right) { return left /= right; }

std::ostream& operator<<(std::ostream& out, const Fraction& fraction) {
  return out << fraction.numerator << '/' << fraction.denominator;
}

std::istream& operator>>(std::istream& in, Fraction& fraction) {
  int denominator, numerator;
  char slash;
  in >> numerator >> slash >> denominator;

  if (slash != '/') {
    in.setstate(std::ios_base::failbit);
  }
  if (in) {
    fraction.numerator = numerator;
    fraction.denominator = denominator;
  }

  return in;
}


void ask_fraction() {
  Fraction fraction;
  std::cout << "Give fraction (for example 2/3): ";
  std::cin >> fraction;

  if (!std::cin) {
    std::cout << "invalid input\n";
    std::cin.clear();
    return;
  }

  std::cout << fraction << '\n';
}

int main() {
  Fraction x { 4, 6 };
  Fraction y { -5, -10 };
  Fraction z { -3 };

  std::cout << x << '\n';
  std::cout << y << '\n';
  std::cout << z << "\n\n";

  std::cout << "- " << x << " = " << -x << '\n';
  std::cout << x << " + " << y << " = " << x+y << '\n';
  std::cout << x << " - " << y << " = " << x-y << '\n';
  std::cout << x << " * " << y << " = " << x*y << '\n';
  std::cout << x << " / " << y << " = " << x/y << "\n\n";

  std::cout << std::boolalpha;
  std::cout << x << " == " << y << " = " << (x == y) << '\n';
  std::cout << x << " != " << y << " = " << (x != y) << '\n';
  std::cout << x << " < " << y << " = " << (x < y) << '\n';
  std::cout << x << " <= " << y << " = " << (x <= y) << '\n';
  std::cout << x << " > " << y << " = " << (x > y) << '\n';
  std::cout << x << " >= " << y << " = " << (x >= y) << "\n\n";

  ask_fraction();
  ask_fraction();
}

Here is Makefile:

appname = fraction

CXX = g++
CXXFLAGS = -Wall -g -std=c++2a

SRCS = main.cpp

.PHONY: test
test: $(appname)
    ./$(appname) < test_input.txt > user_output.txt
    diff -y user_output.txt test_output.txt

.PHONY: run
run: $(appname)
    ./$(appname)

$(appname): $(SRCS)
    $(CXX) $(CXXFLAGS) -o $(appname) $(SRCS)

.PHONY: clean
clean:
    $(RM) $(appname) user_output.txt

Here is test_input.txt:

3+2
5/3

Here is test_output.txt:

2/3
1/2
-3/1

- 2/3 = -2/3
2/3 + 1/2 = 7/6
2/3 - 1/2 = 1/6
2/3 * 1/2 = 1/3
2/3 / 1/2 = 4/3

2/3 == 1/2 = false
2/3 != 1/2 = true
2/3 < 1/2 = false
2/3 <= 1/2 = false
2/3 > 1/2 = true
2/3 >= 1/2 = true

Give fraction (for example 2/3): invalid input
Give fraction (for example 2/3): 5/3

Answer 1

Overview:

Note sure it is best to reduce on every construction. That may be a bit expensive. I see a couple of places where every mathematical operation creates an object. I would reduce before display or when it helps with some mathematical operation (potentially you could have a private constructor that your mathematical operators could use to prevent calls to reduce for intermediate values).

I went through this.

I can't really spot anything wrong with the code. I submitted it because I spent the time reading the code but there was nothing really that would stop me accepting this pull request. Every disagreement is personal preference - not something to worry about.

Code Review

I did not know that comparison had an automatic version.

    constexpr bool operator==(const Fraction& other) const = default;

Nice to know. I suppose I should have known as it is supported in C.

---

This looks normal to me:

    constexpr Fraction& operator+=(const Fraction& other) {
      numerator = numerator*other.denominator + other.numerator*denominator;
      denominator *= other.denominator;
      reduce();
      return *this;
    }

I suppose this is the normal. But you are constructing another object first (hence an extra call to reduce). I would just write it out like the operator+=. I don't think you are getting anything special here. I don't think you are going to save any work from nor having perfectly DRY code.

    constexpr Fraction& operator-=(const Fraction& other) {
      operator+=(-other);
      return *this;
    }

Normal:

    constexpr Fraction& operator*=(const Fraction& other) {
      numerator *= other.numerator;
      denominator *= other.denominator;
      reduce();
      return *this;
    }

This is a nice trick. I wish it would not call reduce in the constructor though.

    constexpr Fraction& operator/=(const Fraction& other) {
      operator*=(Fraction { other.denominator, other.numerator });
      return *this;
    }

---

    constexpr void reduce() {
      int gcd = std::gcd(numerator, denominator);


      // Don't be lazy.
      // The habit of not putting in the curly braces will
      // one day cause you a lot of headaches. Always use
      // the curly braces around blocks after for/while/for etc.
      if (denominator < 0) gcd *= -1;

      numerator /= gcd;
      denominator /= gcd;
    }

---

Love the use of the pass by value. Though personally I would have made them members of the class rather than free standing functions. I am not going to give you a real fuss that they are free standing.

constexpr Fraction operator+(Fraction left, const Fraction& right) { return left += right; }
constexpr Fraction operator-(Fraction left, const Fraction& right) { return left -= right; }
constexpr Fraction operator*(Fraction left, const Fraction& right) { return left *= right; }
constexpr Fraction operator/(Fraction left, const Fraction& right) { return left /= right; }

---

Again Great.

As not a mathematician but a CS I would prefer to see braces around the fraction (1/2) to make sure it is not ambiguous in a larger expression.

std::ostream& operator<<(std::ostream& out, const Fraction& fraction) {
  return out << fraction.numerator << '/' << fraction.denominator;
}

std::istream& operator>>(std::istream& in, Fraction& fraction) {
  int denominator, numerator;
  char slash;
  in >> numerator >> slash >> denominator;

  // Like this check.    
  if (slash != '/') {
    in.setstate(std::ios_base::failbit);
  }

  // Love this check.
  // Failure means no change to `fraction`
  if (in) {
    fraction.numerator = numerator;
    fraction.denominator = denominator;
  }

  return in;
}

Answer 2

For performance, I would suggest using gcd to simplify before multiplications / divisions. As an example:

Fraction x { 2027, 2029 };
Fraction y { 2029, 2027 };
return x * y;

The current approach would reduce (2027 * 2029)/(2029 * 2027). For (a/b) * (c/d), I would use gcd(a, d) and gcd(c, b). This is because you already know gcd(a, b) = 1 and gcd(c, d) = 1. It would save time compared to gcd(a * c, b * d). Similarly, you can compute the gcd of the denominators in advance and use that to speed up addition.

Reducing before multiplication lets you use more numbers without integer overflow. A future direction might be making your Fraction work with integer data types of various sizes. Also, good job covering the case of division by 0 - maybe add (1/2) / (0/1) to the test suite.

Answer 3

Value range

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

Most platforms define int to be 32 bits wide. But the C++ language standard only guarantees that int has at least 16 bits. If you do calculations with the restriction that numerators and denominators are less than about 32k, this can substantially limit the usefulness of the Fraction class.

One solution is to use long, which the language guarantees to be at least 32 bits, though many platforms define it as 64 bits. Another solution is to use std::int32_t (from <cstdint>), which is exactly 32 bits. Be aware of the integer types offered to you and make a deliberate choice.

Intermediate overflow

constexpr std::strong_ordering operator<=>(const Fraction& other) const {
  return numerator*other.denominator <=> other.numerator*denominator;
}

Even when both operands are legitimate Fraction values, the above calculation to compare values for ordering can overflow.

For example, assuming int is 16 bits (range −32768 ≤ x < 32767), this = Fraction(4000,3001), other = Fraction(2999,5000), then the comparison is 20000000 <=> 8999999 but both operands overflow. Note that signed integer overflow is undefined behavior in C++.

Consider bigint

In a chain of scalar calculations, dealing with integer ranges to avoid overflow is difficult enough. Carefully managing overflow involving two or more linked numbers (such as in this case with rationals, or another case where a complex number is represented as a pair of floats) is much, much harder.

As a consequence, every rational number class I have ever written uses a pair of arbitrary-precision integers. I value program correctness and my sanity (not needing to reason about ranges beforehand or debug overflow after the fact) much more than saving a few CPU cycles by using machine-sized integers.

Example: https://github.com/nayuki/Project-Euler-solutions/blob/12f3dbc66ab9b7905c6cfe6f2fb4810583307923/java/Library.java#L335-L423

Answer 4

This assertion looks unjustified:

      assert(denominator != 0);

Since denominator is supplied by the caller, we have no way to be sure that it's non-zero. I think we have to explicitly test for /0, and take appropriate action - perhaps throw a std::domain_error?

---

Consider adding functionality:

• ++, -- (prefix and suffix versions)
• unary + and -
• operator double
• literal operator
• template for the representation type (instead of always int)
• overflow prevention