6
\$\begingroup\$

I have program that uses a Money class which has several constructors for different methods of input and overloaded operators to add, subtract, and compare values.

I am having difficulty defining my int asCents(const Money& amount) const; method so that it can be used in other functions. Used correctly, is it best practice to return the integer it produces as opposed to creating two new ones equivalent to it (amount.getCents() + amount.getDollars() * 100;)? How else can this program be improved?

#include <iostream>
#include <cstdlib>
#include <cmath>
#include <string>
using std::cerr;
using std::cin;
using std::cout;
using std::endl;
using std::string;
using std::ostream;
using std::istream;

class Money {

public:

    Money();
    Money(double asMoney);
    Money(int justDollars);
    Money(int withDollars, int withCents);

    friend const Money operator +(const Money& amount1, const Money& amount2);
    friend const Money operator -(const Money& amount1, const Money& amount2);

    friend const Money operator %(const Money& amount, double percentage);

    friend bool operator ==(const Money& amount1, const Money& amount2);
    friend bool operator <(const Money& amount1, const Money& amount2);
    friend bool operator >(const Money& amount1, const Money& amount2);
    friend const Money operator -(const Money& amount1);
    friend ostream& operator <<(ostream& outputStream, const Money& amount);
    friend istream& operator >>(istream& inputStream, Money& amount);

    int getDollars() const { return dollars; }
    int getCents() const { return cents; }

    //int asCents(const Money& amount) const; // unused
    Money percent(const Money& amount, double percentage) const;

private:

    int dollars;
    int cents;
    int dollarsPart(double amount) const;
    int centsPart(double amount) const;
    int round(double number) const;
};

    int Money::centsPart(double amount) const {
    double doubleCents = amount * 100;
    int intCents = (round(fabs(doubleCents))) % 100;
    if (amount < 0)
        intCents = -intCents;
    return intCents;
}

int Money::dollarsPart(double amount) const { return static_cast<int>(amount); }
int Money::round(double amount) const { return static_cast<int>(floor(amount + 0.5)); }

const int asCents(const Money& amount) {
    int amountAsCents = amount.getCents() + amount.getDollars() * 100;
    return amountAsCents;
}

ostream& operator <<(ostream& outputStream, const Money& amount) {
    int absDollars = abs(amount.dollars);
    int absCents = abs(amount.cents);
    if (amount.dollars < 0 || amount.cents < 0)
        outputStream << "$-";
    else
        outputStream << '$';
        outputStream << absDollars;
    if (absCents >= 10)
        outputStream << '.' << absCents;
    else
        outputStream << '.' << '0' << absCents;
    return outputStream;
}

istream& operator >>(istream& inputStream, Money& amount) {
    char dollarSign;
    while (inputStream >> dollarSign && dollarSign != '$') {
        cout << "\nPlease enter a dollar sign ($) for money input.\n" << endl;
        cin.ignore();
        cout << "Enter an amount of money: ";
    }
    double amountAsDouble;
    inputStream >> amountAsDouble;
    amount.dollars = amount.dollarsPart(amountAsDouble);
    amount.cents = amount.centsPart(amountAsDouble);
    return inputStream;
}   

const Money operator +(const Money& amount1, const Money& amount2) {
    int centsSum = asCents(amount1) + asCents(amount2);
    int absAllCents = abs(centsSum);
    int finalDollars = absAllCents / 100;
    int finalCents = absAllCents % 100;
    if (centsSum < 0) {
        finalDollars = -finalDollars;
        finalCents = -finalCents;
    }
    return Money(finalDollars, finalCents);
}

const Money operator -(const Money& amount1, const Money& amount2) {
    int centsDifference = asCents(amount1) - asCents(amount2);
    int absAllCents = abs(centsDifference);
    int finalDollars = absAllCents / 100;
    int finalCents = absAllCents % 100;
    if (centsDifference < 0) {
        finalDollars = -finalDollars;
        finalCents = -finalCents;
    }
    return Money(finalDollars, finalCents);
}

bool operator ==(const Money& amount1, const Money& amount2) {
    return ((amount1.getDollars() == amount2.getDollars())
        && (amount1.getCents() == amount2.getCents()));
}

bool operator <(const Money& amount1, const Money& amount2) {
    return ((amount1.getDollars() < amount2.getDollars())
        && (amount1.getCents() < amount2.getCents()));
}

bool operator >(const Money& amount1, const Money& amount2) {
    return ((amount1.getDollars() > amount2.getDollars())
        && (amount1.getCents() > amount2.getCents()));
}

