4
\$\begingroup\$

I am creating a Number class having Integer and Fraction derive class which add/compare different kinds of numbers. In addition, the caller need not needs to know what kind of numbers are being added. For example, the solutions expected to be able to do the following:

  • \$\frac{1}{2} + 2 = \frac{5}{2}\$ (fraction)
  • \$\frac{2}{6} + \frac{2}{3} = 1\$ (integer)

The solution creates three classes: Number, Integer and Fraction and overloads the + and == operators.

Can someone please review my solution? I particularly wanted to know about the c++ design and how I can improve it. Also, is the use of unique_ptr correct?

Factory.h

#pragma once
#include "Number.h"
#include <memory>
class Factory
{
public:

    std::unique_ptr<Number> createObj(int a);
    std::unique_ptr<Number> createObj(int a, int b);
    Factory() {}
    ~Factory() {}

};

Factory.cpp

#include "Factory.h"
#include "Integer.h"
#include "Fraction.h"
#include "Utility.h"


std::unique_ptr<Number> Factory::createObj(int a)
{
    if (utility::integeroverflow(a))
    {
        return make_unique<Integer>(a);
    }
}

std::unique_ptr<Number> Factory::createObj(int a, int b)
{   
    if (utility::integeroverflow(a))
    {
        if (utility::dividebyzero(b))
        {
            return make_unique<Fraction>(a, b);
        }
    }
}

Number.h

#ifndef NUMBER_CLASS_H
#define NUMBER_CLASS_H

#include <iostream>
#include <memory>
using namespace std;


class Integer;
class Fraction;

class Number
{

public:
    virtual void display(void) =0;

    virtual Integer * isInteger() { return NULL; }
    virtual Fraction * isFraction() { return NULL; }

    std::unique_ptr<Number> operator+ ( Number &);
    bool operator==( Number &);

    virtual ~Number() {};


};

#endif

Number.cpp

#include "Number.h"
#include "Integer.h"
#include "Fraction.h"


// overloading "=="
bool Number::operator==(Number &rhs)
{
    Integer *intPtr1 = this->isInteger();
    Integer *intPtr2 = rhs.isInteger();

    Fraction *fracPtr1 = this->isFraction();
    Fraction *fracPtr2 = rhs.isFraction();

    // checking of two integers
    if (intPtr1 && intPtr2)
    {
        if (*intPtr1 == *intPtr2)
            return true;
        else
            return false;
    }

    // checking of two fraction
    else if (fracPtr1 && fracPtr2)
    {
        if (*fracPtr1 == *fracPtr2)
            return true;
        else
            return false;
    }

    // checking of one integer and one fraction
    else if (intPtr1 && fracPtr2)
    {
        if (*intPtr1 == *fracPtr2)
            return true;
        else
            return false;
    }

    // checking of fraction and integer the last case
    else
    {
        if (*fracPtr1 == *intPtr2)
            return true;
        else
            return false;
    }

    return false;

}

std::unique_ptr<Number> Number::operator+ ( Number& rhs) 
{
    Integer *intPtr1 = this->isInteger();
    Integer *intPtr2 = rhs.isInteger();

    Fraction *fracPtr1 = this->isFraction();
    Fraction *fracPtr2 = rhs.isFraction();

    std::unique_ptr<Number> sumNumberObj = nullptr;

    // addition of two integers
    if (intPtr1 && intPtr2)
        sumNumberObj = *intPtr1 + *intPtr2;

    // addition of two fraction
    else if (fracPtr1 && fracPtr2)
        sumNumberObj = *fracPtr1 + *fracPtr2;
    // addition of one integer and one fraction
    else if (intPtr1 && fracPtr2)
        sumNumberObj = *intPtr1 + *fracPtr2;

    // addition of fraction and integer the lase case
    else
        sumNumberObj = *fracPtr1 + *intPtr2;

    return std::move(sumNumberObj);

}

Integer.h

#ifndef INTEGER_CLASS_H
#define INTEGER_CLASS_H


#include "Number.h"

class Integer : public Number
{
    int intValue;

public:
    void display(void) { cout << this->intValue; }

    int getValue() const { return this->intValue; }
    void setValue(int x) { this->intValue = x; }

