Problem
I started learning classes and operator overloading in C++. To test my skills I decided to make a Fraction class with all necessary operations.
I would appreciate improvements and suggesstions on my implementation.
Here is the code –
#include <iostream>
#include <string>
#include <stdexcept> // throw std::invalid_argument
// Function to find GCD(Greatest Common Divisor) of two numbers
int gcd(int x, int y)
{
if (y == 0)
return x;
return gcd(y, x % y);
}
// Function to find LCM(Lowest Common Multiple) of two numbers
int lcm(int x, int y)
{
return (x * y) / gcd(x, y);
}
// An immutable Fraction class
class Fraction
{
private:
int numerator;
int denominator;
public:
// Constructor for improper fractions
Fraction(int num, int deno=1)
{
if (deno == 0)
{
throw std::invalid_argument("Denominator can not be 0.");
}
int gcd_ = gcd(num, deno);
num /= gcd_;
deno /= gcd_;
numerator = num;
denominator = deno;
if (denominator < 0)
{
numerator *= -1;
denominator *= -1;
}
}
// Constructor for mixed fractions
Fraction(int whole, int num, int deno)
{
if (deno == 0)
{
throw std::invalid_argument("Denominator can not be 0.");
}
int gcd_ = gcd(num, deno);
num /= gcd_;
deno /= gcd_;
numerator = (whole * deno) + num;
denominator = deno;
}
// Getter methods
int get_numerator() {return numerator;}
int get_denominator() {return denominator;}
// Type casting methods
operator int() {return numerator / denominator;}
operator float() {return float(numerator) / denominator;}
operator double() {return double(numerator) / denominator;}
operator long() {return long(numerator) / denominator;}
operator std::string()
{
std::string str_frac = std::to_string(numerator);
if (denominator != 1)
{
str_frac += '/' + std::to_string(denominator);
}
return str_frac;
}
// Helps to print a fraction to the console
friend std::ostream& operator<<(std::ostream& os, const Fraction& frac)
{
os << frac.numerator;
if (frac.denominator != 1)
{
os << '/' << frac.denominator;
}
return os;
}
// Assingment operator
Fraction operator=(Fraction frac2)
{
numerator = frac2.numerator;
denominator = frac2.denominator;
int gcd_ = gcd(numerator, denominator);
numerator /= gcd_;
denominator /= gcd_;
}
// Unary '-' operator, eg. -(1/2)
Fraction operator-()
{
numerator *= -1;
if (denominator < 0)
{
numerator *= -1;
denominator *= -1;
}
return *this;
}
// Binary operators - (+, -, *, /)
Fraction operator+(Fraction frac2)
{
int lcm_ = lcm(denominator, frac2.denominator);
return Fraction(
(numerator * lcm_ / denominator) +
(frac2.numerator * lcm_ / frac2.denominator),
lcm_
);
}
Fraction operator-(Fraction frac2)
{
int lcm_ = lcm(denominator, frac2.denominator);
return Fraction(
(numerator * lcm_ / denominator) -
(frac2.numerator * lcm_ / frac2.denominator),
lcm_
);
}
Fraction operator*(Fraction frac2)
{
return Fraction(numerator * frac2.numerator, denominator * frac2.denominator);
}
Fraction operator/(Fraction frac2)
{
return Fraction(numerator * frac2.denominator, denominator * frac2.numerator);
}
// Comparision operators
bool operator<(Fraction frac2)
{
int lcm_ = lcm(denominator, frac2.denominator);
return (numerator * lcm_ / denominator) <
(frac2.numerator * lcm_ / frac2.denominator);
}
bool operator>(Fraction frac2)
{
int lcm_ = lcm(denominator, frac2.denominator);
return (numerator * lcm_ / denominator) >
(frac2.numerator * lcm_ / frac2.denominator);
}
bool operator==(Fraction frac2)
{
int lcm_ = lcm(denominator, frac2.denominator);
return (numerator * lcm_ / denominator) ==
(frac2.numerator * lcm_ / frac2.denominator);
}
bool operator<=(Fraction frac2)
{
int lcm_ = lcm(denominator, frac2.denominator);
return (numerator * lcm_ / denominator) <=
(frac2.numerator * lcm_ / frac2.denominator);
}
bool operator>=(Fraction frac2)
{
int lcm_ = lcm(denominator, frac2.denominator);
return (numerator * lcm_ / denominator) >=
(frac2.numerator * lcm_ / frac2.denominator);
}
};
int main()
{
Fraction frac1(-1, 2);
Fraction frac2(1, -2);
std::cout << "-1/2 * 1/-2 = " << frac1 * frac2 << 'n';
}
Thanks for your time!
Solution
The helper functions gcd() and lcm() should have internal linkage (declared static or in an anonymous namespace), to avoid link-time name conflicts when we make this a library.
Prefer to implement gcd() iteratively rather than recursively. Not all C++ compilers will eliminate even tail-recursion for you.
Prefer to initialise your members, rather than assigning within the constructor:
// Constructor for improper fractions
Fraction(int num, int deno=1)
: numerator{num},
denominator{deno}
{
if (denominator == 0)
{
throw std::invalid_argument("Denominator can not be 0.");
}
int gcd_ = gcd(numerator, denominator);
numerator /= gcd_;
denominator /= gcd_;
if (denominator < 0)
{
numerator *= -1;
denominator *= -1;
}
}
// Constructor for mixed fractions
Fraction(int whole, int num, int deno)
: Fraction{num + whole * deno, deno}
{}
The assignment operator has some problems:
- It fails to return a value (your compiler should be telling you about this – make sure you enable plenty of warnings).
- It shouldn’t need to reduce the fraction to simplest terms (we should simplify as part of each arithmetic operation).
It’s probably best to remove this operator and let the compiler default it, as we do for the copy/move constructor.
Conversion to float should probably do double division before truncating to float. Certainly on platforms such as the one I tested on (Linux/amd64) where float has less precision than int.
operator float() const { return static_cast<float>(operator double()); }
Conversion to integer types should be explicit:
explicit operator int() const { return numerator / denominator; }
Unary - should be a const operator, and not modify *this.
We should use the output streaming operator in our tests, instead of hard-coding the values in the string:
std::cout << frac1 << " * " << frac2 << " = "
<< frac1 * frac2 << 'n';
There’s no guard against integer overflow in any of this code. That’s something that should be considered.
Also look at my review of Implementation of a Rational Number class in C++ for some further inspiration, such as including user-defined literals and adding a suite of unit-tests.
Generally, making your binary arithmetic operators member function a you have here is an issue because the two arguments are treated differently. The right argument will undergo implicit conversions, but the left will not! So, given a Fraction f and int y you could write f+y but cannot compile y+f.
What you should do, canonically, is implement += as a member function, and then implement + as a non-member (not a friend either) that just calls += on a copy of the left argument.
You are missing const on a lot of your member functions. Why can’t you support, e.g.
const Fraction f (17,3);
int x = f; // invoke operator int().
if (7 > f) ... // can't do that either
if (f < 7) ... // but this one works?
const Fraction g (19,2);
if (f < g) ... // can't do that :(
Instead of int, you should declare the component scalar type as a named alias. Later, you can easily turn it into a template.
Instead of writing all the comparison operators (again, you have the left argument is different issue) individually, in C++20 you should implement the 3-way comparison operator and let them all autogenerate from that.
In older compilers, if that is your intent to support the, you are missing !=. In C++20 it’s autogenerated from ==.