const Money operator -(const Money& amount) {
    return Money(-amount.getDollars(), -amount.getCents());
}

Money Money::percent(const Money& amount, double percentage) const {
    int amountToCents = amount.getCents() + amount.getDollars() * 100;
    double pScaledMoney = amountToCents * percentage;
    int scaledDollars = abs((round(fabs(pScaledMoney / 100))) / 100);
    int scaledCents = abs((round(fabs(pScaledMoney / 100))) % 100);
    if (pScaledMoney < 0) {
        scaledDollars = -scaledDollars;
        scaledCents = -scaledCents;
    }
    return Money(scaledDollars, scaledCents);
}   

// Varying constructors
Money::Money() : dollars(0), cents(0) {}
Money::Money(int justDollars) : dollars(justDollars), cents(0) {}
Money::Money(double asMoney) : dollars(dollarsPart(asMoney)), cents(centsPart(asMoney)) {}
Money::Money(int withDollars, int withCents) {
    if ((withDollars < 0 && withCents > 0) || (withDollars > 0 && withCents < 0)) {
        cout << "You're fired";
        exit(1);
    }
    dollars = withDollars;
    cents = withCents;
}

void compareMoney(const Money& amount1, const Money& amount2) {

    cout << "\nYour amount is " << amount1 << endl;
    cout << "My amount is " << amount2 << endl;

    if (amount1 == amount2)
        cout << "\nWe have the same amount of money\n" << endl;
    else
        cout << "\nSomeone has more money..\n" << endl;

    Money ourAmount = amount1 + amount2;
    cout << amount1 << " + " << amount2 <<
    " equals " << ourAmount << endl;
        Money amountDifference = amount1 - amount2;
    cout << amount1 << " - " << amount2 <<
        " equals " << amountDifference << endl;

    Money halfYourMoney = amount1.percent(amount1, 50);
    Money halfMyMoney = amount2.percent(amount2, 50);

    cout << "%50 of your money is " << halfYourMoney << endl;
    cout << "%50 of my money is " << halfMyMoney << endl;

    if (amount1 < amount2)
        cout << "\nI have more money!" << endl;
    if (amount1 > amount2)
        cout << "\nYou have more money!" << endl;
}   

int main() {

    cout << "Use a dollar sign ($) to enter a money value." << endl;
    cout << "-------------------------------------------------------------------------" << endl;

    bool isRunning(true);
    while (isRunning) {

        try {

            Money yourAmount, myAmount(10, 9);
            cout << "Enter an amount of money: ";
            cin >> yourAmount;
            compareMoney(yourAmount, myAmount);

        } // Close try
        // Generalized Catch
        catch (std::exception & e) {
            cerr << "\nERROR : " << e.what() << '\n';
        }

        char choice('c');
        bool invalidChoice(true);
        // Option to run again
        while (cout << "\nRun Again? (y/n) \n\n" && (!(cin >> choice)) || (!(choice == 'n')) && (!(choice == 'y'))) {
            cin.clear();
            cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
            cerr << "\nERROR: The only valid answers are y/n. \n" << endl;
        }
        if (choice == 'y') {
            invalidChoice = false;
            cout << "\nSelected: \"" << choice << "\" ---> *** isRunning AGAIN. ***\n" << endl;
        }
        else if (choice == 'n') {
            invalidChoice = false;
            isRunning = false;
            cout << "\nSelected: \"" << choice << "\" ---> *** EXITING. ***\n" << endl;
        }

        } // Close running
        do {
    cin.ignore();
        cout << '\n' << "Press enter to continue...";
    } while (cin.get() != '\n');

    return 0;

}
\$\endgroup\$
11
\$\begingroup\$

This is way too complicated. You could have just the cents as a member and have some helper functions that converts it to the respective parts. This will simplify most of your functions a lot.

const Money operator +(const Money& amount1, const Money& amount2) 
{
    return Money(amount1.cents + amount2.cents);
}

const Money operator -(const Money& amount1, const Money& amount2)
{  
    return Money(amount1.cents - amount2.cents)
}

int dolarsFloor() const
{
    return cents / 100;
}

int centsReminder() const
{

    return cents % 100 ;
}

There are other things that can be improved but I just wanted to leave my 2 cents ;).

\$\endgroup\$
  • 1
    \$\begingroup\$ That's basically the most important point that needs to be made. \$\endgroup\$ – Martin York Sep 27 '15 at 3:37
5
\$\begingroup\$