    Integer() :intValue(0) {};
    Integer(int num) :intValue(num) {};
    ~Integer() {};

    std::unique_ptr<Number> operator+ (const Integer &) const ;
    std::unique_ptr<Number> operator+ (const Fraction &) const;

    bool operator==(const Fraction &) const;
    bool operator==(const Integer &) const;

    virtual Integer* isInteger() { return this; }


};
#endif

Integer.cpp

#include "Number.h"
#include "Integer.h"
#include "Fraction.h"
#include "Utility.h"
#include <stdexcept>


// operator "+" overloading
std::unique_ptr<Number> Integer::operator+(const Integer &secondNumber) const
{
    int result = utility::safeaddition(this->intValue, secondNumber.intValue);
     return std::make_unique<Integer>(result);
}
// operator "+" overloading INTEGER + Fraction
std::unique_ptr<Number> Integer::operator+(const Fraction& secondNumber) const
{

    int num = secondNumber.getnumerator().getValue();
    int den = secondNumber.getdenominator().getValue();
    int numoriginal = this->getValue();
    int mult = utility::safemultiplication(numoriginal,den);
    int numeratorVal = utility::safeaddition(mult, num);
    int denominatorVal = den;

     return std::make_unique<Fraction>(numeratorVal, denominatorVal);

}

// comparasion operator overload
bool Integer::operator==(const Fraction& rhs) const
{
    int numFrac = rhs.getnumerator().getValue();
    int denFrac = rhs.getdenominator().getValue();
    int numVal = this->getValue();
    if (numVal == (numFrac / denFrac))
        return true;
    return false;
}
// Coparision Integer Integer
bool Integer::operator==(const Integer& rhs) const
{
    return this->intValue == rhs.getValue();
}

Fraction.h

#ifndef FRAC_CLASS_H
#define FRAC_CLASS_H

#include "Number.h"
#include "Integer.h"

    class Fraction : public Number
    {
        Integer numerator;
        Integer denominator;

    public:

        void display(void);

        Fraction() {};
        Fraction(const Integer &num, const Integer &den) : numerator(num), denominator(den) {}
        ~Fraction() {};

        std::unique_ptr<Number> operator+ (const Integer&) const;
        std::unique_ptr<Number> operator+ (const Fraction&) const;

        bool operator==(const Integer&) const;
        bool operator==(const Fraction&)const;

        // int gcd(int  a, int  b);
        // int lcm(const int  a, const int  b);

        Integer getnumerator() const;
        Integer getdenominator()const;

        virtual Fraction * isFraction() { return this; }


    };

#endif

Fraction.cpp

#include "Number.h"
#include "Integer.h"
#include "Fraction.h"
#include "Utility.h"
#include <stdexcept>

    // display the value of number on output console
    void Fraction::display(void)
    {
        int temp = this->denominator.getValue();
        std::cout << this->numerator.getValue() << "/" << this->denominator.getValue();
    }

    // "+" operator overloading for fraction and fraction

    std::unique_ptr<Number> Fraction::operator+ (const Fraction& numberTwo) const
    {
        int Lcm = utility::lcm(this->denominator.getValue(), numberTwo.denominator.getValue());
        int temp1 = Lcm / this->denominator.getValue();
        int temp2 = Lcm / numberTwo.denominator.getValue();
        int temp3 = utility::safemultiplication(this->numerator.getValue(),temp1);
        int temp4 = utility::safemultiplication(numberTwo.numerator.getValue(), temp2);
        int temp5 = utility::safeaddition(temp3, temp4);

        return std::make_unique<Fraction>(static_cast<int>(temp5), Lcm);

    }

    // "+" operator overloading for fraction and integer
    std::unique_ptr<Number> Fraction::operator+(const Integer &secondNumber) const
    {
        int num = this->getnumerator().getValue();
        int den = this->getdenominator().getValue();
        int numoriginal = secondNumber.getValue();
        int numeratorVal = utility::safemultiplication(numoriginal, den);
        int temp = utility::safeaddition(numeratorVal, num);
        return std::make_unique<Fraction>(temp, den);

    }
    // comparision operator overload for fraction and fraction
    bool Fraction::operator==(const Fraction& rhs) const
    {
        Integer numCheck = this->numerator;
        Integer denCheck = this->denominator;
        if (rhs.numerator.getValue())
            numCheck.setValue(numCheck.getValue() / rhs.numerator.getValue());
        if (rhs.denominator.getValue())
            denCheck.setValue(denCheck.getValue() / rhs.denominator.getValue());
        if (numCheck == denCheck) {
            return true;
        }
        return false;

    }

    // comparision operator overload for fraction and integer
    bool Fraction::operator==(const Integer& rhs) const
    {
        int numFrac = this->getnumerator().getValue();
        int denFrac = this->getdenominator().getValue();
        int numVal = rhs.getValue();
        if (numVal == (numFrac / denFrac))
            return true;
        return false;

    }

    // getter for numberator
    Integer Fraction::getnumerator() const
    {
        return this->numerator;

    }

    // getter for denominator
    Integer Fraction::getdenominator() const
    {
        return this->denominator;

    }

