C++ money class for basic banking application

Question

I wanted to get my code reviewed as I have tried to implement a basic Money class to represent money values in c++. Can anyone tell me if I am implementing my basic arithmetic operator overloads incorrectly? or if there are any c++ best practices I am breaking since I am new to c++ but tried to do as much research as possible. Thank you.

Money.h

class Money {
private:
    long pounds;
    int pence;

public:
    Money() : pounds(0), pence(0) {}
    Money(const long& pounds, const int& pence) : pounds(pounds), pence(pence) {}
    Money(const long& pounds) : pounds(pounds), pence(0) {}

    // Overload operators to allow easier arithmetic of money objects
    Money operator+(const Money& moneyRhs) const; //constructor for addition  
    Money operator-(const Money& moneyRhs) const; //constructor for subtraction 
    Money operator*(const Money& moneyRhs) const; //constructor for multiply
    Money operator/(const Money& moneyRhs) const; //constructor for division

    // toString method to print out money object
    std::string toString() const;

    long getPounds() const;
    int getPence() const;
};

Money.cpp

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


Money Money::operator+(const Money& moneyRhs) const
{
    const long poundsInPence = (this->pounds + moneyRhs.pounds) * 100;
    const long totalPence = this->pence + moneyRhs.pence;
    const long allPence = poundsInPence + totalPence;

    const Money m3 = Money(allPence / 100, allPence % 100);
    return m3;
}

Money Money::operator-(const Money& moneyRhs) const
{
    const long poundsInPence = (this->pounds - moneyRhs.pounds) * 100;
    const long totalPence = this->pence - moneyRhs.pence;
    const long allPence = poundsInPence + totalPence;

    const Money m3 = Money(allPence / 100, allPence % 100);
    return m3;
}

Money Money::operator*(const Money& moneyRhs) const {
    const long poundsInPence = (this->pounds * moneyRhs.pounds) * 100;
    const long totalPence = this->pence * moneyRhs.pence;
    const long allPence = poundsInPence + totalPence;

    const Money m3 = Money(allPence / 100, allPence % 100);
    return m3;
}

Money Money::operator/(const Money& moneyRhs) const {
    const long poundsInPence = (this->pounds / moneyRhs.pounds) * 100;
    const long totalPence = this->pence / moneyRhs.pence;
    const long allPence = poundsInPence + totalPence;

    const Money m3 = Money(allPence / 100, allPence % 100);
    return m3;
}

std::string Money::toString() const {
    std::string stringFormat = std::to_string(getPounds()) + "." + std::to_string(getPence());
    return stringFormat;
}

long Money::getPounds() const {
    return this->pounds;
}

int Money::getPence() const {
    return this->pence;
}

Answer 1

Consider not storing pounds and pence separately

There are some issues with having separate variables for pounds and pence. Apart from you having to convert to "allPence" format and back, what happens if the value of pence is negative? Or if pence is equal to or larger than 100? Your constructor allows you to create Money object with a negative value for pence. But even if you didn't allow it, consider that if you subtract two Moneys from each other, the second being larger than the first, then allPence will be negative, which means both allPence / 100 and allPence % 100 will be negative (see this StackOverflow question for why that is).

The solution is to store just the value in pence. This makes the arithmetic operators mostly trivial, and you only need to convert to and from pound+pence format in the constructor, in toString(), getPounds() and getPence().

Multiplying and dividing money

What does it mean if I multiply 5 pounds by 3 pence? You would never multiply money by another amount of money in real life, you would instead just say that you multiply 5 pounds by 3. So you rather want some way to multiply Money by a regular integer number, or perhaps even by a floating point value. So for multiplication and division, you want something like:

Money operator*(double rhs) const;

But that would only allow you to write Money(5) * 0.03, it would fail for 0.03 * Money(5). The solution to that is to overload the operators using friend functions, for example like so:

class Money {
    long allPence;

public:
    ...
    friend Money operator*(Money lhs, double rhs);
    friend Money operator*(double lhs, Money rhs);
    ...
};

Money operator*(Money lhs, double rhs) {
    lhs.allPence *= rhs;
    return lhs;
}

Money operator*(double lhs, Money rhs) {
    rhs.allPence *= lhs;
    return rhs;
}

Division is more tricky. You can ask "I have 15 pounds and I want to split it among 3 friends. How much does each friend get?" The answer should be 5 pounds. But another valid question is "I have 15 pounds and a bread costs 3 pounds. How many breads can I buy?" The answer is 5, not 5 pounds. So you want to be able to divide money by money and get a dimensionless value, or divide money by a dimensionless value and get money.

Make the non-default constructors explicit

One problem with a constructor that takes a long as an argument is that it allows implicit conversion of a long to a Money. For example, consider:

Money m = Money(1) + 2;

This makes it easy to make mistakes. It is better to make the constructor explicit:

explicit Money(const long& pounds): ...

Then the line of code above would fail to compile, instead you would have to write:

