Simple C++ class for storing and manipulations with money

Question

Problem

I 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);
    }

}

Answer 1

Design

You 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 Review

Why 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);
    }

Answer 2

Cents make Sense

A much easier way to think about money is to just store everything in cents. That makes all your mathematical operations trivial (just translate to the appropriate operator). This is especially a big deal for things like comparison, where comparing one thing is easy, but comparing multiple things is much harder...

Unnecessary work

Money only has integer types. It doesn't manage any memory. So the default copy/move constructor/assignment and destructor all do the right thing. Let the compiler do its job for you, and do not write these functions!

If you insist, you should just default them:

Money(const Money&) = default;
Money(Money&&) = default;
// etc

Swap doesn't

Your signature for swap is void swap(Money). Internally you're swapping against a temporary, not what is passing in. You need to take the argument t by reference.

Although normal std::swap already does the right thing just as efficiently, so this is unnecessary too

Total Ordering

You have the right idea that you only need to fully implement two functions, but your implementation of most of the comparisons is inefficient. The typical way is to implement:

operator==
operator<

And then implement

a != b   ===   !(a==b)
a > b    ===   b < a
a >= b   ===   !(a < b)
a <= b   ===   !(b < a)

Just one check everywhere is sufficient.

op=

You are implementing += and -= in terms of + and -. This is backwards and much less efficient. You want to implement + in terms of +=, as in:

class Money {
    friend Money operator+(Money lhs, const Money& rhs) {
        return lhs += rhs;
    }
};

Use a mem-initializer list

When you construct, construct the members with the target values directly:

Money::Money(int32_t _whole, int8_t _fraction)
: whole(_whole)
, fraction(_fraction)
{ }

This is more meaningful with more complicated types, but might as well get in the habit now.

Answer 3

This will not localize well to Zimbabwe:

int32_t whole;
int8_t fraction;

The country once dropped twenty-five zeros off of their then-currency (though not all at once). Even a uint64_t only goes up to ~10^19 or so, meaning it would have overflowed many times over. You need to use an arbitrary-precision data type here, or risk problems with hyperinflation in the future. It is entirely possible some other country will experience hyperinflation as bad as or even worse than Zimbabwe did at some point in the future.

Unfortunately, so far as I can tell, arbitrary-precision stuff doesn't currently exist in STL. You will have to either find a library or roll your own. I recommend the former; you might start here.