Utility.h

#pragma once

namespace utility
{

    int safeaddition(int, int);
    int safemultiplication(int, int);
    bool dividebyzero(int) ;
    int gcd(int, int);
    int lcm(int, int) ;
    bool integeroverflow(int a);
}

Utility.cpp

#include <iostream>
#include "Utility.h"
#include <stdexcept>

int utility::gcd(int a, int b)
{
    int temp;
    while (b != 0) {
        temp = b;
        b = a%b;
        a = temp;
    }
    return a;
}

int utility::lcm(int a, int b) 
{
    int temp = gcd(a, b);
    return temp ? (a/temp * b) : 0;

}

int utility::safeaddition (int a, int b) 
{
    long long result = static_cast<long long>(a) + static_cast<long long>(b);
    if ((result > INT_MAX) || (result < INT_MIN)) // overflow
        throw std::overflow_error("Exception:Integer overflow");
    else
        return static_cast<int>(result);
}

int utility::safemultiplication(int a, int b) 
{
    long long result = static_cast<long long>(a)*static_cast<long long>(b);
    if ((result > INT_MAX) || (result < INT_MIN)) // overflow
        throw std::overflow_error("Exception:Integer overflow");
    else
        return static_cast<int>(result);
}

bool utility::dividebyzero(int a)
{
    if (a == 0) 
        throw std::overflow_error("Exception: Denominator cannot be zero");
    else if((a > INT_MAX) || (a < INT_MIN)) // overflow
        throw std::overflow_error("Exception:Integer overflow");
    else
        return true;
}

bool utility::integeroverflow(int a)
{
    if ((a > INT_MAX) || (a < INT_MIN)) // overflow
        throw std::overflow_error("Exception:Integer overflow");
    else
        return true;
}

main.cpp

#include "Number.h"
#include <memory>
#include "Factory.h"
#include <stdexcept>


