C++ basic bank money class

Question

I am trying to create a basic money class that fits into a small console banking application I am making.

Money.h

#pragma once
#include <string>

class Money {
private:
    long pounds;
    int pence;

public:
    Money();
    // Overloaded constructors
    explicit Money(long pounds);
    Money(long pounds, int pence);

    /* Overload operators to allow easier arithmetic of money objects, we will
     not overload * or / as it does not make logical sense for money to be multiplied
     or divided.
    */
    Money operator+(const Money& moneyRhs) const;
    Money operator-(const Money& moneyRhs) const;

    friend std::ostream& operator<<(std::ostream& os, const Money& money);

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

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

Money.cpp

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

Money::Money(): pounds(0), pence(0) {}

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

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

Money Money::operator+(const Money& moneyRhs) const {
    // Convert all money to pence then do addition
    const long poundsInPence = (pounds + moneyRhs.pounds) * 100;
    const int totalPence = 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 {
    // Convert all money to pence then do subtraction
    const long poundsInPence = (pounds - moneyRhs.pounds) * 100;
    const int totalPence = 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 strMoneyFormat;

    // Check so see if the pence value is 1 digit, if so we need to add a trailing 0 for output
    // e.g £150.5 becomes £150.05
    if((getPence() > 0 ? static_cast<int>(log10(static_cast<double>(getPence()))) + 1 : 1) < 2) {
        strMoneyFormat = std::to_string(getPounds()) + "." + "0" + std::to_string(getPence());
    }
    else {
        strMoneyFormat = std::to_string(getPounds()) + "." + std::to_string(getPence());
    }

    return strMoneyFormat;
}

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

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

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

I'm relatively new to C++ still and want to know what I could improve in this code, especially what should I do about the + and / operator as I know when you overload arithmetic operators you should try to overload all, however you cant really dvided and multiply money. Any other tips would be appreciated.

Answer 1

Store Money as Fixed-Point

Monetary quantities are the classic example of something that might have fractional values, but you do not want to represent as a floating-point number, because you need exact arithmetic, not fast approximations.

At the same time, it’s a good idea to store at least one extra digit of precision, so that fractions of a penny will round and add up correctly.

You also definitely want to store this in a single integral value. Otherwise, you’ll constantly need to be checking whether your pence overflowed or underflowed. This integral type needs to be more than 32 bits long, so it can hold numbers in the billions and trillions, and to be signed. So, use long long int to hold the quantity

It’s a good exercise in how to write a class that encapsulates an opaque internal representation. In my tests, I used mills.

Mind Your Signs

Does Money( -12, 50 ) represent £-12.50 or £-11.50? Are your member functions consistent about this?

This is another problem that a fixed-point representation will solve.

Declare Default Copy Constructor and Assignment

You should ignore what I said here before; it was a bad explanation. (I’m grateful to Sebastian Redl for pointing out my error.) Here’s what’s actually going on. The compiler will create a default copy constructor and = operator for each class. In many cases, you will want to write one of your own, instead of using the defaults. For example, a move might be more efficient than a copy, and declaring a move constructor would replace the default copy constructor from being created. You might want copying and assignment to be private. You might have some non-trivial initialization to do.

None of these apply here; what you wrote will work. I think it’s good practice to declare these implicit functions as part of the interface and remove all ambiguity about them. Otherwise, some other code could make the compiler delete one of them.

The Rule of Three holds that, if your class is complicated that it needs an explicit copy constructor, assignment or destructor, it should have all three. If it’s managing any resources that can’t be trivially copied, the assignment operator also needs to copy them, and usually they can’t be trivially deleted either. This doesn’t really apply to the default constructors, but, because the rule is so widely recommended, I normally declare all three, even if it’s as default or delete. (There is also a Rule of Five, which says that, if you need either a move constructor or move assignment, you should declare both of those two, plus the other three, which they would otherwise replace. Here, we don’t need them.)

Use the Standard Library’s Formatting

With <iomanip> you can set the fixed and precision specifiers to make all quantities display exactly two digits after the decimal point.

Edit: Since long double conversion on some platforms (including MSVC) could improperly round off the lower digits. I wrote a new version that uses the <iomanip> manipulators

Check for Errors

If your library is ever managing trillions and trillions of dollars, it should definitely be checking for out-of-range errors!

A good way to do this, especially in constructors, is to throw exceptions from <stdexcept>, such as invalid_argument, overflow_error and underflow_error.

This is trickier than it sounds, because signed overflow or underflow is undefined behavior. The instructions on modern CPUs will wrap around, but compiler writers feel this gives them permission to break your error-checking code. What you actually need to do is find the difference between the maximum/minimum representable value, and one operand, then compare that to the other operand to see if you have room to add or subtract it.

Name Your Constants When Possible

You should prefer defining constants such as Money::pence_per_GBP to constants such as 100. This makes it much easier to change the type, easier to understand why you are using the constants you do and whether they are correct, and harder to make typos.

And besides, the number of pence in a pound has changed before.

These should be either static constexpr class members, or declared in the module that uses them.

Edit: Contrary to what I said before, static constexpr data members do not need to be defined outside the class declaration any longer, and doing so was deprecated in C++17. I’ve deleted that part of the code.

Declare Your Functions constexpr and noexcept where Appropriate

This helps optimize, and also allows them to be used to initialize compile-time constant expressions.

Don’t Have More Friends than You Need

If the stream output operator calls the public string conversion function, it doesn’t need to be a friend of the class. That allows it to breach encapsulation and access non-public members.

Speaking of which:

Follow Standard Naming Conventions

I like camelCase, but the STL uses snake_case consistently. In particular, the STL classes that convert to string call that function .to_string(), not .toString(), and several templates duck-type to the former.

CapitalizedCamelCase class names (also called PascalCase) are widely-used, though, so that’s fine.

Optionally: What Other Functions Make Sense

You have a stream output operator << but no stream input operator >>. There’s no += or -=. You have a comment that multiplication and division wouldn’t make sense, but what about scalar multiplication and division, such as full_price * discounted_rate? What about a negation operator, such as borrower_liability = -loan_amount;? What about comparisons, like income >= expenses?

Consider User-Defined Suffixes

I went ahead and put two of these in for fun, _L for monetary values in pounds and _p for monetary values in pence.

If these might clash with someone else’s _L or _p, a good solution is to put their declarations in a namespace, like the STL does for string literals. This way, it doesn’t overload _s for either strings or seconds unless I add the line:

using namespace std::literals::string_literals;

Putting it All Together

#include <iostream>
#include <limits>
#include <string>

class Money {
private:
/* “The mill is a unit of currency, used in several countries as one-
 * thousandth of the main unit.” (In this case, the GBP.)  This is
 * at least 64 bits wide, to be able to represent amounts larger than
 * £2,147,483.64.  It has one extra digit of precision, to ensure
 * proper rounding.
 *
 * If a long double has fewer than 63 mantissa bits, this implementation
 * might incorrectly round off extremely large credits or debits.  You
 * might want to flag this and throw a std::range_error exception.
 */
    long long mills;
 
/* Compile-time constants.  I originally had definitions of these in
 * namespace scope as well, but that is no longer necessary and is now
 * deprecated.
 */
    static constexpr auto money_max =
      std::numeric_limits<decltype(mills)>::max();
    static constexpr auto money_min =
      std::numeric_limits<decltype(mills)>::min();
    static constexpr int mills_per_GBP = 1000;
    static constexpr int mills_per_penny = 10;

 /* Used internally to bypass round-trip conversion.  The dummy parameter is
  * there only to distinguish this constructor from the one that takes a
  * value in pounds.
  */
    struct dummy_t {};
  
    explicit constexpr Money (long long source, dummy_t) noexcept;

public:
    // Use the trivial default constructor instead of a custom one.
    constexpr Money() noexcept = default;
    /* Allow either Money(12.34) or Money(12,34).  Note that Money(12.345)
     * is legal, but Money(12,345) should throw a std::invalid_argument
     * exception.
     */
    constexpr Money(long double pounds);
    constexpr Money( long long int pounds, unsigned pence );
    // The implicit members would have sufficed.
    constexpr Money(const Money&) noexcept  = default;
    Money& operator= (const Money&) noexcept = default;
    ~Money() = default;
    /* If this class can be a base class, it would need a virtual destructor.
     * Otherwise, trivial destruction suffices.
     */

    /* These are constexpr, but not noexcept, because they could throw a
     * std::overflow_error or std::underflow_error exception.
     */
    constexpr Money operator+(const Money&) const;
    constexpr Money operator-(const Money&) const;

    // This should be named in snake_case, not camelCase:
    std::string to_string() const;
    
    /* Returns the quantity denominated in pounds, rounded to the nearest
     * penny.  You might throw an exception rather than return an incorrectly-
     * rounded result.
     */
    constexpr long double to_pounds() const noexcept;
    /* Returns only the part of the currency string beginning with the
     * point.  E.g., for £12.34, returns 0.34, and for £-56.78, returns 0.78.
     */
    constexpr double pence() const noexcept;
    
    static constexpr int pence_per_GBP = 100;
};

// This only needs to be a friend if it uses an interface that isn’t public:
std::ostream& operator<< ( std::ostream&, Money );
// Unimplemented:
std::istream& operator>> ( std::istream&, Money& );

// User-defined literal for money in pounds, e.g. 12.34_L.
constexpr Money operator""_L (long double);
// User-defined literal for money in pence, e.g. 56_p.
constexpr Money operator""_p (long double);

#include <cmath>
#include <iomanip>
#include <sstream>
#include <stdexcept>
#include <string>

using namespace std::literals::string_literals;

static const std::string invalid_arg_msg = "Monetary quantity out of range.",
                         overflow_msg = "Monetary overflow.",
                         underflow_msg = "Monetary underflow.";

constexpr Money::Money(const long double pounds)
/* Converts the quantity in GBP to the internal representation, or throws a
 * std::invalid_argument exception.  Rounds to the nearest mill.
 */
{
  /* Refactored so that a NaN value will fall through this test and correctly
   * raise an exception, rather than, as before, be spuriously converted.
   * On an implementation where the precision of long double is less than that
   * of long long int, such as MSVC, the bounds tests below could spuriously
   * reject some values between the bounds and the next representable value
   * closer to zero, but it will only convert values that are in range.
   */
  if ( mills_per_GBP * pounds < static_cast<long double>(money_max) &&
       mills_per_GBP * pounds > static_cast<long double>(money_min) ) {
    // We unfortunately cannot use llroundl in a constexpr function.
    mills = static_cast<decltype(mills)>( mills_per_GBP * pounds );
  } else {
    throw std::invalid_argument(invalid_arg_msg);
  }
}

constexpr Money::Money( const long long pounds, const unsigned pence )
/* Converts the quantity in pounds and pence to the internal representation,
 * or throws a std::invalid_argument exception.
 *
 * For example, Money(-1234,56) represents £-1234.56.
 */
{   
  if ( pounds > money_max / mills_per_GBP ) {
    throw std::invalid_argument(invalid_arg_msg);
  }

  if ( pence >= pence_per_GBP ) {
    throw std::invalid_argument(invalid_arg_msg);
  }

  const long long base = mills_per_GBP * pounds;
  const long long change = mills_per_penny *
                           ( (pounds >= 0) ? pence : -(long long)pence );                
  
  if ( base > 0 && money_max - base < change ) {
    throw std::invalid_argument(invalid_arg_msg);
  }
  
  if ( base < 0 && money_min - base > change ) {
    throw std::invalid_argument(invalid_arg_msg);
  }
  
  mills = base + change;
}

constexpr Money::Money ( const long long source,
               [[maybe_unused]] Money::dummy_t dummy // Only to disambiguate.
             ) noexcept
 : mills(source)
/* Used internally to bypass  unnecessary conversions and range-checking.
 */
{}

constexpr Money Money::operator+(const Money& other) const
/* Adds this and other, checking for overflow and underflow.  We cannot
 * portably rely on signed integer addition to wrap around, as signed
 * overflow and underflow are undefined behavior.
 */
{
  if ( mills > 0 && money_max - mills < other.mills ) {
    throw std::overflow_error(overflow_msg);
  }

  if ( mills < 0 && money_min - mills > other.mills ) {
    throw std::underflow_error(underflow_msg);
  }
    
  return Money( mills+other.mills, dummy_t() );
}

constexpr Money Money::operator-(const Money& other) const
/* Subtracts other from this, checking for overflow and underflow.
 */
{
  if ( mills > 0 && money_max - mills < -other.mills ) {
    throw std::overflow_error(overflow_msg);
  }

  if ( mills < 0 && money_min - mills > -other.mills ) {
    throw std::underflow_error(underflow_msg);
  }
    
  return Money( mills-other.mills, dummy_t() );
}

std::string Money::to_string() const
/* In the future, you may be able to use std::format instead.  You might also
 * use snprintf.
 *
 * Changed to do integer math rather than a FP conversion that might display a
 * spuriously-rounded value.
 */
{
  std::stringstream to_return;
  const auto pounds = mills/mills_per_GBP;
  const auto pence = (mills >= 0) ? ( mills % mills_per_GBP )/mills_per_penny
                                  : -( mills % mills_per_GBP )/mills_per_penny;

  to_return << "£" << pounds << '.'
            << std::setw(2) << std::setfill('0') << std::right
            << pence;
  return to_return.str();
}

constexpr long double Money::to_pounds() const noexcept
{
  return static_cast<long double>(mills) / mills_per_GBP;
}

constexpr double Money::pence() const noexcept
{
  const double remainder = (double)(mills % mills_per_GBP);
  return (remainder >= 0) ? remainder/mills_per_GBP
                          : -remainder/mills_per_GBP;
}

std::ostream& operator<< ( std::ostream& os, const Money x )
{
  return os << x.to_string();
}

// User-defined literal for money in pounds, e.g. 12.34_L.
constexpr Money operator""_L (const long double pounds)
{
  return Money(pounds);
}

// User-defined literal for money in pence, e.g. 56_p.
constexpr Money operator""_p (const long double pence)
{
  return Money(pence/Money::pence_per_GBP);
}


#include <cstdlib> // for EXIT_SUCCESS
#include <iostream>
#include <limits>
#include <stdexcept>

using std::cout;

int main()
{
  constexpr Money one_million_GBP(1e6),
                  minus_twelve_forty_GBP( -12, 40 ),
                  lotta_money = 4.62e15_L;
  
  try {
    const Money invalid_value = Money(1.0e16L);
    cout << "Stored unexpectedly large value " << invalid_value << ".\n";
  } catch (const std::invalid_argument&) {
    cout << "Properly caught a value too high.\n";
  }
  
  try {
    const Money invalid_value = Money(-1.0e16L);
    cout << "Stored unexpectedly small value " << invalid_value << ".\n";
  } catch (const std::invalid_argument&) {
    cout << "Properly caught a value too low.\n";
  }
  
  try {
    const Money invalid_value = lotta_money + lotta_money;
    cout << "Added unexpectedly large quantity " << invalid_value << ".\n";
  } catch (const std::overflow_error&) {
    cout << "Properly caught arithmetic overflow.\n";
  }
  
  try {
    const Money invalid_value = Money() - lotta_money - lotta_money;
    cout << "Added unexpectedly large liabilty " << invalid_value << ".\n";
  } catch (const std::underflow_error&) {
    cout << "Properly caught arithmetic underflow.\n";
  }
  
  try {
    const Money invalid_value =
      Money(std::numeric_limits<long double>::quiet_NaN());
    cout << "Improperly interpreted a NaN value as " << invalid_value << ".\n";
  } catch (const std::invalid_argument&) {
    cout << "Properly caught initialization of money from NaN.\n";
  }
  
  // Expected output: £0.10, £1000000.00, £-12.40, £999987.60 and £1000012.40.
  cout << 10.1_p << ", "
       << one_million_GBP << ", "
       << minus_twelve_forty_GBP << ", "
       << (one_million_GBP + minus_twelve_forty_GBP) << " and "
       << (one_million_GBP - minus_twelve_forty_GBP) << ".\n";
       
  cout << one_million_GBP.to_pounds() << " pounds and "
       << Money::pence_per_GBP*minus_twelve_forty_GBP.pence() << " pence.\n";
  
  return EXIT_SUCCESS;
}

Update: I decided to take my own advice about declaring functions constexpr where possible, and made all the constructors constexpr. Now the declaration,

  constexpr Money one_million_GBP(1e6),
                  minus_twelve_forty_GBP( -12, 40 ),
                  lotta_money = 4.62e15_L;

compiles (on clang++13 with std=c++20 -O3 -march=x86-64-v4) to:

mov     qword ptr [rsp + 72], 1000000000
mov     qword ptr [rsp + 64], -12400
movabs  rax, 4620000000000000000

It could be a huge boost in performance if your Money objects can be optimized into compile-time integer constants.

There’s another way to optimize this class, and other tiny classes, that I did not do. Since it’s trivially-copyable and so small that an object can fit in a register, there is no reason to pass it by const reference. If you pass it by value instead, some variables that might otherwise need to be spilled onto the stack to take their address can instead stay in registers.

This code outputs the non-ASCII pound sign. This should work on modern OSes, but if you’re having some difficulty on an older version of Windows, you might want to give CL.EXE the flag /utf-8 and set chcp 65001 in your console window.

One word of explanation about something that might not be obvious: because I made the public constructors more complicated, I wanted to add a more lightweight private constructor that set the internal field directly. Since the compiler would otherwise be unable to tell when I was calling the public or private constructor, I created a nullary Money::dummy_t type solely to make sure the type signature of the private constructor was unique. This solution was a bit ungainly, but it’s not part of the public interface anyway.

Answer 2

Store money as a single integer

Using integers instead of floating point values to store an amount of money is a very good idea. However, you don't need two integers, you can just use one, that stores the value in pence. This makes a lot of things easier.

Also, by having two signed integers, you have to worry about all four possible combinations of signs of pounds and pence, with one integer you don't have that problem.

Multiplying and dividing money

[...] as it does not make logical sense for money to be multiplied or divided.

Yes and no. It does not make sense to multiply an amount of money by another amount of money, but it does make sense to multiple an amount of money by, say, an integer, or even a floating point number. For example, say one bread costs 3 pounds, and I want to buy 5 breads. How much will that cost me in total?

Dividing is a bit different. It actually makes sense to divide money by money. Going by the same example: say I have 15 pounds, and a bread costs 3 pounds. How many breads can I buy?

You can implement operator overloads for multiplying Money by an int for example, which returns Money. Or one that divides Money by another amount of Money and returns a double.

Converting to string

Checking if getPence() returns a value between 0 and 9 can be done much simpler than taking the base-10 logarithm of a value. Especially if you could ensure that pence is always between 0 and 99, it is just:

if (pence < 10)
    strMoneyFormat = std::to_string(pounds) + ".0" + std::to_string(pence);
} else {
    strMoneyFormat = std::to_string(pounds) + "." + std::to_string(pence);
}

Although that is still repeating things twice, it can be rewritten to:

std::string Money::toString() const {
    return std::to_string(pounds)
           + (pence < 10 ? ".0" : ".")
           + std::to_string(pence);
}

But since you tagged the post C++20, consider using std::format():

std::string Money::toString() const {
    return std::format("{}.{:02}", pounds, pence);
}

If your compiler's standard library doesn't support std::format() yet, you can use fmt::format() instead.

Answer 3

Use default values in your parameters, instead of creating overlapping function definitions

Instead of:

Money(); // Overloaded constructors 
explicit Money(long pounds);
Money(long pounds, int pence);

Do:

explicit Money(long pounds = 0, int pence = 0);

Then you just need :

// Money::Money(): pounds(0), pence(0) {} 
// Money::Money(const long pounds): pounds(pounds), pence(0) {}

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

---

As pointed out in the comments, not every overloaded function is suitable for parameter defaults.

The constructors can (for C++11 and above) be written using delegating constructors:

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

Money::Money()
      : Money(0, 0) 
{} 

Money::Money(const long pounds)
      : Money(pounds, 0) 
{}

Answer 4

See C++ money class for basic banking application for a similar class. See my answer and others that address the identical issues in your code.

In particular, you are multiplying out the pounds and pence into a single number of pence whenever you do work on it, so why not just store it that way? Only factor it if someone asks for the pounds and pence values.

You have a + and a - but not += and -=. The normal thing is to define += as a member and then + just calls +=.

How about a unary - (negation)? That would make it trivial to write subtraction by calling addition. I think someone might want to write -m at some point and it would be annoying to write Money{}-m instead.

You have a toString member but everyone (including template code) expects a to_string non-member function to exist.

Your constructors should be inline in the class definition, because they are very trivial and should be inlined when used. You prevent that and require a function call. (That's the case for most compiler environments today; might not be an issue if you have link-time code generation)

I see you have no implicit conversions to your class. That makes the asymmetric operator+ OK I guess, but it's still unusual to write it as a member function that treats the left and right arguments differently.

Did you know that % is very slow?

I think your toString formatting is awkward. You have some crazy formula to predict the number of digits and then call two different formatting passages that differ only in one place. Appending C strings of one character is inefficient compared to using a char constant instead. Adding strings like this causes multiple re-allocations and copies to be made.