HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Simple C++ class for storing and manipulations with money

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
moneysimplestoringmanipulationswithforandclass

Problem

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
#include
#include

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

Solution

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 = 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.

Code Snippets

Money(const Money&) = default;
Money(Money&&) = default;
// etc
operator==
operator<
a != b   ===   !(a==b)
a > b    ===   b < a
a >= b   ===   !(a < b)
a <= b   ===   !(b < a)
class Money {
    friend Money operator+(Money lhs, const Money& rhs) {
        return lhs += rhs;
    }
};
Money::Money(int32_t _whole, int8_t _fraction)
: whole(_whole)
, fraction(_fraction)
{ }

Context

StackExchange Code Review Q#109821, answer score: 9

Revisions (0)

No revisions yet.