int main()
{

    try
    {

        std::unique_ptr<Factory> facObj(new Factory);

        std::unique_ptr<Number> num1(facObj->createObj(9));
        std::unique_ptr<Number> num2(facObj->createObj(1));

        cout << "Test case 1 : Addition of two Integer" << endl;

        num1->display();
        cout << " + ";
        num2->display();
        cout << " : ";
        auto result1 = *num1 + *num2;
        result1->display();
        cout << endl;

        cout << "Test case 2 : Equality of two Integer" << endl;
        num1->display();
        cout << " = ";
        num2->display();
        cout << " : ";
        if (*num1 == *num2)
            cout << "Numebers are equal!" << endl;
        else
            cout << "Numebers are NOT equal!" << endl;
        cout << endl;

        cout << "Test case 3 : Addition of two Fraction" << endl;
        std::unique_ptr<Number> frac1(facObj->createObj(2,3));
        std::unique_ptr<Number> frac2(facObj->createObj(1,2));

        frac1->display();
        cout << " + ";
        frac2->display();
        cout << " : ";
        auto result2 = *frac1 + *frac2;
        result2->display();
        cout << endl;

        cout << "Test case 4 : Equality of two Fraction" << endl;
        frac1->display();
        cout << " = ";
        frac2->display();
        cout << " : ";
        if (*frac1 == *frac2)
            cout << "Numebers are equal!" << endl;
        else
            cout << "Numebers are NOT equal!" << endl;
        cout << endl;

        cout << "Test case 5 : Addition of Integer and Fraction" << endl;
        num1->display();
        cout << " + ";
        frac1->display();
        cout << " : ";
        auto result3 = *num1 + *frac1;
        result3->display();
        cout << endl;

        cout << "Test case 6 : Equality of Integer and Fraction" << endl;
        num1->display();
        cout << " = ";
        frac2->display();
        cout << " : ";
        if (*num1 == *frac2)
            cout << "Numebers are equal!" << endl;
        else
            cout << "Numebers are NOT equal!" << endl;
        cout << endl;

        cout << "Test case 7 : Addition of Fraction and Integer" << endl;
        frac2->display();
        cout << " +";
        num2->display();
        cout << " : ";
        auto result4 = *frac2 + *num2;
        result4->display();
        cout << endl;


        cout << "Test case 8 : Equality of Fraction and Integer" << endl;
        frac2->display();
        cout << " = ";
        num1->display();
        cout << " : ";
        if (*frac2 == *num1)
            cout << "Numebers are equal!" << endl;
        else
            cout << "Numebers are NOT equal!" << endl;
        cout << endl;


        cout << "Test case 9 : Equality of Integer and Fraction : Positive " << endl;

        std::unique_ptr<Number> num3(facObj->createObj(5));
        std::unique_ptr<Number> frac4(facObj->createObj(5,1));

        num3->display();
        cout << " = ";
        frac4->display();
        cout << " : ";
        if (*num3 == *frac4)
            cout << "Numebers are equal!" << endl;
        else
            cout << "Numebers are NOT equal!" << endl;
        cout << endl;

    }

    catch (std::exception e)
    {
        cout << "\n" << e.what() << "\n";
        return 1;
    }

    getchar();
    return 0;

}
\$\endgroup\$
3
\$\begingroup\$

I think you've got too much code there; certainly I didn't read all of it. But I'll take you on my journey through the parts I did read.

std::unique_ptr<Number> Factory::createObj(int a, int b)
{   
    if (utility::integeroverflow(a))
    {
        if (utility::dividebyzero(b))
        {
            return make_unique<Fraction>(a, b);
        }
    }
}

At first glance this seems nice and straightforward... it looks like you're using a classical-OOP class hierarchy where a Fraction is-a Number, which means you have to use heap allocation and unique_ptrs all over the place. Now, I personally don't like classical-OOP style in C++ (because it requires all that heap allocation), but sure, as a learning exercise, go for it. Your use of unique_ptr is indeed correct, as far as that goes.

However, I would expect a good programmer to write

if (a && b) {
    ...
}

instead of

if (a) {
    if (b) {
        ...
    }
}

But hang on, what is this if integeroverflow, if dividebyzero? That's actually quite unusual control flow you've got there! I infer that utility::dividebyzero is going to be some sort of guard against division by zero... but I can't figure out how you're going to make it work. Well, let's go look at it. (Actually, I looked at integeroverflow first.)

bool utility::integeroverflow(int a)
{
    if ((a > INT_MAX) || (a < INT_MIN)) // overflow
        throw std::overflow_error("Exception:Integer overflow");
    else
        return true;
}

Red flag. An int can never be greater than INT_MAX nor less than INT_MIN. And indeed, if you compile this with clang++ -O2, you get:

__ZN7utility15integeroverflowEi:
        pushq   %rbp
        movq    %rsp, %rbp
        movb    $1, %al
        popq    %rbp
        retq

The compiler is smart enough to know that this function is a no-op that always returns true.

So, you've got some fundamental misconceptions about what an int is in C++. On your platform, it's a 32-bit quantity, and it has no "not-a-number" values — it's not like floating-point. You should go read up on bits and bytes, and maybe start your math programming with something a lot simpler, like the old classic "Fahrenheit to Celsius temperature converter".


Furthermore, once I saw what you were trying to do, it was apparent that utility::dividebyzero(b) wouldn't be good enough either. Try the expression *Factory::createObj(1, -1) + *Factory::createObj(1, INT_MIN)... can you figure out why it crashes?


