Implementation of a Rational Number class in C++

Question

First of all, disclaimer; this is for a school assignment. I hope I'm not doing anything inappropriate by asking for input on it, but if it makes a difference, the assignment has already been submitted and graded.

I've been having some problems with the professor that submitted the assignment, but I value my education and want to understand this as much as possible. He just announced in class that he will no longer be answering questions via email, and that we'll have to attend his office hours if we have any. This is a bit of a problem for me as I'm working full time while in school.

The assignment was to implement a Rational number class from a header that he provided. We aren't allowed to modify the contents of the header and are required to implement the class exactly as it's found there.

To make a long story short, there's a section on the submission website where he posts feedback for us on our assignments. My feedback was "Too many errors to list" and nothing else. I'm at a loss, I'm sure my code isn't perfect but I felt like I tested it pretty thoroughly at it always produced the expected output.

If anyone more experienced/knowledgeable than me would be kind enough to review my code and help me understand some of the errors he may be referring to, I'd really appreciate it. I plan on finding a chance to go into his office hours, but he's got a very eclectic way of explaining things and seems to view explaining concepts to students as an inconvenience, so I don't want to put all my eggs in that basket. Its frustrating because I do a ton of outside reading, I'm really motivated to learn and spend a ton of time helping other students in that class understand the concepts that he doesn't lecture on. I'm just at a loss for what I'm missing here.

Here's the code: As I mentioned, Rational.h is provided by him and we can't change it. I don't like some things about it (like the using namespace std in the header), but I can't do anything about it unfortunately.

Rational.h

#ifndef _RATIONAL_H_
#define _RATIONAL_H_

#include<iostream>


using namespace std;

class Rational
{
  long long _p;
  long long _q;

  void simplify();

  public:
  Rational();
  Rational (long long p, long long Q = 1);
  Rational (const Rational&);

  Rational& operator= (const Rational&);
  Rational& operator+= (const Rational&);
  Rational& operator-= (const Rational&);
  Rational& operator*= (const Rational&);
  Rational& operator/= (const Rational&);

  friend ostream& operator<< (ostream&, const Rational&);
  friend istream& operator>> (istream&, Rational&);

  Rational operator+ (const Rational&);
  Rational operator+ (long long) const;
  friend Rational operator+ (long long, const Rational&);
  Rational operator- (const Rational&);
  Rational operator- (long long) const;
  friend Rational operator- (long long, const Rational&);
  Rational operator* (const Rational&);
  Rational operator* (long long) const;
  friend Rational operator* (long long, const Rational&);
  Rational operator/ (const Rational&);
  Rational operator/ (long long) const;
  friend Rational operator/ (long long, const Rational&);

  bool operator== (const Rational&) const;
  bool operator== (long long) const;
  friend bool operator== (long long, const Rational&);
  bool operator!= (const Rational&) const;
  bool operator!= (long long) const;
  friend bool operator!= (long long, const Rational&);
  bool operator> (const Rational&) const;
  bool operator> (long long) const;
  friend bool operator> (long long, const Rational&);
  bool operator< (const Rational&) const;
  bool operator< (long long) const;
  friend bool operator< (long long, const Rational&);
  bool operator>= (const Rational&) const;
  bool operator>= (long long) const;
  friend bool operator>= (long long, const Rational&);
  bool operator<= (const Rational&) const;
  bool operator<= (long long) const;
  friend bool operator<= (long long, const Rational&);

  Rational operator++ (int);
  Rational operator-- (int);
  Rational& operator++ ();
  Rational& operator-- ();
  Rational operator- () const;
  Rational operator+ () const;

  Rational pow (unsigned exp) const;
  Rational inverse() const;
};

#endif

Rational.cpp

#include "Rational.h"
#include <iostream>

void validate (long long, long long);

int gcd (long long, long long);

Rational::Rational()
{
    _p = 0;
    _q = 1;
}

Rational::Rational (long long p, long long Q)
{
    validate (p, Q);
    _p = p;
    _q = Q;
}

Rational::Rational (const Rational& rat)
{
    this->_p = rat._p;
    this->_q = rat._q;
}
void Rational::simplify()
{
    // Fixes negative denominators.
    if (_q < 0)
    {
        _p *= -1;
        _q *= -1;
    }

    // Simplifies Rational Numbers.
    int denom = gcd(_p, _q);
    _p /= denom;
    _q /= denom;

}

