Clean code attempt at ATM problem on codechef.com

The problem asks you to take an integer (debit amount) and a double (credit or initial balance amount) and process the requested debit verifying that 1 it's a multiple of the minimum denomimation amount of $5 and that it's also smaller than the credit/balance. If either is untrue, it is supposed to return the initial deposit amount otherwise it will return the new balance. Full problem description I have created 3 objects for this problem: 1. Transaction - This object reads in the two initial values given and then is used in ATM 2. ATM - Takes the transaction and applies them to the account and then displays the new balance. 3. Account - This object keeps track of the current account balance and updates the balance if the ATM passes it a value. Limitations: I understand that it can only process a single account, but that is more a limitation set by the problem description than it is me not accounting for multiple accounts. Also no error is returned if the balance cannot be updated, but it is not a requirement. I also understand I made a mountain out of a molehill with this problem as it can be solved by much less code. In what ways can I improve this code other than the limitations mentioned? #include <istream> #include <iostream> #include <iomanip> class Account { public: Account() : mBalance(0.0) {} void updateBalance(double transaction) { mBalance += transaction; } double getBalance() { return mBalance; } private: double mBalance; }; class Transaction { public: Transaction() : mDebit(0) , mCredit(0.0) {} int getDebit() { return mDebit; } double getCredit() { return mCredit; } friend std::istream& operator>>(std::istream& input, Transaction& transaction) { input >> transaction.mDebit; input >> transaction.mCredit; return input; } private: int mDebit; double mCredit; }; class ATM { public: ATM() : mAccount() , mMinDenomination(5) , kWithdrawal_fee(0.50) {} void processTransaction(Transaction& transaction) { credit(transaction); debit(transaction); } void displayBalance() { std::cout << mAccount.getBalance() << '\n'; } private: Account mAccount; int mMinDenomination; const double kWithdrawal_fee; bool debit(Transaction& transaction) { if(isWithdrawable(transaction.getDebit())){ mAccount.updateBalance(-1*(transaction.getDebit() + kWithdrawal_fee)); return true; } return false; } void credit(Transaction& transaction) { if(transaction.getCredit() > 0) { mAccount.updateBalance(transaction.getCredit()); } } bool isWithdrawable(int transaction) { if(transaction % mMinDenomination == 0) { return mAccount.getBalance() >= transaction + kWithdrawal_fee; } return false; } }; int main() { std::iostream::sync_with_stdio(false); std::cout << std::setprecision(2) << std::fixed; Transaction transaction; ATM atm; std::cin >> transaction; atm.processTransaction(transaction); atm.displayBalance(); return 0; }  - 3 Answers Design. You use a mixture of int and doubles to represent monatary units. This is not a good idea. double (like all fixed with decimal representations, can not hold all values exactly). You should use an integer like type (where all values are represented exactly). If you are in America and using dollars and cents then I would use an integer but the balance of the account is held in cent. When you print it out you can then place the decimal point in the correct place. Code Review In: class Account {  I always think getters are wrong. They break encapsulation. Looking forward in your code you use them for two reasons. 1) Printing. 2) To test if the account has enough funds for withdraw. In both cases you should add explicit methods.  double getBalance() { return mBalance; }  I would replace the above with:  friend std::ostream& operator<<(std::ostream& s, Account const& data) { // Assuming you changed (as suggested above to hold account balance in cent. s << "$" << data.mBalance / 100 << "." << data.mBalance % 100;
}

virtual bool canWithdraw(double amount)
{
return mBalance > amount;
}


This logic protects you against future improvements to the system. What happens if you add the ability of some accounts to go overdrawn (for a fee). Then in your code you have to find all locations where the balance is being checked and modify those. In the method I propose you only need to modify one place (the Account class). You have localized the test for whether the account can withdraw money.

In:

A debit is an integer and a credit is a double.
I don't understand the logic here.

    int     mDebit;
double  mCredit;


They should be the same. If you have some compelling reason for the difference then I need a big comment about why they are different (you may have a good reason, but you will need to explain it in the code).

Personally I would just have an amount. A negative amount is a debt and positive amount a credit.

Getters. Ahhh. horrible.

    int getDebit() {
return mDebit;
}