Money m = Money(1) + Money(2);

Missing unary operator-

It might make sense to be able to negate the amount of money in some calculations, so also consider implementing the unary operator-:

Money Money::operator-() const {
    Money result;
    result.allPence = -this->allPence;
    return result;
}

Consider adding I/O operators

It would be nice if you could write:

Money money(42);
std::cout << "I have " << money << " in my wallet.\n";

You can do this by overloading operator<<() for std::ostream objects. It's really not that hard, it looks like:

class Money {
    ...
    friend std::ostream& operator<<(std::ostream& out, const Money &money);
    ...
};

std::ostream& operator<<(std::ostream& out, const Money &money) {
    return out << money.toString();
}

You can also do this for operator>> to be able to read money values directly from input, but that's a bit more than a one-liner.

Internationalization and other issues

Your code explicitly mentions pounds and pence, but there are other currencies in the world. Not every currency has the same concepts as pounds and pence (1/100ths of a pound). Different currencies are printed in different ways. When doing arithmetic with money, there are different ways to handle rounding. Sometimes you are doing more complicated computations and you want to store intermediate results with more precision than just pence. Basically, it's a lot more complicated than what is covered by your Money class.

The C++ standard library has some support for formatting currencies (for example, using std::put_money() and std::get_money(), but it doesn't have a built in type to store an amount of money.

You might want to look at existing libraries that implement a money type to see how they are handling these issues, for example Marius Bancila's moneycpp library.

Answer 2

First, let me applaud you for doing this in the first place. It's amazing to me that not only does the standard not have a good Decimal library, but there is no Boost version or common independant library that's commonly used! I guess Bloomberg isn't sharing what they are presumably using. It's great that you recognise the issue: don't use floating point for money and are addressing that with a proper class.

---

// Overload operators to allow easier arithmetic of money objects
    Money operator+(const Money& moneyRhs) const; //constructor for addition  
    Money operator-(const Money& moneyRhs) const; //constructor for subtraction 
    Money operator*(const Money& moneyRhs) const; //constructor for multiply
    Money operator/(const Money& moneyRhs) const; //constructor for division

First of all, what is the comment trying to say? These are not constructors. We know that + is for addition, etc.

Implementing one-argument members introduces an asymmetry that we want to avoid in a class that does arithmetic. The right hand argument will be implicitly converted, but the left (the implicit this) will not.

The normal thing to do is implement += as a member function, and then implement a non-member operator+ that works by calling +. Current wisdom is to make this a "hidden friend" for best performance in overload resolution, but it doesn't actually need the private access.

---

More troubling is the operation you actually defined. Adding two Money is fine. But what do you get if you multiply 5 Pounds × 3 Pounds? It should be 15 Square Pounds. You have it returning (just) Pounds.

Similarly, diving two Pounds values should give a dimensionless result, not Pounds. 150 Pounds ÷ 15 Pounds should be a ratio of 10 (a pure number), not 10 Pounds.

You should be able to multiply and divide by pure numbers, too. You should be able to write 5 Pounds * 3 to get a result of 15 Pounds. And here's an example of symmetry that I mentioned earlier: It should work just as well to write 3 * 5 Pounds. Clearly, that's not a member function of the int!

The deeper issue here is that this class is doing two different things at the same time. It's a non-integer exact math class; and it's a Unit. Those responsibilities should be separated into distinct (separately reusable) classes.

Now I already said you don't really multiply to Money variables. But you do want to multiply Money by other non-integer values, such as interest rates, discounts, and taxes. These may not have the same limitation on the number of digits as the Money itself! For example, you might need 0.01457 as a rate to be applied. This must be done to the full accuracy before rounding back to pense.

---

You have had some suggestions on the internal representation. I'd like to stress that the API design is more important, and it should be possible to change the private data representation without affecting the meaning and usage of the class. So, concentrate on the design of the class. There may be various reasons for choosing one representation over another, such as compatibility with the data as it was being held before introducing this class. In fact, you could have more than one implementation that behave identically.

I mentioned separation of non integer exact representation from Units earlier. In my own Enterprise-level code, I created a Decimal class that matches the semantics (but not the representation!) of IBM 360 mainframe data and SQL data. I'm using it for money, and decimal-exact rate values and so on, with various number of digits configured to the right of the decimal point. But I'm not using a semantic Units wrapper for the money. By being Decimal numbers only, it has no trouble implementing multiplication and it's up to the caller to feed it meaningful values; e.g. multiply a Price by a Tax_Rate. Putting strongly-typed wrappers so that Prices and Interest Rates are compile-time type checked would be another thing (that I'm not doing right now). Using a dimensional Units type wrapper based on that is a further enhancement of the same idea. Again, I'm not doing that in production code right now.

