patterncppMinor
Simple C++ class for storing and manipulations with money
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;
}
}
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
If you insist, you should just
Swap doesn't
Your signature for swap is
Although normal
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:
And then implement
Just one check everywhere is sufficient.
op=
You are implementing
Use a mem-initializer list
When you construct, construct the members with the target values directly:
This is more meaningful with more complicated types, but might as well get in the habit now.
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;
// etcSwap 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;
// etcoperator==
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.