None of your overloaded operators (except for the stream ones) need to be friends. operator+, etc. can be defined inline and operator==, etc. can be defined non-member. The fact that you use the public interface in operator== is a strong hint that it doesn't need to be a friend.

There's a LOT of code duplication going on. The operators can be overloaded in terms of each other. (You should also make the relational operators const.)

bool operator >(const Money& amount1, const Money& amount2) const {
    return !(amount1 < amount2);
}

You forgot to implement operator!=:

bool operator !=(const Money& amount1, const Money& amount2) const {
    return !(amount1 == amount2);
}

If you have C++11, the std::put_money and std::get_money functions from <iomanip> are available. First, choose your locale:

std::cout.imbue(std::locale("en_US.utf8"));

This will simplify your code:

ostream& operator <<(ostream& outputStream, const Money& amount) {
    outputStream.imbue(std::locale("en_US.utf8"));
    outputSteam << std::put_money(dollars + cents);
    return outputStream;
}

dollars + cents isn't real code. However, I leave that as an exercise to you. Of course, it would potentially simplify things if you kept track of the entire amount as a double.

\$\endgroup\$
  • \$\begingroup\$ You should also make the relational operators const” – that is solid advice for most member functions, but then you show a bool operator>(const Money&, const Money&) const that looks suspiciously like a free function rather than a member. Does the const qualifier even work there? What would be made const, considering that both arguments already are const Money&? There is no this pointer. But if it were a member function, how coukd the operator> take two arguments? Sorry, I'm still new to operator overloading in C++. \$\endgroup\$ – amon Sep 27 '15 at 11:25
  • \$\begingroup\$ I diasagree with your friend comment. This is the one classic case where they are better as friends as this is one of those times were auto conversion is useful and only happens properly for friends. i want Money + integer to behave in the same way as Integer + Money this is what friend functions will achieve that members will not. \$\endgroup\$ – Martin York Sep 27 '15 at 15:39
  • \$\begingroup\$ Also you can define friends inline. I do that all the time for simple functions. \$\endgroup\$ – Martin York Sep 27 '15 at 15:40
  • \$\begingroup\$ You should never track money as a double. Floating point values can not store all values exactly and thus when you do other operations you get rounding errors. \$\endgroup\$ – Martin York Sep 27 '15 at 15:42
  • 2
    \$\begingroup\$ !(a<b) is a>=b, not a>b. \$\endgroup\$ – Kroltan Sep 27 '15 at 16:56
1
\$\begingroup\$
  • Although it's mostly personal preference, you could still add std:: wherever needed and leave out all the usings. This could reduce a bit of clutter towards the top. Plus, you never included a using for abs, though the compiler may sometimes fail to catch that.

  • Since you're using just dollars and cents, perhaps this class could be renamed to something more specific like USD. Regardless, your print statements can still include the $ character so that the user will know it's still a monetary value and not just a regular numerical value.

  • There is a slight indentation inconsistency in the centsPart() statement:

        int Money::centsPart(double amount) const {
        double doubleCents = amount * 100;
        int intCents = (round(fabs(doubleCents))) % 100;
        if (amount < 0)
            intCents = -intCents;
        return intCents;
    }
    
  • This may not be a good way of handling error in a constructor:

    Money::Money(int withDollars, int withCents) {
        if ((withDollars < 0 && withCents > 0) || (withDollars > 0 && withCents < 0)) {
            cout << "You're fired";
            exit(1);
        }
        dollars = withDollars;
        cents = withCents;
    }
    

    The print statement, especially such an informal one, isn't necessary. Just leave it for client code if desired. The user may also not be expecting the program to exit after a failed construction, and I don't think it's too common. It may be better to instead throw an exception on failure while making the user aware of that.

    You already have a try block in main() for two constructors and a member function, but it won't be of any use unless they can throw something. If you do make this change, you can also catch that specific exception instead of just a generic std::exception and its associated error. That could also make it easier to determine which entity has thrown an exception, if made possible for more than one of them (but not likely the default constructor).

  • There's no need to mix up both "\n" and endl:

    if (amount1 == amount2)
        cout << "\nWe have the same amount of money\n" << endl;
    else
        cout << "\nSomeone has more money..\n" << endl;
    

    There's no need to also flush the buffer with endl, so just use any additional "\n":

    if (amount1 == amount2)
        cout << "\nWe have the same amount of money\n\n";
    else
        cout << "\nSomeone has more money..\n\n";
    
  • There's no need to have your own return 0 at the end of main() in C++. The compiler will automatically add this same return.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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