Rational& Rational::operator= (const Rational& rat)
{
    _p = rat._p;
    _q = rat._q;

    return *this;
}

Rational& Rational::operator+= (const Rational& rat)
{
    _p = ((_p * rat._q) + (_q * rat._p));
    _q *= rat._q;

    this->simplify();

    return *this;
}

Rational& Rational::operator-= (const Rational& rat)
{
    _p = ((_p * rat._q) - (_q * rat._p));
    _q *= rat._q;

    this->simplify();

    return *this;
}

Rational& Rational::operator*= (const Rational& rat)
{
    _p *= rat._p;
    _q *= rat._q;

    this->simplify();

    return *this;
}

Rational& Rational::operator/= (const Rational& rat)
{
    if (rat._p == 0)
    {
        throw "Division by zero not allowed";
    }
    _p *= rat._q;
    _q *= rat._p;

    this->simplify();

    return *this;
}

ostream& operator<< (ostream& os, const Rational& rat)
{
    os << rat._p << ":" << rat._q;

    return os;
}

istream& operator>> (istream& is, Rational& rat)
{
    long long p, q;

    is >> p >> q;
    validate(p, q);
    rat._p = p;
    rat._q = q;
    rat.simplify();

    return is;
}

Rational Rational::operator+ (const Rational& rat)
{
    Rational result(*this);

    result += rat;
    result.simplify();

    return result;
}

Rational Rational::operator+ (long long num) const
{
    Rational result(*this);
    Rational temp(num);

    result += temp;
    result.simplify();

    return result;
}

Rational operator+ (long long num, const Rational& rat)
{
    Rational result(num);
    result += rat;
    result.simplify();

    return result;
}

Rational Rational::operator- (const Rational& rat)
{
    Rational result(*this);

    result -= rat;
    result.simplify();

    return result;
}

Rational Rational::operator- (long long num) const
{
    Rational result(*this);
    Rational temp(num);

    result -= temp;
    result.simplify();

    return result;
}

Rational operator- (long long num, const Rational& rat)
{
    Rational result(num);
    result -= rat;
    result.simplify();

    return result;
}

Rational Rational::operator* (const Rational& rat)
{
    Rational result(*this);

    result *= rat;
    result.simplify();

    return result;
}

Rational Rational::operator* (long long num) const
{
    Rational result(*this);
    Rational temp(num);
    result *= temp;
    result.simplify();

    return result;
}

Rational operator* (long long num, const Rational& rat)
{
    Rational result(num);
    result *= rat;
    result.simplify();

    return result;
}

Rational Rational::operator/ (const Rational& rat)
{
    Rational result(*this);

    result /= rat;
    result.simplify();

    return result;
}

Rational Rational::operator/ (long long num) const
{
    Rational result(*this);
    Rational temp(num);

    result /= temp;
    result.simplify();

    return result;
}

Rational operator/ (long long num, const Rational& rat)
{
    Rational result(num);
    result /= rat;
    result.simplify();

    return result;
}

bool Rational::operator== (const Rational& rat) const
{
    bool result;

    if ((this->_p == rat._p) && (this->_q == rat._q))
    {
        result = true;
    }
    else
    {
        result = false;
    }

    return result;
}

bool Rational::operator== (long long num) const
{
    bool result;
    Rational temp(num);

    result = (*this == temp);

    return result;
}

bool operator== (long long num, const Rational& rat)
{
    bool result;

    result = (rat == num);

    return result;
}

bool Rational::operator!= (const Rational& rat) const
{
    return !(*this == rat);
}

bool Rational::operator!= (long long num) const
{
    return !(*this == num);
}

bool operator!= (long long num, const Rational& rat)
{
    return !(num == rat);
}

bool Rational::operator> (const Rational& rat) const
{
    bool result;

    if ((this->_p / this->_q) > (rat._p / rat._q))
    {
        result = true;
    }
    else
    {
        result = false;
    }

    return result;
}

bool Rational::operator> (long long num) const
{
    bool result;
    Rational temp(num);

    result = (*this > temp);

    return result;
}

bool operator> (long long num, const Rational& rat)
{
    bool result;

    result = (rat < num);

    return result;
}