double getCredit() {
return mCredit;
}


Again the only use is do tests and fiddling that should be part of the Accounts responsibility. You should send the transaction to the account which may reject the transaction if it fails any of the account specific validations (ie you can have a negative balance).

Like this.

    friend std::istream& operator>>(std::istream& input, Transaction& transaction) {
input >> transaction.mDebit;
input >> transaction.mCredit;
return input;
}


But usually when you have an input stream reader you also have an output stream writer that mirrors the reader. So when you persist to a stream the class can also read the value in.

In ATM:

Interesting. You have a debit action and credit action applied for every transaction. Does this mean that a transaction can perform both operations?

    void processTransaction(Transaction& transaction) {
credit(transaction);
debit(transaction);
}


Its OK to have a print method().

    void displayBalance() {
std::cout << mAccount.getBalance() << '\n';
}


But usually it is best for this to just call the stream operator.

    void displayBalance() {
std::cout << mAccount; // The account should know how to serialize itself.
}


This shows how bad an idea it is to have functions that have success state.

    bool debit(Transaction& transaction) {
if(isWithdrawable(transaction.getDebit())){
mAccount.updateBalance(-1*(transaction.getDebit() + kWithdrawal_fee));
return true;
}
return false;
}


You do it all correctly yet it is still broken. Because the calling code does not check the return value. Yes internally within a class it is absolutely fine to return status codes (because you do not expose the interface publicly). But you must also make sure you do actually test the result codes.

Note: It is never (very rarely) OK to expose status codes that need checking publicly. As we can see in the C world (were this practice is the norm)it is so easy to not check the error codes and thus invalidate any following code. You should write code so it can not be used incorrectly which means forcing your users to do the correct thing (or the program exits (exceptions)).

-
Thank you for the response. The main reason for the int versus double was the description of the input values on the problem: Positive integer 0 < X <= 2000 - the amount of cash which Pooja wishes to withdraw. Nonnegative number 0<= Y <= 2000 with two digits of precision - Pooja's initial account balance. Then they go on to give a few examples such as 30 120.00. Part of the design choice was based on the input examples. A debit would never be received without the initial balance or credit. – M K Aug 18 '14 at 18:52
I'm not arguing that an int isn't a better choice, that was a beginner mistake on my part. – M K Aug 18 '14 at 18:57
@Shaktal: Get/Set (er) break encapsulation and expose the internal implementation details. I would rather have a method called update(Transaction& trans) that modified the account. – Loki Astari Aug 18 '14 at 21:43
@Shaktal: Get/Set (er) are popular in languages like Java/C# because they allow frameworks to automatically serialize/deserialize a class without having to write any code. But they also lock you into an implementation that makes it exceedingly hard to update the code and improve it. – Loki Astari Aug 18 '14 at 21:46
@Shaktal: Now that being said sometimes they are good and simplify the code; so saying you can't use them is just bad response. Its just that they are over used by the inexperienced and there is usually a better way to do it (as in this case). What I would say is. If you see a get/set (er) in your class I would ask why am I exposing the internal details of this class. Usually this is because you are printing (write a serializer) or you are getting multiple parts of the object performing a calculation and putting it back (write an action that is part of the class). That will simplify your code. – Loki Astari Aug 18 '14 at 21:50

This looks quite readable and easy to follow. I just have a few things that have stuck out to me:

• It would not be good to use a floating-point type for currency, and it's especially worse for a more accurate type like a double. For instance, you can wind up with a value like 0.0001, which is not an ideal value to deal with. Unfortunately, C++ doesn't have a standard library to deal with this, such as Java's BigDecimal. You need an integer type for this, which avoids these problems.

Refer to this, this and this for more info. There are already many resources on this very topic.

• Your "getters" should be const as they're not supposed to modify data members:

int getSomeMember() const {
return someMember;
}


mMinDenominations should also be const for the same reason. Initializer lists do also allow initialization of const members (be aware that constructors cannot do this).

Moreover, consider redesigning this to avoid getters. They (and also setters) are generally bad for encapsulation as they expose implementation details. You may not even need them here, either.

• You can replace displayBalance() with an overload of operator<<:

friend std::ostream& operator<<(std::ostream& out, ATM const& obj) {
return out << obj.mAccount.getBalance() << '\n';
}


Since mAccount is private, this will have to be defined within the class. This will still work, even if you remove the getters.

-
Thank you for the response. Yes const-correctness is something I fail to do, I need to work on making const review part of every function or member I write. Would a way around the accuracy be to multiply the initial balance say 123.45 by 100 then divide by 100.00 when displaying the balance back out? – M K Aug 18 '14 at 15:43
@MK: I believe that could work. You may have to try this with some numbers yourself while making sure that your machine is giving the desired results. – Jamal Aug 18 '14 at 16:04
Alternatively, use std::money_get and std::money_put. These store the value in a long double, but also keep track of the correct multiplier to use, so (for example) Dollars, Euros or Pounds will be multiplied by 100, but Yen will probably be multiplied by 1. Of course, they also have the advantage of being more explicit about the intent of what you're doing. – Jerry Coffin Aug 18 '14 at 17:09
@JerryCoffin: I did not know about that! If that would be applicable here, then you may post an example in an answer. – Jamal Aug 18 '14 at 17:13
Note: debit and credit are both verbes. I can credit your account with $10 or I can debit your account by the amount of$15. – Loki Astari Aug 18 '14 at 17:59

I have reworked my initial code to the best of my ability trying to take into account all recommendations:

#include <istream>
#include <iostream>
#include <string>

class Account {
public:
Account()
:   mBalance(0)
{}

void updateBalance(const int amount) {
mBalance += amount;
}

friend std::ostream& operator<<(std::ostream& output, const Account& account) {
output << account.mBalance / 100 << "." << account.centsToString();
return output;
}

bool canWithdraw(const int amount) {
return mBalance > amount;
}

private:
int mBalance;

// In the case of a single digit return of % this function prepends 0
std::string centsToString() const {
if(mBalance % 100 >=0 && mBalance % 100 < 10) {
return "0" + std::to_string(mBalance % 100);
}
else {
return std::to_string(mBalance % 100);
}
}
};

class Transaction {
public:
Transaction()
:   mDebit(0)
,   mInitialBalance(0.0)
,   kCentsInDollar(100)
{}

// Reads a debit(int) and initial balance(double)
friend std::istream& operator>>(std::istream& input, Transaction& transaction) {
input >> transaction.mDebit;
input >> transaction.mInitialBalance;
return input;
}

int getInitialBalanceInCents() const {
return (int)(mInitialBalance * kCentsInDollar);
}

int getDebitInCents() const {
return mDebit * kCentsInDollar;
}

private:
int     kCentsInDollar;
int     mDebit;
double  mInitialBalance;
};

class ATM {
public:
ATM()
:   mAccount()
,   kMakeTransNegative(-1)
,   kMinDenomination(500)   // Min \$5 denomination
,   kWithdrawal_fee(50)     // 50 cents
{}

// Transaction comes in as a debit and initial balance so
// both require processing
void processTransaction(const Transaction& transaction) {
depositInitialBalance(transaction);
debit(transaction);
}

void displayBalance() const {
std::cout << mAccount << '\n';
}

private:
Account             mAccount;
const int           kMinDenomination;
const int           kWithdrawal_fee;
const int           kMakeTransNegative;

void depositInitialBalance(const Transaction& transaction) {
if(transaction.getInitialBalanceInCents() > 0) {
mAccount.updateBalance(transaction.getInitialBalanceInCents());
}
}

void debit(const Transaction& transaction) {
if(transaction.getDebitInCents() % kMinDenomination == 0
&& mAccount.canWithdraw(transaction.getDebitInCents() + kWithdrawal_fee)) {
mAccount.updateBalance(kMakeTransNegative*(transaction.getDebitInCents() + kWithdrawal_fee));
}
}
};

int main() {
std::iostream::sync_with_stdio(false);

Transaction transaction;
ATM         atm;

// Transaction reads in an int specifying the debit amount and then a double
// representing the initial balance on the account.
std::cin >> transaction;
atm.processTransaction(transaction);
atm.displayBalance();

return 0;
}

-