c++operator-overloadingc++14move-semanticsrational-numbers

Rational class and Move Semantics not working


I have tried to add more features to already given class(the answer of @Aak) in the problem: How to output fraction instead of decimal number? for printing fractions in numerator / denominator .

First of all, the given code was not working without any change in the code. Then I made it working after making some changes. However, my implementation gives me a wrong output.

for example:

input: A = 3;
       B = 3;
Output: 9/1
        9
instead of: 1

here is the complete implementation:

#include <iostream>
/********************** Rational class **********************************/
class Rational
{
private:
    int m_numerator, m_denominator;
private:
    inline void simplificate()
    {
        int commondivisor = 1;
        for(int i=2;i<= std::min(abs(m_numerator), abs(m_denominator));i++)
            if( m_numerator%i == 0 && m_denominator%i == 0 )
                commondivisor = i;
        m_numerator /= commondivisor;
        m_denominator /= commondivisor;
    }

public:
    Rational()                                  // Defualt
        :m_numerator(1), m_denominator(1)
        {}

    Rational(const int& num, const int& den=1)  // Parameterized
        :m_numerator(num), m_denominator(den)
        {}

    Rational(const Rational& other)             // Copy
        :m_numerator(other.m_numerator), m_denominator(other.m_denominator)
        {}

    /*Rational(Rational&& other)                  // Move
        :m_numerator(other.m_numerator), m_denominator(other.m_denominator)
        {}*/

    ~Rational(){}

    Rational& operator/ (const int& divisor)
    {
            m_denominator *= divisor;
            simplificate();
            return *this;
    }
    Rational& operator/ (const Rational &divisor)
    {
            m_numerator *= divisor.m_numerator;
            m_denominator *= divisor.m_denominator;
            simplificate();
            return *this;
    }
    const double getrealformat()const
    {
        return  static_cast<double>(m_numerator)/
                static_cast<double>(m_denominator);
    }

    friend double operator/ (Rational& obj, const int& divisor);
    friend void printRational(Rational& obj, const int& A, const int& B);
    friend void printRational(Rational& obj, const int&& A, const int&& B);
};
/************************** Friend functions ********************************/
double operator/ (Rational& obj, const int& divisor)
{
    obj.m_denominator *= divisor;
    obj.simplificate();
    return obj.getrealformat();
}
void printRational(Rational& obj, const int& A, const int& B)   // lvalue
{
    Rational r1(A), r2(B);
    obj = r1/r2;
    std::cout<<obj.m_numerator<<'/'<<obj.m_denominator<<std::endl;
    std::cout<<obj.getrealformat()<<std::endl;
}
void printRational(Rational& obj, const int&& A, const int&& B)  // rvalue
{
    Rational r1(A), r2(B);
    obj = r1/r2;
    std::cout<<obj.m_numerator<<'/'<<obj.m_denominator<<std::endl;
    std::cout<<obj.getrealformat()<<std::endl;
}
/*****************************************************************************/
int main()
{
    Rational obj;
    printRational(obj, 3,3);
    return 0;
}

Question - 1: the logic looks fine, but I don't know why I am getting the wrong answer. Can anybody find the problem?

Question - 2: I have written "Move" constructor for the class, which you can find in the commented section. However, I could not use it because of following error:

D:\Programming\C++\CPP Programs\Class - Fractions\Class - Fractions.cpp|70|error: use of deleted function 'Rational& Rational::operator=(const Rational&)'|

D:\Programming\C++\CPP Programs\Class - Fractions\Class - Fractions.cpp|77|error: use of deleted function 'Rational& Rational::operator=(const Rational&)'|

(whenever it has been called the moved object/instance is destroyed, to my knowledge.)

can anybody help me to implement the Move constructor for this class?


Solution

  • Look your operator/()

    Rational& operator/ (const Rational &divisor)
    {
            m_numerator *= divisor.m_numerator;
            m_denominator *= divisor.m_denominator;
            simplificate();
            return *this;
    }
    

    This code is correct for operator*(), not for operator/().

    Maybe

            m_numerator *= divisor.m_denominator;
            m_denominator *= divisor.m_numerator;
    

    But is worse that you're operator/() modify the object.

    Your code (corrected switching numerator and denominator) should be correct for operator/=(), not for operator/() that should return a new object.

    I suggest something as follows

    Rational& operator/= (const Rational &divisor)
    {
            m_numerator *= divisor.m_denominator;
            m_denominator *= divisor.m_numerator;
            simplificate();
            return *this;
    }
    
    friend Rational operator/ (Rational A, Rational const & B);
    

    and, outside the class,

    Rational operator/ (Rational A, Rational const & B)
     { return A/=B; }
    

    Regarding question 2 ("I have written "Move" constructor for the class, [...]However, I could not use it because of following error"), you can see in this page that

    A implicitly-declared copy assignment operator for class T is defined as deleted if any of the following is true:

    • T has a user-declared move constructor;
    • T has a user-declared move assignment operator.

    So, when you define the move contructor, you delete the implicit copy operator.

    You can solve the problem adding

     Rational & operator= (Rational const &) = default;
    

    reactivating the implicit copy constructor