A simple Fraction class

Question

My code seems to work fine. However, I feel like it is a bit too long (fraction.cpp). I really want to create simple, readable, and efficient code.

Sample Run:

Here is the fraction.h file:

#ifndef FRACTION_H 
#define FRACTION_H
#include <QString>

class Fraction {

public:
     Fraction();
     Fraction(int numberator, int denominator);
     void set(int numberator, int denominator);
     QString toString() const;
     double toDouble() const;
     int findGCD(int a, int b);
     Fraction simplify();
     Fraction add(const Fraction& other);
     Fraction substract(const Fraction& other);
     Fraction multiphy(const Fraction& other);
     Fraction divide(const Fraction& other);

private:
     int m_Numerator;
     int m_Denominator;
};

#endif // FRACTION_H

Here is the fraction.cpp file:

#include "fraction.h"

// Constructors:
Fraction::Fraction()
    :m_Numerator(0), m_Denominator(0) {}

Fraction::Fraction(int numerator, int denominator)
    :m_Numerator(numerator), m_Denominator(denominator) {
}

// Setter method
void Fraction::set(int numberator, int denominator) {
    m_Numerator = numberator;
    m_Denominator = denominator;
}

// GCD
int Fraction::findGCD(int a, int b) {
    if (a == 0) 
        return b;
    return findGCD(b%a, a);
}

// Simplify
Fraction Fraction::simplify() {
    int gcd = findGCD(m_Numerator, m_Denominator);
    return Fraction::Fraction(m_Numerator / gcd, m_Denominator / gcd);
}

double Fraction::toDouble() const {
    return ((double) m_Numerator/m_Denominator);
}

QString Fraction::toString()const {
    return QString("%1 / %2 = %3")
        .arg(m_Numerator)
        .arg(m_Denominator)
        .arg(toDouble());
} 

Fraction Fraction::add(const Fraction &other) {
    int numeratorTemp(m_Numerator);
    int denominatorTemp(m_Denominator);

    if (m_Denominator == other.m_Denominator) {
        numeratorTemp = m_Numerator + other.m_Numerator;
    } else {
        numeratorTemp = m_Numerator * other.m_Denominator + other.m_Numerator * m_Denominator;
        denominatorTemp = m_Denominator * other.m_Denominator;
    }
    return Fraction::Fraction(numeratorTemp, denominatorTemp);
}

Fraction Fraction::substract(const Fraction &other) {
    int numeratorTemp(m_Numerator);
    int denominatorTemp(m_Denominator);

    if (m_Denominator == other.m_Denominator) {
        numeratorTemp = m_Numerator - other.m_Numerator;
    } else {
        numeratorTemp = m_Numerator * other.m_Denominator - other.m_Numerator * m_Denominator;
        denominatorTemp = m_Denominator * other.m_Denominator;
    }
    return Fraction::Fraction(numeratorTemp, denominatorTemp);
}

Fraction Fraction::divide(const Fraction &other) {
    int numeratorTemp(m_Numerator);
    int denominatorTemp(m_Denominator);

    numeratorTemp = m_Numerator * other.m_Denominator;
    denominatorTemp = m_Denominator * other.m_Numerator;
    return Fraction::Fraction(numeratorTemp, denominatorTemp);
}

Fraction Fraction::multiphy(const Fraction &other) {
    int numeratorTemp(m_Numerator);
    int denominatorTemp(m_Denominator);

    numeratorTemp  = m_Numerator * other.m_Numerator;
    denominatorTemp = m_Denominator * other.m_Denominator;
    return Fraction::Fraction(numeratorTemp, denominatorTemp);
}

Main.cpp:

#include <QCoreApplication>
#include <iostream>
#include "fraction.h"

int main() {
    using namespace std;

    Fraction frac1(1, 2);
    Fraction frac2(5, 2);

    cout << "Addition: 1/2 + 5/2 = " << frac1.add(frac2).simplify().toString().toUtf8().constData() << "\n";
    cout << "Substraction: 1/2 - 5/2 = " << frac1.substract(frac2).simplify().toString().toUtf8().constData() << "\n";
    cout << "Divide: 1/2 : 5/2 = " << frac1.divide(frac2).simplify().toString().toUtf8().constData() << "\n";
    cout << "Multiplication: 1/2 * 5/2 = " << frac1.multiphy(frac2).simplify().toString().toUtf8().constData() << "\n";
}

Output: https://i.stack.imgur.com/cnT9A.png

Answer 1

#ifndef FRACTION_H 
#define FRACTION_H

Your guard name is vulnerable to collisions. Append more information and a good differentiator to reduce the chance a collision occurs.

//      PROJECT_FILENAME_CREATIONDATE_CREATIONAUTHOR (GUID, etc)
#ifndef MYPROJ_FRACTION_H_042018_ANNAPHUONG

---

Fraction::Fraction()
    :m_Numerator(0), m_Denominator(0) {}

Did you intend for 0 to be in the denominator? Consider the following code:

Fraction f;
std::cout << f.toDouble() << '\n';  // prints -nan

If your default constructed Fraction is supposed to be equal to zero, you should initialize the denominator to 1.

If you are going to initialize your members to constants, prefer the in-class initializers.

class Fraction {
    Fraction() = default;   // required since you provide
    Fraction(int n, int d); // <-- another constructor

private:
    int m_Numerator {0};    // 0 if other ctors don't initialize
    int m_Denominator {1};  // 1 if other ctors don't initialize
};

