HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppModerate

A minimal Fraction class in C++

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
minimalclassfraction

Problem

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 

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 (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
#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 (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

Solution

-
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

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

Code Snippets

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

Context

StackExchange Code Review Q#162338, answer score: 17

Revisions (0)

No revisions yet.