bool Rational::operator< (const Rational& rat) const
{
    bool result;

    if (!(*this > rat) && !(*this == rat))
    {
        result = true;
    }
    else
    {
        result = false;
    }

    return result;
}

bool Rational::operator< (long long num) const
{
    bool result;
    Rational temp(num);

    result = (*this < temp);

    return result;
}

bool operator< (long long num, const Rational& rat)
{
    bool result;

    result = (rat > num);

    return result;
}

bool Rational::operator>= (const Rational& rat) const
{
    bool result;

    if (!(*this < rat))
    {
        result = true;
    }
    else
    {
        result = false;
    }

    return result;
}

bool Rational::operator>= (long long num) const
{
    bool result;
    Rational temp(num);

    result = (*this >= temp);

    return result;
}

bool operator>= (long long num, const Rational& rat)
{
    bool result;

    result = (rat <= num);

    return result;
}

bool Rational::operator<= (const Rational& rat) const
{
    bool result;

    if (!(*this > rat))
    {
        result = true;
    }
    else
    {
        result = false;
    }

    return result;
}

bool Rational::operator<= (long long num) const
{
    bool result;
    Rational temp(num);

    result = (*this <= temp);

    return result;
}

bool operator<= (long long num, const Rational& rat)
{
    bool result;

    result = (rat >= num);

    return result;
}

Rational Rational::operator++ (int) // Postfix
{
    Rational temp(*this);

    this->_p++;
    this->_q++;

    return temp;
}

Rational Rational::operator-- (int) // Postfix
{
    Rational temp(*this);

    this->_p--;
    this->_q--;

    return temp;
}

Rational& Rational::operator++()
{
    this->_p++;
    this->_q++;

    return *this;
}

Rational& Rational::operator--()
{
    this->_p--;
    this->_q--;

    return *this;
}

Rational Rational::operator-() const
{
    Rational temp(-(this->_p), (this->_q));

    return temp;
}

Rational Rational::operator+() const
{
    Rational temp(+(this->_p), +(this->_q));

    return temp;
}

Rational Rational::pow (unsigned exp) const
{
    Rational result(*this);
    Rational temp(*this);

    if (exp == 0)
    {
        result = 1;
    }
    else
    {
        for (unsigned i = 1; i < exp; i++)
        {
            result *= temp;
        }
    }

    return result;
}

Rational Rational::inverse() const
{
    Rational temp(this->_q, this->_p);

    return temp;
}

void validate(long long p, long long q)
{
    p++; // Supress error for unused value. Decided to keep value in parameter list to maintain clarity.
    if (q == 0)
    {
        throw "Zero Denominator";
    }
}

int gcd(long long p, long long q)
{
    // Euclid's Algorithm
    if (q == 0)
    {
        return p;
    }
    return gcd (q, p%q);
}

Answer 1

In the comments below, where a family of operators all follow a common pattern, I've just addressed one of them; understand that to mean that my comments likely apply equally to the others of that family.

Prefer initialization of members to assignment

Instead of

Rational::Rational()
{
    _p = 0;
    _q = 1;
}

we normally write

Rational::Rational()
  : _p{0},
    _q{1}
{
}

And in modern C++, we delegate to a different constructor:

Rational::Rational()
    : Rational{0}
{
}

In truth, if we controlled the interface, we'd just declare the first argument with a default of 0 and not even need to write a default constructor:

  Rational(long long = 0, long long = 1);

The other constructors should also initialize rather than assign:

Rational::Rational(long long p, long long Q)
    : _p{p},
      _q{Q}
{
    validate(_p, _q);
}

Rational::Rational(const Rational& rat)
    : _p{rat._p},
      _q{rat._q}
{
}

Alternatively, delegate the copy constructor, too:

Rational::Rational(const Rational& rat)
    : Rational{rat._p, rat._q}
{
}

Normally though, we would just let the compiler generate that one:

Rational::Rational(const Rational& rat) = default;

Prefer standard exception types

Here you throw a const char*:

if (rat._p == 0)
    throw "Division by zero not allowed";

Instead, we can use the Standard Library exception type for this:

if (rat._p == 0)
    throw std::domain_error{"Division by zero not allowed"};

You'll need to include <stdexcept> for this.