Furthermore, once I actually started writing up this answer, and had to cut-and-paste the code above, I noticed something that your compiler already warned you about — you should go fix all the compiler warnings immediately! Don't think "oh, I know what I'm doing, the compiler is wrong." At your level of experience, the compiler is always right, with a margin of error of plus or minus 1 in a billion.

The function Factory::createObj(int a, int b) doesn't return a value along all of its possible codepaths. You can fix this by adding return std::make_unique<Fraction>(1,1); at the bottom, or by adding __builtin_unreachable(); at the bottom, or by refactoring your dividebyzero etc functions to be assertions rather than conditions, i.e.

std::unique_ptr<Number> Factory::createObj(int a, int b)
{
    utility::throw_if_overflow(a);  // see above as to why this is silly
    utility::throw_if_zero(b);
    return std::make_unique<Fraction>(a, b);
}

Not only will the compiler be happy with this, but it'll generate more efficient code (in unoptimized builds), and it's several lines shorter as well, which means you can fit more code on your screen, which means you can keep more of your program in view at one time. (I also picked you some clearer function names while I was at it.)


Anyway, that's all I bothered to look at, since I assume most of the mass of code you posted is occupied with nonsense similar to utility::integeroverflow(int).

BTW, if you can find a used copy of The Elements of Programming Style, I highly recommend picking it up and reading it cover to cover. You'll learn a lot about programming, and the bonus is that it'll be harsh critiques of other people's code instead of yours. ;)

\$\endgroup\$
2
\$\begingroup\$

In addition to the excellent suggestions by @Quuxplusone in their answer, I suggest the following:

1. Use consistent style to ensure a .h file is #included only once

In Fraction.h and Utility.h, you are using

#pragma once

while in Number.h, Fraction.h, and Integer.h, you are using #include guards such as:

#ifndef NUMBER_CLASS_H
#define NUMBER_CLASS_H

...

#endif

I recommend using a consistent style that uses both the elements

#pragma once
#ifndef FACTORY_H
#define FACTORY_H

...

#endif

This will make sure that on platforms that support #pragma once, the .h file will not even be opened more than once. On platforms that don't support #pragma once, the contents of the file will be included in a compilation unit only once even if they are opened multiple times.

Also, I don't see any reason to use NUMBER_CLASS_H. You can be consistent even on that front by using NUMBER_H.

#pragma once
#ifndef NUMBER_H
#define NUMBER_H

...

#endif

2. Avoid std::make_unique

Since you tagged your question c++11, I want to mention that std::make_unique is a C++14 feature, not a C++11 feature. If you want your code to work with C++11 compiler, I suggest changing:

return std::make_unique<Integer>(a);

to

return std::unique_ptr<Integer>(new Integer(a));

Replace all usages of std::make_unique similarly.

3. Prefer use of std::numeric_limits member functions over use of INT_MAX and INT_MIN

Instead of INT_MAX, use std::numeric_limits<int>::max().
Instead of INT_MIN, use std::numeric_limits<int>::min().

INT_MAX and INT_MIN were good while they lasted. We now have std::numerical_limits that has many other features that might be helpful in other usages. It will be good to get familiar with it.

4. Divide the contents of main into many functions

Having a large function is harder to debug and maintain. I suggest dividing the contents of main into multiple functions.

void test1()
{

   cout << "Test case 1 : Addition of two Integer" << endl;

   std::unique_ptr<Factory> facObj(new Factory);
   std::unique_ptr<Number> num1(facObj->createObj(9));
   std::unique_ptr<Number> num2(facObj->createObj(1));

   num1->display();
   cout << " + ";
   num2->display();
   cout << " : ";
   auto result1 = *num1 + *num2;
   result1->display();
   cout << endl;
}

void test2()
{
   cout << "Test case 2 : Equality of two Integer" << endl;

   std::unique_ptr<Factory> facObj(new Factory);
   std::unique_ptr<Number> num1(facObj->createObj(9));
   std::unique_ptr<Number> num2(facObj->createObj(1));

   num1->display();
   cout << " = ";
   num2->display();
   cout << " : ";
   if (*num1 == *num2)
      cout << "Numebers are equal!" << endl;
   else
      cout << "Numebers are NOT equal!" << endl;
   cout << endl;
}