So, I suggest you forget "Pounds and Pence" e.g. 2 decimal places decimal, and make a general-purpose Decimal type. Make it a template or handle enough digits not just for Pence in consumer-facing prices, but have more digits for internal parts costs, special prices that use more digits normally (e.g. in the USA automobile gasoline prices end in 9/10ths of a cent), and auxiliary uses such as tax rates and other things you may need to multiply by, and the extra digits needed when you do such a multiplication (e.g. a price with 2 decimal places multiplied by a rate with 4 decimal places produces a product with 6 decimal places, to get an exact result).

If you make it a template, you have to worry about mixing usages of different types.

What I did was go through the design, earlier programs I was replacing, and concept code prototypes, and make a note of all the operations that were actually needed. I didn't have to design a Decimal template that did all things for all users; it only needed to do these specific operations. The program doesn't have ridiculous arbitrary different instantiations; only a few typed things like Prices, high-precision Totals, cents Totals, a Rate with a large number of decimal places, etc. And they are used together in certain ways befitting their roles, so full automatic mixed-type usage of every operation was not necessary.

I suggest you do the same.

---

design

Consider how values are imported and exported from this class. Parsing text input and producing text output is important I assume. Literals and importing from ordinary numbers will also be important building block even if you don't use that much when the full class gets used downstream. So make it constexpr and easy and efficient.

In my case, I'm representing values as integers with a power-of-ten scaling factor. Look at the code generated (via Compiler Explorer, perhaps) to make sure that the basic operations are efficient, and addition/subtraction of same type is basically free. Make sure that any division by a power of ten you need to do when scaling things is optimized with compile-time constants as division is a remarkably expensive operation on modern CPUs. Look at parameter passing and returning from functions, too: ideally you should be able to pass by value as efficiently as the underlying integer type.

Your implementation is multiplying and diving by 100 all over the place, even when just adding or subtracting values of the same type. This suggests that keeping the integer and fraction separate isn't a good idea; you are putting it all in one "as pennies" integer internally anyway! So just represent it that way.

Try to make all the basic operations constexpr.

Make things noexcept unless you are using exceptions to guard against overflow and whatnot.

Follow standards: Given some Money x; variable, you should be able to call to_string(x) just like with built-in types and types provided by most every other library. Insisting that yours uses x.toString() instead will annoy uses and best and break templates everywhere... like those Units and Dimensional wrappers that you specifically want to use with your type! You'll probably want to use your type with iostream and with fmt, so provide those as well.

If you have a single type with "enough" precision for all uses (normal consumer prices, internal high-precision prices, interest rates, etc.) you'll need a way to efficiently and easily truncate or round to a fewer number of decimals. If you have different types or a template, you'll need to worry about converting between them.

Work on the public API first. Without any implementation, you can still compile (but not link) your test cases. This will catch overloading ambiguities, conversions you wish were implicit, making sure things work together the way you want, etc. If your test cases embody the operations you need for your program, that will give you a class that is well fit for your purpose. Make sure you're not missing any operations you actually needed.

Review the class API and its documentation among your peers.
Only then, worry about implementing it.

misc

std::string Money::toString() const {
    std::string stringFormat = std::to_string(getPounds()) + "." + std::to_string(getPence());
    return stringFormat;
}

This is a particularly inefficient way to do it, on multiple fronts. You are not only creating two string objects for the components (though that's not so bad if your std library uses Short String Optimization), but you are using "." for a single character which loses the information on the length so it ends up calling the form of operator+ that takes a const char* and has to do strlen first thing. Then you allocate a string that's just long enough to add the dot, which might not be long enough to hold the decimal portion as well; so it has to re-allocate the storage again.

I implement the underlying text output routines in a function that is based on the newer std::to_chars which does not itself allocate memory. The various to_string, operator<< etc. can call that, with an internal fixed buffer or a string that's had enough space reserved, or whatnot.

So, even if you don't make a perfectly efficient implementation on the first go, design the API with that in mind. That is, having to_chars as the actual implementation and other things call that is more important than implementing to_chars optimally right out of the gate. You can use a quick&dirty output routine to get started and replace it later as a project in itself.

Answer 3

Just a few things:

Modern C++ has generally moved away from using types like long and int. Instead, use int64_t / int32_t. Advantage being that they are more transparent to what values they can actually store, and point to different native types, depending on the operating system. (A long on Windows does not guarantee to be the same size as a long on Linux, for example. An int64_t however does).

Initializing values like you do in your constructors initializer list is perfectly fine, and what I will say may just be preference, but if possible I like to default my values in the declaration directly, especially if with that I can get rid of the (default) constructor entirely.

Edit: @JDługosz informed be that the below part is absolutely not right. Sorry for that. Generally, I think storing money as whole numbers is a no go, outside of this just being a testing ground. Say you divide 1 pence by 3 and then multiply that by 1000. In real life, you'd end up with 333 pence, in your code with 0 (use floating points for storage, ints only for display)

There's no need for "this" pointers for accessing members of a class, unless you'd have some input- or local symbols with the same name. So best don't do that