Inline the argument validation

Instead of a stand-alone validate() function, it's clearer to test the argument in the constructor:

Rational::Rational(long long p, long long Q)
    : _p{p},
      _q{Q}
{
    if (_q == 0)
        throw std::domain_error{"Zero Denominator"};
}

If we use the constructor in the input stream operator, then there's no need for validate() any more, and we've reduced duplication even further:

istream& operator>> (istream& is, Rational& rat)
{
    long long p, q;
    is >> p >> q;
    rat = {p, q};
    return is;
}

No need to call simplify() on already-reduced values

All the binary arithmetic operators follow the same (correct) pattern:

Rational Rational::operator+ (const Rational& rat)
{
    Rational result(*this);

    result += rat;
    result.simplify();

    return result;
}

The assignment operator (here, +=) has already called simplify(), so there's no need for us to repeat it:

Rational Rational::operator+ (const Rational& rat)
{
    return Rational{*this} += rat;
}

But you should call simplify() when constructing:

Rational::Rational(long long p, long long Q)
    : _p{p},
      _q{Q}
{
    if (_q == 0)
        throw std::domain_error{"Zero Denominator"};
    simplify();
}

If you don't do that, then Rational{1,2} != Rational{2,4} for example.

You can cheat with the overloads of binary operators

The interface makes you overload the binary operators with both const Rational& and long long. But there's a default promotion from long long to Rational so that wasn't really necessary (and isn't any more efficient if the compiler is doing its job properly). We can explicitly construct a Rational to save us repeating ourselves:

Rational Rational::operator+(long long num) const
{
    return *this + Rational{num};
}

Rational operator+(long long num, const Rational& rat)
{
    return Rational{num} + rat;
}

At least we could, if the header had declared operator+() properly (with const):

  Rational operator+(const Rational&) const;

Oddly, the long long overload is declared const, so perhaps that was just to trip you up.

Simplify expressions

Here's a very long-winded way to write a simple test:

bool Rational::operator== (const Rational& rat) const
{
    bool result;

    if ((this->_p == rat._p) && (this->_q == rat._q))
    {
        result = true;
    }
    else
    {
        result = false;
    }

    return result;
}

I'd write that without the temporary, as:

bool Rational::operator==(const Rational& rat) const
{
    return _p == rat._p && _q == rat._q;
}

Bug: comparison operators

(Conventionally, we implement our comparators in terms of operator<() and operator==; many algorithms require only <, and using namespace std::rel_ops can allow us to default the others. But I'll go with your approach).

This is integer division:

if (_p / t_q > rat._p / rat._q)

It's almost certainly not what you meant, as it will report 1/2 and 1/3 as equal (both expressions truncate, and 0 == 0). Perhaps

return double(_p)/q > double(rat._p)/rat.q;

(I've simplified to a simple return, as for operator==() above). If you had control over the header, you could could consider adding

explicit operator double() const { return double(_p)/q; }

Then the comparison becomes:

return static_cast<double>(*this) > static_cast<double>(rat);

An alternative approach is to use the binary operator-() that you wrote:

auto diff = *this - rat;
# We know the denominator is kept positive
return diff._p > 0;

A bug in increment/decrement

Rational& Rational::operator++()
{
    this->_p++;
    this->_q++;

    return *this;
}

This should add 1 to the value. Either write it as

Rational& Rational::operator++()
{
     return (*this) += 1;
}

Or re-write the arithmetic:

Rational& Rational::operator++()
{
    _p += _q;
    return *this;
}

Write unary operators in terms of the binary ones

Having written binary operators, it's simplest to re-use them for the unary ones, using the identities +x == x and -x == 0-x:

Rational Rational::operator-() const
{
    return 0 - *this;
}

Rational Rational::operator+() const
{
    return *this;
}

Simplify pow()

You don't need to special-case exp==0, if you start with result = 1; and multiply successively:

Rational Rational::pow (unsigned exp) const
{
    Rational result{1}

    for (unsigned i = 0;  i < exp;  ++i)
        result *= *this;

    return result;
}

There are more efficient methods for large exp, but I'll leave you to research that if you're interested.

Prefer iteration to recursion

Euclid's Algorithm is naturally recursive, but C++ works better with iterative algorithms, as this conserves stack space. It may be that your compiler performs tail-call elimination, but that's not mandated by C++ specifications. If it doesn't, or if you want to help it, or simply improve your understanding, you could convert it to iterative form:

long long gcd(long long p, long long q)
{
    while (q) {
        auto t = p%q;
        p = q;
        q = t;
    }
    return p;
}

or

long long gcd(long long p, long long q)
{
    while (q) {
        std::swap(p, q);
        q %= p;
    }
    return p;
}

I've also changed the return type to match the possible results.

It's also a good idea to give gcd internal linkage, so it doesn't collide with any other definition when used in a program. You can declare it static or put it into an anonymous namespace.

Check input stream when reading

The >> stream operator ignores errors in reading to p and q. This can cause it to try to construct a Rational from uninitialised data. Also, there's a bug, because it doesn't read the separating : that you write (I found this using unit test - see end of answer):

std::istream& operator>>(std::istream& is, Rational& rat)
{
    long long p, q;
    char sep;
    if (is >> p >> sep >> q && sep == ':')
        rat = {p, q};
    return is;
}

A bug somewhere

The very first test I wrote, failed:

int main()
{
    Rational r1{1,4};
    Rational r2{2,4};

    return (r1 + r2 != Rational{3/4});
}

But this was a bug in the test! Rational{3/4} means Rational{0} (integer division), but I meant Rational{3,4}. I improved the test:

int verify(Rational actual, Rational expected, const char *expression)
{
    if (actual != expected)
        std::cerr << expression << " was " << actual << " but should be " << expected << '\n';
    return (actual != expected);
}

#define TEST(a, b) verify(a, b, #a)

int main()
{
    Rational r1{1,4};
    Rational r2{2,4};

    return TEST(r1 + r2, Rational{3}/Rational{4})
        +  TEST(2 * r1 - r2, 0);
}

This prints diagnostics for failing tests, and is a step on the way towards employing a real unit-test framework:

r1 + r2 was 3:4 but should be 0:1

When I fixed the error, I got a nice clean build+run.

---

If you were allowed to change the class definition...

I know it's not up for review, but I would fix the following issues:

Don't use namespace std;

You know about this one

Don't include <iostream> in headers

There's <iosfwd> that provides forward definitions of classes including ostream and istream.

Constructors should be constexpr

  constexpr Rational (long long p = 0, long long Q = 1);
  constexpr Rational (const Rational&);

This allows us to create user-defined literals, e.g.

constexpr Rational operator""_r(unsigned long long p)
{
    // default conversion
    return p;
}

Then we can write 1_r/4 or 1/4_r instead of Rational{1,4} and so on.

Use constexpr elsewhere that you can

Most of the methods depend only on their arguments, so can be declared constexpr. This can transfer computation from run-time to compile-time, and is desirable. It's also necessary for user-defined literals to work.

Allow any integer type to convert to Rational

We can do this with Concepts:

template<typename T>
constexpr Rational::Rational(T p, T q) requires (std::is_integral<T>::value)
    : _p{p},
      _q{q}
{
    if (_q == 0)
        throw std::domain_error{"Zero Denominator"};
    simplify();
}

If you don't have Concepts, you can use std::enable_if instead, but it's noticeably more verbose.

One problem that can now arise is that unsigned long long won't necessarily fit, so we'll have to have to test for overflow, or provide an overload:

constexpr Rational::Rational(unsigned long long p, unsigned long long q)
    : Rational{static_cast<long long>(p), static_cast<long long>(q)}
{
    // Were the static_cast<> both valid?
    constexpr unsigned long long max_ll =  std::numeric_limits<long long>::max();
    if (p > max_ll || q > max_ll)
        throw std::domain_error{"value out of range"};
}

Reduce the binary operators

The binary operators that take integers as the second argument are not required, as default promotions will convert the argument to a Rational. I suspect they were declared so that a plain int would convert (the compiler is not allowed a two-step conversion int -> long long -> Rational, but we've now provided a constructor taking any integer type, which addresses that problem.

The binary operators that take integers as the first argument don't need to be friends, as they can all be re-written using the public interface (and they too can be generic). You can either promote the first argument to Rational or you can re-order the arguments; here's one of each:

template<typename T>
Rational operator-(T a, Rational b)
{
    return -b + a;
}

template<typename T>
Rational operator*(T a, Rational b)
{
    return Rational{a} * b;
}

You can add optionally add requires std::is_integral<T>::value if you're using Concepts (if you don't, a violation will be reported from the Rational constructor, so it doesn't add significant value).

Binary operators should be non-members

We have an asymmetry because we can add Rational + int but not int + Rational. We need a non-member operator+ to get our first argument converted for us.

Let C++ declare your comparison operators

If you provide operator< and operator==, you can get all the others by using namespace rel_ops; where you want them.

When we move to C++20, we can use the new "spaceship" operator <=> instead; that simplifies our work even further.

Make simplify() return a reference to the object

Most places where simplify is called are of the form:

simplify();
return *this;

By allowing simplify() to return that reference, we can reduce all those to

return simplify();

---

My version

I've made nearly all the changes above (including the changes to the class definition that were out of scope for you) and I'm assuming C++17 with concepts. My compilation command is

g++ -std=c++17 -fPIC -g \
    -Wall -pedantic -Wextra -Wwrite-strings -Wno-parentheses -Weffc++ \
    -fconcepts

Header file

#ifndef _RATIONAL_H_
#define _RATIONAL_H_

#include <iosfwd>
#include <stdexcept>
#include <type_traits>
#include <utility>

class Rational
{
    long long p;
    long long q;

    constexpr Rational& simplify();

public:
    template<typename T> constexpr Rational(T p, T q = 1) requires std::is_integral<T>::value;
    constexpr Rational(unsigned long long p = 0, unsigned long long q = 1);
    constexpr Rational(const Rational&);

    // assignment operators
    constexpr Rational& operator=(const Rational&);
    constexpr Rational& operator+=(const Rational&);
    constexpr Rational& operator-=(const Rational&);
    constexpr Rational& operator*=(const Rational&);
    constexpr Rational& operator/=(const Rational&);

    // comparison operators
    friend constexpr bool operator==(const Rational&, const Rational&);
    friend constexpr bool operator<(const Rational&, const Rational&);

    // increment and decrement operators
    constexpr Rational operator++(int);
    constexpr Rational operator--(int);
    constexpr Rational& operator++();
    constexpr Rational& operator--();

    // type conversion
    constexpr explicit operator double() const;

    // stream operators
    friend std::ostream& operator<<(std::ostream&, const Rational&);
    friend std::istream& operator>>(std::istream&, Rational&);

    // arithmetic functions
    constexpr Rational pow(unsigned exp) const;
    constexpr Rational inverse() const;
};

// arithmetic operators
constexpr Rational operator+(const Rational&, const Rational&);
constexpr Rational operator-(const Rational&, const Rational&);
constexpr Rational operator*(const Rational&, const Rational&);
constexpr Rational operator/(const Rational&, const Rational&);
constexpr Rational operator-(const Rational&);
constexpr Rational operator+(const Rational&);


template<typename T>
constexpr Rational::Rational(T p, T q) requires (std::is_integral<T>::value)
    : p{p},
      q{q}
{
    if (q == 0)
        throw std::domain_error{"zero Denominator"};
    simplify();
}

#endif

Implementation

#include "rational.h"
#include <iostream>
#include <limits>
    
constexpr Rational operator""_r(unsigned long long p)
{
    // default conversion
    return p;
}


namespace {
    constexpr long long gcd(long long p, long long q)
    {
        while (q) {
            p %= q;
            std::swap(p, q);
        }
        return p;
    }
}

constexpr Rational::Rational(unsigned long long p, unsigned long long q)
    : Rational{static_cast<long long>(p), static_cast<long long>(q)}
{
    // Retrospectively justify static_cast<> above
    constexpr unsigned long long max_ll = std::numeric_limits<long long>::max();
    if (p > max_ll || q > max_ll)
        throw std::domain_error{"value out of range"};
}

constexpr Rational::Rational(const Rational& rat) = default;

constexpr Rational& Rational::simplify()
{
    // Fix negative denominators
    if (q < 0) {
        p = -p;
        q = -q;
    }

    // Reduce by greatest common divisor.
    // N.B. if p==0, this results in 0/1 as desired.
    const auto denom = gcd(p, q);
    p /= denom;
    q /= denom;

    return *this;
}

constexpr Rational& Rational::operator=(const Rational& rat)
{
    p = rat.p;
    q = rat.q;
    return *this;
}

constexpr Rational& Rational::operator+=(const Rational& rat)
{
    p = p * rat.q + q * rat.p;
    q *= rat.q;
    return simplify();
}

constexpr Rational& Rational::operator-=(const Rational& rat)
{
    p = p * rat.q - q * rat.p;
    q *= rat.q;
    return simplify();
}

constexpr Rational& Rational::operator*=(const Rational& rat)
{
    p *= rat.p;
    q *= rat.q;
    return simplify();
}

constexpr Rational& Rational::operator/=(const Rational& rat)
{
    if (rat.p == 0)
        throw std::domain_error{"Division by zero not allowed"};
    return *this *= rat.inverse();
}


constexpr Rational operator+(const Rational& a, const Rational& b)
{
    return Rational{a} += b;
}

constexpr Rational operator-(const Rational& a, const Rational& b)
{
    return Rational{a} -= b;
}

constexpr Rational operator*(const Rational& a, const Rational& b)
{
    return Rational{a} *= b;
}

constexpr Rational operator/(const Rational& a, const Rational& b)
{
    return Rational{a} /= b;
}

constexpr Rational operator-(const Rational& r)
{
    return 0 - r;
}

constexpr Rational operator+(const Rational& r)
{
    return r;
}


constexpr bool operator==(const Rational& a, const Rational& b)
{
    return a.p == b.p && a.q == b.q;
}

constexpr bool operator<(const Rational& a, const Rational& b)
{
    return a.p * b.q  <  a.q * b.p;
}


constexpr Rational Rational::operator++(int) // Postfix
{
    Rational temp{*this};
    p += q;
    return temp;
}

constexpr Rational Rational::operator--(int) // Postfix
{
    Rational temp{*this};
    p -= q;
    return temp;
}

constexpr Rational& Rational::operator++()
{
    return *this += 1;
}

constexpr Rational& Rational::operator--()
{
    return *this -= 1;
}

constexpr Rational::operator double() const
{
    return static_cast<double>(p) / static_cast<double>(q);
}


std::ostream& operator<<(std::ostream& os, const Rational& rat)
{
    return os << rat.p << ":" << rat.q;
}

std::istream& operator>>(std::istream& is, Rational& rat)
{
    long long p, q;
    char sep;
    if (is >> p >> sep >> q && sep == ':')
        rat = {p, q};
    return is;
}

constexpr Rational Rational::pow(unsigned exp) const
{
    auto x = *this;
    Rational r{1};
    for (;  exp;  exp /= 2) {
        if (exp%2) r *= x;
        x *= x;
    }
    return r;
}

constexpr Rational Rational::inverse() const
{
    if (p < 0) {
        // Make the denominator positive
        return {-q, -p};
    } else {
        return {q, p};
    }
}



//************************************************************************************
// Test Code starts here

#include <sstream>
using namespace std::rel_ops;

int verify(bool result, Rational aval, Rational bval, const char *a, const char *op, const char *b, const char *file, int line)
{
    if (!result)
        std::cerr << file << ":" << line << ": "
                  << a << " " << op << " " << b << "  --  "
                  << aval << " " << op << " " << bval << "\n";
    return !result;
}

template<typename A, typename B>
int verify(A aval, B bval, const char *a, const char *b, const char *file, int line)
{
    if (!(aval == bval))
        std::cerr << file << ":" << line << ": "
                  << a << " == " << b << "  --  "
                  << aval << " == " << bval << "\n";
    return !(aval == bval);
}

#define TEST_OP(a, op, b) verify((a) op (b), (a), (b), #a, #op, #b, __FILE__, __LINE__)
#define TEST_EQUAL(a, b) verify((a), (b), &#a[0], &#b[0], &__FILE__[0], __LINE__)

int main()
{
    int errors{};

    errors += TEST_OP(Rational(1,2), ==, 1_r/2);
    errors += TEST_OP(Rational(1,2), ==, 1/2_r);
    errors += TEST_OP(Rational(1,2), ==, 2/4_r);
    errors += TEST_OP(2/4_r, ==, 1/2_r);
    errors += TEST_OP(-1/2_r, ==, 1/-2_r);
    errors += TEST_OP(2u, ==, 2_r);
    errors += TEST_OP(2_r, ==, 2u);
    errors += TEST_OP(Rational(1,2), !=, 1/3_r);

    errors += TEST_OP(1/3_r, <, 2/5_r);
    errors += TEST_OP(2/5_r, >, 1/3_r);
    errors += TEST_OP(1/3_r, <=, 2/5_r);
    errors += TEST_OP(1/3_r, <=, 1/3_r);

    errors += TEST_EQUAL(1/3_r + 1/4_r, 7/12_r);
    errors += TEST_EQUAL(1L + 1/4_r, 5/4_r);
    errors += TEST_EQUAL(1/4_r - 1/3_r, -1/12_r);
    errors += TEST_EQUAL(1/3_r - 1/3_r, 0_r);
    errors += TEST_EQUAL(1/5_r * 5, 1);
    errors += TEST_EQUAL(-2_r * -2_r, 4);
    errors += TEST_EQUAL(1/5_r / 3, 1/15_r);
    errors += TEST_EQUAL(1/3_r * 0_r, 0_r);

    Rational x;
    errors += TEST_EQUAL(x, 0);
    errors += TEST_EQUAL(++x, 1);
    errors += TEST_EQUAL(x++, 1);
    errors += TEST_EQUAL(x, 2);
    errors += TEST_EQUAL(x = 1/2_r, 1/2_r);
    errors += TEST_EQUAL(++x, 3/2_r);
    errors += TEST_EQUAL(++x, 5/2_r);
    errors += TEST_EQUAL(x--, 5/2_r);
    errors += TEST_EQUAL(x--, 3/2_r);
    errors += TEST_EQUAL(x, 1/2_r);

    errors += TEST_EQUAL(Rational(2,3).pow(3), 8/27_r);
    errors += TEST_EQUAL((2/3_r).inverse(), 3/2_r);
    errors += TEST_EQUAL((-2/3_r).inverse(), -3/2_r);

    {
        std::stringstream buf;
        Rational r;
        buf << 1/4_r;
        errors += buf.str() != "1:4";
        buf >> r;
        errors += TEST_EQUAL(r, 1/4_r);
    }
    {
        std::stringstream buf("2:5");
        Rational r;
        buf >> r;
        errors += TEST_EQUAL(r, 2/5_r);
    }

    {
        std::stringstream buf("2bar");
        Rational r;
        buf >> r;
        errors += TEST_EQUAL(r, 0);
    }

    return errors;
}

---

Further suggestions

At present, there's no check for overflow in any of the arithmetic operations. This can happen surprisingly quickly in rational arithmetic, so consider how you might detect or avoid it.

Answer 2

You professor sounds like a ... let's say "piece of work." This code seems really reasonable to me. I even ran the static analyzer on it, and got no complaints beyond what the compiler already suggested. I see 2 minor things (not counting the problems with the header he created) that I would change:

Don't Double #include

The header file #includes <iostream> and so does the source file. Since it's in the header and you're including the header, don't include it again. It just wastes CPU time for the compiler to check.

Use The Correct Return Type (or at least cast it to the correct type)

The gcd() function takes 2 long longs, but returns an int. This causes a warning (at least with llvm) because you could be losing precision if the result is actually larger than fits in an int. Since you can't change the function prototype, you need to at least cast it to an int to get rid of the warning. It would be nice to check to make sure the result is less than or equal to INT_MAX and throw an exception if it's not, but you're probably fine just casting for this homework assignment. It turns out you can change the prototype! So I would have it return a long long instead.

Here are some issues I'd have with the header if it came to me in a code review at work:

Don't use using namespace std

You obviously know this one, and you're right. Enough said.

const Correctness

There is some use of const in the header, but there are many places where it's not used, and that's almost worse than not using it anywhere. For example, in the constructor that takes long longs, why aren't both arguments const? You're not changing them.

Specify Visibility

I program in C++ daily, and I'll be honest, I don't remember what the visibility rules are for members that don't have it specified. Are _p, _q, and simplify() public? Beats me! It's literally 10 characters in the worst case (protected:) to spell it out, so just spell it out! Don't make me think! I need to concentrate on what the code is doing, not on remembering arcane rules of the language that the compiler can't enforce for me.