ProblemI want to create a simple class for storing money - I will be storing dollars/cents as just ints for this model.Idea is to implement various operations on Money class - to be able to add/subtract/multiply/divide money by constant (division is integer for our model - so we don't support money less then a penny - for simplification).Please, comment on the correctness of implementations in terms of - operators overloading, copy/move constructors/numbers manipulations and general code style and quality.I have a few tests passing for my class here - so this is fully functional (except overflow checks - marked as TODO - would be great if someone could recommend industry standard for overflow/underflow checks in cpp which is used everywhere, safe and fast).Money.h#ifndef CHANGE_MONEY_H_#define CHANGE_MONEY_H_#include <exception>#include <iostream>#include <string>namespace change {    class Money {    private:        int32_t whole;        int8_t fraction;        void swap(Money other);    public:        Money(const Money& other);        Money(const Money&& other);        Money(int32_t _whole, int8_t _fraction);        ~Money();        int32_t getWhole() const { return whole; }        int8_t getFraction() const { return fraction; }        Money& operator=(const Money& other) {            if (&other != this) {                whole = other.whole;                fraction = other.fraction;            }            return *this;        }        bool operator==(const Money& other) const {            return whole == other.whole && fraction == other.fraction;        }        bool operator!=(const Money& other) const {            return !(*this == other);        }        bool operator>(const Money& other) const {            if (whole > other.whole) {                return true;            } else if (whole == other.whole) {                return fraction > other.fraction;            } else{                return false;            }        }        bool operator>=(const Money& other) const {            return (*this == other || *this > other);        }        bool operator<(const Money& other) const {            return !(*this == other || *this > other);        }        bool operator<=(const Money& other) const {            return (*this == other || *this < other);        }        Money operator+(Money& other) const {            Money result(0, 0);            int16_t sum = fraction + other.fraction;            int16_t newFrac = static_cast<int16_t>(sum) % 100;            int64_t carry = sum > 100 ? 1 : 0;            // TODO: overflow check            result.whole =  whole + other.whole + carry;            result.fraction = static_cast<int8_t>(newFrac);            return result;        }        Money operator+=(Money other) {            *this = *this + other;            return *this;        }        Money operator-(const Money& other) const {            Money result(0, 0);            int8_t newFrac = (fraction - other.fraction) % 100;            int32_t carry = newFrac < 0 ? -1 : 0;            newFrac = newFrac < 0 ? 100 + newFrac : newFrac;            // TODO: overflow            result.whole = whole - other.whole + carry;            result.fraction = newFrac;            return result;        }        Money operator-=(Money other) {            *this = *this - other;            return *this;        }        template <class T>        Money operator*(T number) const {            // TODO: overflow check            int64_t fracMult = static_cast<int64_t>(fraction) * number;            int8_t newFrac =  static_cast<int8_t>(fracMult % 100);            // TODO: overflow check            int32_t newWhole = whole * number + (fracMult / 100);            Money tmp(newWhole, newFrac);            return tmp;        }        template <class T>        Money operator/(T number) const {            if (number == 0) {                throw std::invalid_argument(Division by zero);            }            // TODO: overflow check            int64_t total = (100 * static_cast<int64_t>(whole)) + fraction;            int64_t result = total / number;            int8_t newFrac = static_cast<int8_t>(result % 100);            int32_t newWhole = static_cast<int32_t>(result / 100);            Money tmp(newWhole, newFrac);            return tmp;        }        friend std::ostream& operator<<(std::ostream& os, const Money& money);    };}#endif //CHANGE_MONEY_H_Money.cpp#include Money.h#include <iomanip>namespace change {    Money::Money(const Money& other) {        whole = other.whole;        fraction = other.fraction;    }    Money::Money(const Money&& other) {        *this = std::move(other);    }    Money::Money(int32_t _whole, int8_t _fraction) {        whole = _whole;        fraction = _fraction;    }    Money::~Money() {        // nothing for now    }    void Money::swap(Money other) {        std::swap(whole, other.whole);        std::swap(fraction, other.fraction);    }    // this is just for test purposes - print in dollars always    std::ostream& operator<<(std::ostream& os, const Money& money)  {        return os << '$' << std::to_string(money.whole) << '.' << std::setw(2) << std::setfill('0') << static_cast<int16_t>(money.fraction);    }}
Simple C++ class for storing and manipulations with money
c++;c++11
DesignYou could have made your class much simpler by storing the value as a single value (the number of cent). You only need to convert back into dollars for display to the user (you don't need to keep track of the two values).Code ReviewWhy is swap private?        void swap(Money other);Also you need to pass the parameter by reference. Otherwise you are swapping with a temporary. So your object will get the value of the other object but it will retain its original value.I usually put the Assignment operators with the Constructors/Desrtuctors.        Money(const Money& other);        Money(const Money&& other);        Money(int32_t _whole, int8_t _fraction);        ~Money();With the whole rule of 3/5 they sort of logically group together for construction (with the swap method).Getters are horrible things that break encapsulation.        int32_t getWhole() const { return whole; }        int8_t getFraction() const { return fraction; }Don't use them unless you need too. I see no need to get the individual pieces out of the object.You should use the copy and swap idiom for defining the assignment operator. It simplifies the processes and when you create more complex classes all the hard stuff is already taken care off.        Money& operator=(const Money& other) {            if (&other != this) {                whole = other.whole;                fraction = other.fraction;            }            return *this;        }        // Copy and swap looks like this:        Money& operator=(Money other)  // pass by value to get an         {                              // implicit copy.            other.swap(*this);         // Swap this value with the copy.            return *this;        }                              // destructor of copy handles any                                       // resource management.Also its really simple to read.I also notice you missed the move assignment operator. This means your class will only use copy assignment (which comes into play when you look at your move constructor).If you change the design writing these becomes trivial.        bool operator==(const Money& other) const {            return whole == other.whole && fraction == other.fraction;        }        bool operator!=(const Money& other) const {            return !(*this == other);        }Sure this looks correct.        bool operator>(const Money& other) const {            if (whole > other.whole) {                return true;            } else if (whole == other.whole) {                return fraction > other.fraction;            } else{                return false;            }        }But there is a much simpler and standard way of writing this using tuples.        bool operator>(const Money& other) const {            return std::make_tuple(whole, fraction) > std::make_tuple(other.whole, other.fraction);        }This same technique can be applied to all the other comparison functions. Saves a lot of time fretting that you got it correct.Normally you write operator+= then define operator+ in terms of operator+=. It really makes it very simple.        Money operator+(Money& other) const {            // why define the result here.            // Much simplr and obvious to return a Money object constructed in place.            Money result(0, 0);            int16_t sum = fraction + other.fraction;            int16_t newFrac = static_cast<int16_t>(sum) % 100;            int64_t carry = sum > 100 ? 1 : 0;            // TODO: overflow check            result.whole =  whole + other.whole + carry;            result.fraction = static_cast<int8_t>(newFrac);            return result;            // I would have done.            return Money(sum + newFrac / 100, newFrac % 100); // or something like that.        }        Money operator+=(Money other) {  // Why are you passing by value?            *this = *this + other;            return *this;        }If we did it the other way around it looks like this:        Money operator+=(Money cosnt &other) {            whole    += other.whole;            fraction += other.fraction;            whole    += fraction / 100;            fraction = fraction % 100;            return *this;        }        Money operator+(Money const& other) const {            Money  result(*this);            return result += other;        }OK. Normal Copy constructor:    Money::Money(const Money& other) {        whole = other.whole;        fraction = other.fraction;    }Move constructor has a few issues.    // Move constructor should be a noexcept    // There are certain container operations that are more efficient if    // you make these noexcept because the container is trying to gurantee    // the strong exception gurantee. If your move constructor can throw    // the container must copy its members. If your constructor does not    // throw then it can use a move of the container internals.    // Also you can not pass by const here.    // You are stealing the content of the other object. This usually means    // you have to alter it (so it does not release any resources).    Money::Money(const Money&& other) {        // Also because your class does not have a move         // assignment operator. This means it will default to copy        *this = std::move(other);        // Also note that you don't initialize the members before you        // Try and do the move. This does not matter here because        // you have no resources. But since move is usally defined        // in terms of swap you would be swapping random values into        // the source location.    }But normally its just easier to define the Move constructor and Move assignement in terms of the swap operator.    Money::Money(Money&& other) noexcept        : whole(0)        , fraction(0)    {        other.swap(*this);    }    Money& Money::operator=(Money&& other) noexcept    {        other.swap(*this);        return *this;    }Sure standard constructor.Have you though about default values?Also prefer to use the initializer list for constructor. Its not going to make a difference here. But with any type with a constructor it will. And it pays to be consistent throughout your code base.    Money::Money(int32_t _whole, int8_t _fraction) {        whole = _whole;        fraction = _fraction;    }    // I would do this.    Money::Money(int32_t whole = 0 , int8_t fraction = 0)        : whole(whole)        , fraction(fraction)    {}If your destructor does nothing (or the default thing). Then just let the compiler generate one for you. There is no need to waste space and cognitive resonance in your reader for something that is automatic.    Money::~Money() {        // nothing for now    }Using std::swap is slightly different from anything else. Your code is fine here but for more complex classes you need to make sure you use the correct swap operator and to do this you need the compiler to perform  type dependant look up to get the correct swap function.    void Money::swap(Money other) {        std::swap(whole, other.whole);        std::swap(fraction, other.fraction);    }    // This is what a generic swap should look like.    void Money::swap(Money other)    {        using std::swap;                    // bring std::swap into the current scope        // The only place where you don't prefix the function with `std::`        // Now the compiler will look up the type of the object being swapped        // It will use any type specific swap operation if it exists.        // If this does not exist then it will use the one in the current        // scope which is std::swap because of the using line above.        swap(whole, other.whole);        swap(fraction, other.fraction);    }