17
\$\begingroup\$

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
\$\endgroup\$
  • 2
    \$\begingroup\$ One reference worth checking out is std::ratio, std::ratio_add, std::gcd, etc. It does all this in compile time. That doesn't necessarily detract from the want to do this in runtime though. \$\endgroup\$ – David May 2 '17 at 17:51
  • \$\begingroup\$ Don't break encapsulation with get/set methods. Make your free floating functions part of the public interface and let them access the object or use the constructor to set up the object correctly. \$\endgroup\$ – Martin York May 2 '17 at 21:19
  • 1
    \$\begingroup\$ @LokiAstari Comments are for seeking clarification to the question. Please put all suggestions for improvements in answers, even if the answer is short and trivial. \$\endgroup\$ – 200_success May 2 '17 at 23:18
  • \$\begingroup\$ One thing to keep in mind is you will probably need careful handling of numbers with zero numerator and/or zero denominator. Also since you're using ints for the numerator and denominator you probably need to think carefully about sign - e.g. what if one or both are negative? Do your comparison functions work for these? \$\endgroup\$ – Michael Anderson May 3 '17 at 1:52
  • \$\begingroup\$ idiomatic Modern C++ could such a thing even be possible? XD \$\endgroup\$ – MikeTheLiar May 3 '17 at 13:56
11
\$\begingroup\$

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++.

\$\endgroup\$
18
\$\begingroup\$
  • 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.

\$\endgroup\$
  • 5
    \$\begingroup\$ Rather than defining your own operator> etc. You can use the standard ones. Just include <utility> thanusing namespace std::rel_ops; en.cppreference.com/w/cpp/utility/rel_ops/operator_cmp \$\endgroup\$ – Martin York May 2 '17 at 21:18
  • 2
    \$\begingroup\$ @LokiAstari, if it's not inside of a namespace, it might bring some weird behavior to the other parts of the code. \$\endgroup\$ – Incomputable May 2 '17 at 21:53
  • \$\begingroup\$ operator== can be even more simplified to: return std::tie(lhs.numerator, lhs.denominator) == std::tie(rhs.numerator, rhs.denominator) \$\endgroup\$ – Sergei Kurenkov May 3 '17 at 12:45
  • \$\begingroup\$ @SergeiKurenkov I don't consider that simpler. I find it harder to read \$\endgroup\$ – Justin May 3 '17 at 17:28
  • 1
    \$\begingroup\$ "I recommend dropping [unrestricted] getters and setters altogether." i recommend strongly against. Unless you are absolutely sure you will never change it and accept your solution with it's flaws. Otherwise, when you find a loophole, you'll be unable to put any restrictions on the state of the object in the future. E.g. preventing to set denominator to 0. I am very interested about the motivation for leaving a state variable public. I don't see it ever being a good idea, unless one is a fanatic of open/closed principle and fixing a bug in restrictions results in a derived class. \$\endgroup\$ – luk32 May 3 '17 at 21:58
11
\$\begingroup\$

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.

\$\endgroup\$
  • 5
    \$\begingroup\$ There's no need to declare copy/move constructors and assignment operators - leaving them out is the standard, well-understood way to indicate a trivially-copyable type. So I disagree with your second recommendation. Interestingly, you're not consistent between them and the destructor! \$\endgroup\$ – Toby Speight May 3 '17 at 9:23
  • \$\begingroup\$ @TobySpeight The rules for automatically-generated constructors and assignment operators are complicated (to me, at least), and risky for a non-expert (in that changing the class changes if and which are generated). So stating = default etc. seems less problematic. The rules for destructors are simpler [it is always generated if at all possible, IIRC], so omitting it seems less problematic [with some caveats...]. \$\endgroup\$ – Pablo H May 3 '17 at 18:02
  • \$\begingroup\$ I guess it's a trade-off - if you declare a copy constructor = default but forget the move constructor, then you could lose out on an efficient move. So there's scope for error whether you declare them or rely on the compiler-generated versions. Swings and roundabouts... \$\endgroup\$ – Toby Speight May 3 '17 at 20:22
10
\$\begingroup\$

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].

\$\endgroup\$
  • 4
    \$\begingroup\$ Not sure how much of an optimization '/' over "/" is. Even if it exists its probably a minute difference. But it definitely makes it look strange. So there is an argument for consistency and using "/". \$\endgroup\$ – Martin York May 2 '17 at 21:22
  • 1
    \$\begingroup\$ @LokiAstari: You're probably right in that the overhead of ostream and I/O in general probably exceeds that of "/" vs. '/' by far. \$\endgroup\$ – David Foerster May 2 '17 at 21:25
  • \$\begingroup\$ Thanks! I want to find out how much of a difference the "" and '' makes. I recall correctly, python/ruby have a tangible difference for these operators. \$\endgroup\$ – rustcpp May 5 '17 at 11:30
  • \$\begingroup\$ @rustcpp: Uh… sure go ahead and find out. If that was your way of asking a follow-up question, please take it to Stack Overflow. Both this comment section and Code Review in general are unsuitable for that. \$\endgroup\$ – David Foerster May 5 '17 at 11:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.