---

Fraction::Fraction(int numerator, int denominator)
    :m_Numerator(numerator), m_Denominator(denominator) {
}

If you don't intend to define the behavior for division by zero, you may want to check the 2nd argument.

Have you considered providing a single argument constructor for a whole number? i.e. Fraction f(42);.

---

void set(int numberator, int denominator);

Since you are working with int as your underlying type, there is no reason to allow the user to reuse an existing Fraction. If you were using a type that allowed for infinite precision (modulo available memory), perhaps it would then be appropriate. You take an immutable approach with every other function, so I would remain consistent and remove this setter.

---

int Fraction::findGCD(int a, int b) {
    if (a == 0) 
        return b;
    return findGCD(b%a, a);
}

findGCD doesn't require knowledge of the internal representation of your fraction. I would make this a free-standing non-member function.

---

double Fraction::toDouble() const {
    return ((double) m_Numerator/m_Denominator);
}

QString Fraction::toString()const {
    return QString("%1 / %2 = %3")
        .arg(m_Numerator)
        .arg(m_Denominator)
        .arg(toDouble());
} 

You could allow users to implement any additional required functionality if you provided num() and den() accessors.

template <typename Float>
Float fraction_cast(Fraction const f) {
    return static_cast<Float>(f.num()) / f.den();
}

template <>
QString fraction_cast<QString>(Fraction const f) {
    return QString("%1 / %2").arg(f.num()).arg(f.den());
}

int main() {
    Fraction f1(3, 2);
    std::cout << fraction_cast<QString>(f1) << '\n'; // prints "3 / 2"
    std::cout << fraction_cast<double>(f1) << '\n';  // prints "1.5"
}

Your Fraction is no longer dependent on QString, only the QString specialization for fraction_cast<> is.

---

Fraction Fraction::simplify() {
    int gcd = findGCD(m_Numerator, m_Denominator);
    return Fraction::Fraction(m_Numerator / gcd, m_Denominator / gcd);
}

Doesn't make any changes to the Fraction itself, consider const-correctness. If the denominator ever became negative, do you plan on simplifying (normalizing) the sign to the numerator?

---

     Fraction add(const Fraction& other);
     Fraction substract(const Fraction& other);
     Fraction multiphy(const Fraction& other);
     Fraction divide(const Fraction& other);

I would suggest overloading the arithmetic operators for each of these.

    const Fraction operator+(const Fraction&) const;
    const Fraction operator-(const Fraction&) const;
    const Fraction operator*(const Fraction&) const;
    const Fraction operator/(const Fraction&) const;

Also, some non-member free-standing functions, like comparisons, math functions (abs, sqrt, pow, invert, truncate, modulus, negate), boolean conversion (including operator!), increment/decrement, sign determination, etc.

Answer 2

One thing that would help simplify your implementation is to fall back to already implemented code. For example, you could implement subtraction as addition using the negative of the second fraction. Likewise you could implement division as multiplication with the reciprocal of the second fraction.

Another point about readability: because you are implementing a mathematical notion which fits well with the standard arithmetic operations, consider adding +, *, etc as operators. As above, these operators could simply call the add, multiply, etc functions you already have. E.g. Fraction operator+(const Fraction& other) {return add(other);}

Consider adding helper methods for negation and reciprocal. These could be private, and used only by the simplified subtract and divide as above, or they could be exposed as potentially useful to the class consumer.

Another simplification would be to avoid coding special cases when the general case works just as well. For example, when adding, you don't need to first check whether the denominators match. To help avoid the numbers getting out of hand, you could call simplify as a matter of course when doing these operations.

Consider moving some of your functions into the private section of the class definition. Generally, things that are intended to be used by the class and are not intended to be used from outside should be private. findGCD looks like a good candidate for moving. (One advantage of doing this to readability is that someone who wants to use your class knows they're only meant to look at the public methods, so they don't get drowned in irrelevant options.)

Consider getting rid of the set function completely. According to everything else, Fraction objects are immutable and cannot be changed. Instead, each operation returns a new Fraction object. Having set breaks this convention, and actually isn't that helpful a feature.

Consider marking all your member functions const if they don't change the current Fraction object. For example, Fraction simplify() const;

Consider marking all your functions (and your constructors) constexpr if you are using c++14 or greater. This tells the compiler that it's allowed to do some calculations at compile time if it knows the inputs, saving you having to do them at runtime.

As a point of efficiency, int numeratorTemp(m_Numerator); is unnecessarily setting a variable that will then be immediately overwritten. Hopefully an optimising compiler would simply pretend that never happened, but you might as well jump straight to const int numeratorTemp = m_Numerator * other.m_Denominator; or as appropriate.

You could do with some checks against illegal numbers. In particular, any fraction with zero as a denominator including 0/0 as given in your default constructor is undefined, and you should probably throw an exception. (For the same reason, I'd make the default constructor return 0/1.)

Nitpicks.

Although you should make your classes fit with the convention of your broader codebase, all things being equal it's probably better to have toString in a generic class produce a standard string rather than a <QString>.

You have a typo in multiply as multiphy. You also want numerator instead of numberator and subtract instead of substract.

When calling a constructor, don't qualify it. E.g Fraction(n, d); rather than Fraction::Fraction(n, d);