4
\$\begingroup\$

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;
}
\$\endgroup\$
1

3 Answers 3

5
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ It looks like moneycpp that you cited is centered around using different currencies in the manner of units. It relies on boost::multiprecision::cpp_dec_float or other numeric library of your choice for the actual arithmetic. \$\endgroup\$
    – JDługosz
    Dec 28, 2021 at 16:10
  • \$\begingroup\$ This is a nice answer, say you were to use pounds and pence, how would the unary operator work? would you just set pounds = -pounds? \$\endgroup\$ Jan 18, 2022 at 22:26
  • \$\begingroup\$ @mathewsjoyy It depends on how you convert from pounds and pence to allPence. If you do allPence = pounds * 100 + pence, then you need to flip the sign of both pounds and pence, or do something like pounds = -pounds - 1; pence = 100 - pence if you want to keep pence non-negative. If you just flip the sign of pounds, then you need to do something like allPence = pounds * 100 + (pounds < 0 ? -pence : pence). \$\endgroup\$
    – G. Sliepen
    Jan 18, 2022 at 22:50
3
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ There is no commonly used class for money? How about just using boost::units? \$\endgroup\$ Dec 24, 2021 at 13:06
  • \$\begingroup\$ Could you suggest a book/source expounding in detail the C++ memory model? So that one can fully understand the under-the-hood advantages of std::to_chars, the expensiveness of divisions, etc. Is this info available in the big Stroustrup book? \$\endgroup\$
    – Giogre
    Dec 25, 2021 at 22:54
  • 1
    \$\begingroup\$ @Joger start with boost.org/doc/libs/1_77_0/doc/html/container/… and the link in the first paragraph; There's a new book, The Art of Writing Efficient Programs published by Packt that I've looked through the memory chapter, but not read the whole thing. There are videos on performance on YouTube from CppCon and other major conferences. I don't know anything that discusses division specifically, ... \$\endgroup\$
    – JDługosz
    Dec 28, 2021 at 15:31
  • 1
    \$\begingroup\$ ... but the other performance killer that is discussed at length in conferences would be Branch Prediction. I'd suggest going through the CppCon videos over the past few years and find interesting things to you, and learn which speakers you like and search for them for more. \$\endgroup\$
    – JDługosz
    Dec 28, 2021 at 15:33
  • \$\begingroup\$ @Deduplicator hmm, I think your point is that you could define different units like Dollars and Cents as scaling factors in the same dimension, even though it's not a "decimal" library. Interesting idea. My remarks are from my search for a decimal library specifically. I don't know how popular Boost::units is now, or if it plays well with new code. I know that a compile-time error will generate "a small novel by Oscar Wilde" as feedback. \$\endgroup\$
    – JDługosz
    Dec 28, 2021 at 15:52
-2
\$\begingroup\$

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

\$\endgroup\$
5
  • 3
    \$\begingroup\$ Storing money as "whole numbers" (e.g. in the smallest unit), not floating point, is de rigueur. DO NOT use floating point! \$\endgroup\$
    – JDługosz
    Dec 24, 2021 at 7:19
  • 1
    \$\begingroup\$ @JDługosz Interesting. Will edit my posting. Admittedly, I did not think about it this way and myself have never spend more time than 15 seconds thinking about the matter. I just presumed the choosing of integers was done as a rookie-mistake. Will do some research on why that is and how the obstacles of integer divisions are then overcome. Guess fractions are then just stored as a divident and divisor? \$\endgroup\$ Dec 24, 2021 at 11:41
  • \$\begingroup\$ The problem with floating point numbers for money is that it introduces floating point errors. This can cause problems maintaining bank accounts. \$\endgroup\$
    – pacmaninbw
    Dec 24, 2021 at 13:44
  • \$\begingroup\$ Modern C++ doesn't use int64_t in this case. Modern C++ uses a template defaulted to e.g. int: template<typename T=int> class Money { ... }. In that case, the user of the library can adjust the underlying type to whatever is needed. \$\endgroup\$
    – Sjoerd
    Dec 24, 2021 at 17:30
  • 3
    \$\begingroup\$ The claim about modern C++ “moving away” from int and long complete nonsense. In fact, it’s the fixed-width types std::int64_t/std::int32_t that are not portable. \$\endgroup\$
    – indi
    Dec 24, 2021 at 21:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.