void test3()
{
   cout << "Test case 3 : Addition of two Fraction" << endl;

   std::unique_ptr<Factory> facObj(new Factory);
   std::unique_ptr<Number> frac1(facObj->createObj(2,3));
   std::unique_ptr<Number> frac2(facObj->createObj(1,2));

   frac1->display();
   cout << " + ";
   frac2->display();
   cout << " : ";
   auto result2 = *frac1 + *frac2;
   result2->display();
   cout << endl;
}

void test4()
{
   cout << "Test case 4 : Equality of two Fraction" << endl;

   std::unique_ptr<Factory> facObj(new Factory);
   std::unique_ptr<Number> frac1(facObj->createObj(2,3));
   std::unique_ptr<Number> frac2(facObj->createObj(1,2));

   frac1->display();
   cout << " = ";
   frac2->display();
   cout << " : ";
   if (*frac1 == *frac2)
      cout << "Numebers are equal!" << endl;
   else
      cout << "Numebers are NOT equal!" << endl;
   cout << endl;
}

void test5()
{
   cout << "Test case 5 : Addition of Integer and Fraction" << endl;

   std::unique_ptr<Factory> facObj(new Factory);
   std::unique_ptr<Number> num1(facObj->createObj(9));
   std::unique_ptr<Number> frac1(facObj->createObj(2,3));

   num1->display();
   cout << " + ";
   frac1->display();
   cout << " : ";
   auto result3 = *num1 + *frac1;
   result3->display();
   cout << endl;
}

void test6()
{
   cout << "Test case 6 : Equality of Integer and Fraction" << endl;

   std::unique_ptr<Factory> facObj(new Factory);
   std::unique_ptr<Number> num1(facObj->createObj(9));
   std::unique_ptr<Number> frac2(facObj->createObj(1,2));

   num1->display();
   cout << " = ";
   frac2->display();
   cout << " : ";
   if (*num1 == *frac2)
      cout << "Numebers are equal!" << endl;
   else
      cout << "Numebers are NOT equal!" << endl;
   cout << endl;
}

void test7()
{
   cout << "Test case 7 : Addition of Fraction and Integer" << endl;

   std::unique_ptr<Factory> facObj(new Factory);
   std::unique_ptr<Number> num2(facObj->createObj(1));
   std::unique_ptr<Number> frac2(facObj->createObj(1,2));

   frac2->display();
   cout << " +";
   num2->display();
   cout << " : ";
   auto result4 = *frac2 + *num2;
   result4->display();
   cout << endl;
}

void test8()
{
   cout << "Test case 8 : Equality of Fraction and Integer" << endl;

   std::unique_ptr<Factory> facObj(new Factory);
   std::unique_ptr<Number> num1(facObj->createObj(9));
   std::unique_ptr<Number> frac2(facObj->createObj(1,2));

   frac2->display();
   cout << " = ";
   num1->display();
   cout << " : ";
   if (*frac2 == *num1)
      cout << "Numebers are equal!" << endl;
   else
      cout << "Numebers are NOT equal!" << endl;
   cout << endl;
}

void test9()
{
   cout << "Test case 9 : Equality of Integer and Fraction : Positive " << endl;

   std::unique_ptr<Factory> facObj(new Factory);
   std::unique_ptr<Number> num3(facObj->createObj(5));
   std::unique_ptr<Number> frac4(facObj->createObj(5,1));

   num3->display();
   cout << " = ";
   frac4->display();
   cout << " : ";
   if (*num3 == *frac4)
      cout << "Numebers are equal!" << endl;
   else
      cout << "Numebers are NOT equal!" << endl;
   cout << endl;

}

int main()
{
   try
   {
      test1();
      test2();
      test3();
      test4();
      test5();
      test6();
      test7();
      test8();
      test9();
   }

   catch (std::exception e)
   {
      cout << "\n" << e.what() << "\n";
      return 1;
   }

   getchar();
   return 0;

}

This allows you to quickly comment out tests that you don't want to deal with while debugging.

\$\endgroup\$

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.