PDA

View Full Version : C++ Confusion: Bank Transaction Test




Josh
Apr 9, 2006, 09:00 PM
Hi,

I'm writing a small program that takes an input file such as this:

Checking Deposit 50.00
Savings Deposit 100.00
Savings Withdrawal 21.50
Checking Transfer 42.68
Checking Deposit 14.95
Checking Withdrawal 25.00
Savings Deposit 1.25
Checking Deposit 25.00


then processes the info, adding or subtracting to the balance of the checking or savings account as needed.

Ideally, the output should be similar to this:
Welcome to the Bank!

Please enter your name: Mark

Transaction Amount Savings Checking
-------------------- ---------- -------- -----------
Checking Deposit 50.00 0.00 50.00
Savings Deposit 100.00 100.00 50.00
Savings Withdrawal 21.50 78.50 50.00
Checking Transfer 42.68 121.18 7.32
Checking Deposit 14.95 121.18 22.27
Checking Withdrawal 25.00 *Insufficient Funds*
Savings Deposit 1.25 122.43 22.27
Checking Deposit 25.00 122.43 47.27

Thanks for visiting the bank Mark


My program will consist of 3 additional boolean functions (titled: deposit, withdrawl, transfer) to handle the processing of this information (seems silly to me why these are boolean; checking to see if it will be succesful should happen before the function is called, IMO).

I'm currently just testing the deposit function on the checking account, seeing if I get the correct total deposit balance at the end, and prints "Success!" for each succesful run.

So far the code I've got is giving me an incorrect answer. I've stared at this for a very long time, and it usually takes a fresh look to spot the problem, so if you can, I'd appreciate any help. I should be getting around 200 as the answer, but it shows me "0." For some reason, the checking_balance is not being changed, even though I used 'float &balance' in my function :/

Here is my code so far (please ignore the 'using namespace std;'...bugs me too, but the prof makes us use it):

#include <iostream>
#include <iomanip>
#include <fstream>
#include <string>
using namespace std;

bool deposit(float,float); // processes deposits
bool withdrawal(float,float); // processes withdrawals
bool transfer(float,float,float); // processes transfers

int main()
{ string user,filename;
fstream transaction_file;
float checking_balance,savings_balance;
float amount;
string account, action;

// set both balances to $0.00

checking_balance = 0.00;
savings_balance = 0.00;

// begin, prompt for name, and open the corresponding file

cout << "Welcome to the Bank!\n\n"
<< "Please enter your name: ";
cin >> user;

filename = "/Users/Josh/Desktop/"+user+".txt";

transaction_file.open(filename.data(),ios::in);

while(!transaction_file.eof() && !transaction_file.fail())
{ transaction_file >> account,action,amount;
if(account == "Checking" && action == "Deposit")
if(deposit(checking_balance,amount))
cout << "Success!" << endl;

}



// close the file

transaction_file.close();

cout << "Total amount of deposits = " << checking_balance << endl;


return 0;
}

bool deposit(float &balance,float amount)
{ if(amount >= 1)
{ balance += amount;
return (true);
}
else
return (false);
}



superbovine
Apr 9, 2006, 11:19 PM
Just a guess.
shouldn't your prototype be:


deposit(&float, float);

Palad1
Apr 10, 2006, 04:40 AM
Let's add some logging, shall we?

#define DEBUG 1
while(!transaction_file.eof() && !transaction_file.fail())
{
transaction_file >> account,action,amount;

#ifdef DEBUG
cerr<<"account :("<<account<< ") action : ("<<action<<") amount("<<amount<<")"<<endl;
#endif

if(account == "Checking" && action == "Deposit")
{
#ifdef DEBUG
cerr <<"Cheking deposit"<<endl;
#endif
if(deposit(checking_balance,amount))
{
cout << "Success!" << endl;
}
}
}



// close the file

transaction_file.close();

cout << "Total amount of deposits = " << checking_balance << endl;


return 0;
}

bool deposit(float &balance,float amount)
{
if(amount >= 1.0F) // I think you may have had a casting problem as well
{
#if DEBUG
cerr<<" deposit"<<endl;
#endif
balance += amount;
return true;
}
else
{
#if DEBUG
cerr<<" deposit <1 "<<endl;
#endif
return false;
}
}


I think your problem may lie in the parsing, you don't really check what you have, adding some logging will make your code easier to understand at runtime without using a debugger.

Also, if( balance >=1) may pose a problem, as you are checking a float against an int, although the casting operator should be implicitly called, it never hurts to be correct : if (balance >= 1F)

A final point: use curly braces, or don't, but don't mix, it makes code less readeable:

if(foo){
dobar()
woot(1);
}else
woot(-1);
// vs
if(foo)
{
dobar();
woot(1);
}
else
{
woot(-1);
}


Well, you may not like the way curly braces are distributed, so just choose your style, then BE CONSISTENT ;)

Hope that helps,
Palad1

Josh
Apr 10, 2006, 01:12 PM
Thanks for the help guys, I really appreciate it!

(float &) was the problem. I also put the account actions/amounts/balances into arrays to make things go smoother.

Thanks again :)

superbovine
Apr 10, 2006, 01:41 PM
Thanks for the help guys, I really appreciate it!

(float &) was the problem. I also put the account actions/amounts/balances into arrays to make things go smoother.

Thanks again :)

for smaller projects, you can move function between your prototypes and int main(). Might make it easier to catch those errors. You could also just encapsulate them in .h file as well.

Josh
Apr 10, 2006, 01:47 PM
for smaller projects, you can move function between your prototypes and int main(). Might make it easier to catch those errors. You could also just encapsulate them in .h file as well.

I will definitely keep that in mind. Glad I have MR to fall back on when I get stuck :o

Although I'm very new at C++ (and programming in general), am I right in thinking that having the transaction functions in this project being boolean is illogical?

The way were are supposed to use them in this project, is to compute the balance (either subtract from or add to it) givin the ammount and the balance, then return 'true' if that was possible (no amounts were negative, and the request was not larger than the balance). Otherwise, no values would be changed, and 'false' would be returned.

Then, in the main program, if the function returned true, it would would display the details (type of transaction, amount, balance). If false, it would display "insufficient funds" or something similar.

Would it not make more sense to have the function as void, and then just call the function, and let *it* decide which text to display? That not only saves repetition, but I cannot think of one reason why the other way would be better :confused:

Here is a snippet to show you want I mean:

if(account[i]=="Checking" && action[i]=="Withdrawal")
{ cout << left << setw(25) << "Checking Withdrawal";
if(withdrawal(checking_balance,amount[i]))
{ cout << right << setw(8) << amount[i]
<< right << setw(13) << savings_balance
<< " " << right << setw(13) << checking_balance << endl;
}
else
{ cout << right << setw(8) << amount[i]
<< right << setw(27) << "*Insufficient Funds*" << endl;
}


--- the function --

bool withdrawal(float &balance,float amount)
{ if(amount >= 0.0 && amount <= balance)
{ balance -= amount;
return (true);
}
else
return (false);
}



Considering there are three types of functions (deposit, withdrawal, transfer) and two different accounts, the amount of code duplication is pretty high.

I'd think something roughly like the example below would be more effecient:

if(account[i]=="Checking" && action[i]=="Withdrawal")
{ withdrawal(checking_balance,amount[i]);

....
}


---- the function ---

void withdrawal(float &balance,float amount)
{ if(amount >= 0.0 && amount <= balance)
{ balance -= amount;
cout << right << setw(8) << amount[i]
<< right << setw(13) << savings_balance
<< " " << right << setw(13) << checking_balance << endl;
}
else
cout << "Insufficient funds" << endl;
}

Thom_Edwards
Apr 10, 2006, 02:08 PM
i've had this same discussion with many people on many different occasions. 'there is more than one way to skin a cat' is what i hear pretty often. but, the most intelligent answer (at least as it seems to me) is that the main shouldn't have to worry about the business logic. the function should be a 'black box' so that you don't know how it does anything; it just sends back a true if what it did was successful. main shouldn't care about whether you can do it or not (or, again, what it is even doing) but only if it was a success. in a more complex project, you would probably design a class to handle all output, so putting that in this function wouldn't 'fit.'

what if, in another part of a complex program, you didn't want the function to display anything? then you have to write a whole new set of functions (and yes, you could overload, but that's another conversation) that just do the math and no output. keeping everything 'loosely coupled' encourages reusability for you functions. each function should have only one, well, function! putting too much in a single function can cause reusability issues later on (when the projects get bigger).

i hope someone else will join in on this topic and correct or verify my thoughts on this.

Josh
Apr 10, 2006, 02:22 PM
what if, in another part of a complex program, you didn't want the function to display anything? then you have to write a whole new set of functions (and yes, you could overload, but that's another conversation) that just do the math and no output. keeping everything 'loosely coupled' encourages reusability for you functions. each function should have only one, well, function! putting too much in a single function can cause reusability issues later on (when the projects get bigger).


You do make a good point.

But if this program for example was to not show output, the way we've been asked to write it would require removing the 6 blocks of identical output code.

The other way I propose would involve removing only 2 indentical blocks of code (the transaction function calls upon deposit() and withdrawal() to move the money).

Here is the code in its entirety so you can see the duplication that bugs me:

/* Program 10 by Josh Stevens 4/4/2006
** Asks user for a name, and then processes
** various bank transactions on the user's account
** based on a transaction file.
*********************************************************/

#include <iostream>
#include <iomanip>
#include <fstream>
#include <string>
using namespace std;

bool deposit(float &,float); // processes deposits
bool withdrawal(float &,float); // processes withdrawals
bool transfer(float &,float &,float); // processes transfers

const int MAX = 50; // maximum number of expected transactions

int main()
{ string user,filename;
string account[MAX], action[MAX];
fstream transaction_file;
float amount[MAX]; // amount of the transaction
float checking_balance,savings_balance;
int i; // counter
int transactions; // the number of transactions taking place

// set balances to $0.00, transactions to 0

checking_balance = 0.00;
savings_balance = 0.00;
transactions = 0;

// set transaction details to have no values

for(i=0;i<MAX;i++)
{ amount[i] = 0.00;
account[i] = "null";
action[i] = "null";
}

// begin and prompt for name

cout << "Welcome to the Bank!\n\n"
<< "Please enter your name: ";
cin >> user;

filename = "C:\\"+user+".txt";

// open the file

transaction_file.open(filename.data(),ios::in);

// read the file into the arrays

while(!transaction_file.eof() && !transaction_file.fail())
{ for(i=0;i<MAX;i++)
{ transaction_file >> account[i] >> action[i] >> amount[i];
}
}

// close the file

transaction_file.close();

// count number of transactions for future loops

for(i=0;i<MAX && account[i] != "null";i++)
transactions++;

// set up display

cout << "\nTransaction Amount Savings Checking" << endl
<< "------------------- -------- --------- ----------" << endl;

// process transactions and ouput the results

cout << fixed << setprecision(2);

for(i=0;i<transactions;i++)

// checking deposit

{ if(account[i]=="Checking" && action[i]=="Deposit")
{ cout << left << setw(25) << "Checking Deposit";
if(deposit(checking_balance,amount[i]))
{ cout << right << setw(8) << amount[i]
<< right << setw(13) << savings_balance
<< " " << right << setw(13) << checking_balance << endl;
}
}

// savings deposit

if(account[i]=="Savings" && action[i]=="Deposit")
{ cout << left << setw(25) << "Savings Deposit";
if(deposit(savings_balance,amount[i]))
{ cout << right << setw(8) << amount[i]
<< right << setw(13) << savings_balance
<< " " << right << setw(13) << checking_balance << endl;
}
}

// checking withdrawal

if(account[i]=="Checking" && action[i]=="Withdrawal")
{ cout << left << setw(25) << "Checking Withdrawal";
if(withdrawal(checking_balance,amount[i]))
{ cout << right << setw(8) << amount[i]
<< right << setw(13) << savings_balance
<< " " << right << setw(13) << checking_balance << endl;
}
else
{ cout << right << setw(8) << amount[i]
<< right << setw(27) << "*Insufficient Funds*" << endl;
}
}

// savings withdrawal

if(account[i]=="Savings" && action[i]=="Withdrawal")
{ cout << left << setw(25) << "Savings Withdrawal";
if(withdrawal(savings_balance,amount[i]))
{ cout << right << setw(8) << amount[i]
<< right << setw(13) << savings_balance
<< " " << right << setw(13) << checking_balance << endl;
}
else
{ cout << right << setw(8) << amount[i]
<< right << setw(27) << "*Insufficient Funds*" << endl;
}
}

// checking transfer (from checking to savings)

if(account[i]=="Checking" && action[i]=="Transfer")
{ cout << left << setw(25) << "Checking Transfer";
if(transfer(checking_balance,savings_balance,amount[i]))
{ cout << right << setw(8) << amount[i]
<< right << setw(13) << savings_balance
<< " " << right << setw(13) << checking_balance << endl;
}
else
{ cout << right << setw(8) << amount[i]
<< right << setw(27) << "*Insufficient Funds*" << endl;
}
}

// savings transfer (from savings to checking)

if(account[i]=="Savings" && action[i]=="Transfer")
{ cout << left << setw(25) << "Checking Transfer";
if(transfer(savings_balance,checking_balance,amount[i]))
{ cout << right << setw(8) << amount[i]
<< right << setw(13) << savings_balance
<< " " << right << setw(13) << checking_balance << endl;
}
else
{ cout << right << setw(8) << amount[i]
<< right << setw(27) << "*Insufficient Funds*" << endl;
}
}
}

// thank user and quit

cout << "\nThank you for visiting the bank, " << user << endl;

return 0;
}

/* bool() takes source balance, destination balance, and the amount of the transaction.
** The amount is subtracted from the source and then added to the destination.
** Calls upon deposit() and withdrawal() to move the money.
***************************************************************************************/

bool transfer(float &source,float &destination, float amount)
{ if(amount >= 0.0 && amount <= source)
{ withdrawal(source,amount);
deposit(destination,amount);
return (true);
}
else
return (false);
}

/* deposit() takes the account balance and transaction amount as parameters.
** If the amount is a positive number, it is added to the balance.
** Otherwise, it does nothing and returns (false).
****************************************************************************/

bool deposit(float &balance,float amount)
{ if(amount >= 0.0)
{ balance += amount;
return (true);
}
else
return (false);
}

/* withdrawal() takes the account balance and ammount as parameters.
** If the amount is positive, and less than the balance, the ammounts is
** subtracted from the balance. Otherwise, nothing happens and (false) is
** returned.
****************************************************************************/

bool withdrawal(float &balance,float amount)
{ if(amount >= 0.0 && amount <= balance)
{ balance -= amount;
return (true);
}
else
return (false);
}

AlmostThere
Apr 10, 2006, 02:37 PM
void withdrawal(float &balance,float amount)
{ if(amount >= 0.0 && amount <= balance)
{ balance -= amount;
cout << right << setw(8) << amount[i]
<< right << setw(13) << savings_balance
<< " " << right << setw(13) << checking_balance << endl;
}
else
cout << "Insufficient funds" << endl;
}


You are dumping the amount, saving's balance and checking balance for every error, regardless of cause. Suppose there are a hundred different account type and a hundred different transactions (and given the combinations of bank accounts, tax rates and interest rates that is not an outrageous proposition), are you going to dump all that information whenever there is an error?

Instead, store the transaction type, balance and account type in variables along with the transaction result. Then write a single reporting function that will tell you what transaction went wrong, with which account and what the balance currently is.

It is also trivial to redirect the output to a file or such like if you need to run 1000s of transactions and check them later.

MarkCollette
Apr 11, 2006, 03:07 AM
Have you learned about classes yet? As a rule, if you're using parallel arrays, such as account, action, and amount, then you should make a class with fields for account, action, and amount, and have one single array of that class. Or better yet, since you're using STL string objects, you should make a vector of that class.

Also, there are 2 accounts, and 3 actions, so instead of doing 6 if statements like:

if(account[i]=="Checking" && action[i]=="Deposit") {
}
else if(account[i]=="Savings" && action[i]=="Deposit") {
}

instead do:

if(action[i]=="Deposit") {
if(account[i]=="Checking") {
deposit(checking_balance,amount[i]);
}
else if(account[i]=="Savings") {
deposit(savings_balance,amount[i]);
}
}

Or better yet, when you parse in the account, you could test right away for the account type and instead of storing the string, store an integer constant, say 0 for Savings and 1 for Checking.


const int ACCOUNT_SAVINGS = 0;
const int ACCOUNT_CHECKING = 1;
const int NUM_ACCOUNTS = 2;
...
int account[MAX];
// Replaces checking_balance,savings_balance
int balances[NUM_ACCOUNTS];
...
if(action[i]=="Deposit") {
deposit(balances[account[i]],amount[i]);
}


And the reason why transfer,withdrawl, and deposit are all bools, instead of checking first, is that bank transactions have to be atomic, meaning that the test for money has to happen right then and there, or else someone could use two different ATMs, and ask for their full balance at both, and get twice the money they should be able to.

Also, you should test your inputs right when you read them in. So, if the file says "Checkfgjsdhg" instead of "Checking", you should fail that input file before proceeding, unlike right now, where it's right when you do the processing of calling transfer,deposit or withdraw.

Another reason for checking input data right away, is so you can decouple the parsed file representation from the internally stored representation. Remember how I showed that storing the account as an integer instead of a string made things easier? Well that decoupling of input format and internal format allows that. Also, what happens when your code has to input data from multiple sources? Like from a web page, or an XML file, or a database.

Josh
Apr 11, 2006, 12:11 PM
^^ thanks a lot - lot of great info there that definitely makes more sense.

We *just* learned about classes yesterday evening, right after needing to hand in that assignment. :o

MarkCollette
Apr 11, 2006, 12:52 PM
^^ thanks a lot - lot of great info there that definitely makes more sense.

We *just* learned about classes yesterday evening, right after needing to hand in that assignment. :o

No problem. It's quite typical to give an assignment that would benefit from classes before they teach classes, to make it all the more apparent how useful classes are. Same thing where you get an assignment doing math with 5 variables, and then learn about arrays right afterwards, etc. :)

superbovine
Apr 11, 2006, 01:40 PM
^^ thanks a lot - lot of great info there that definitely makes more sense.

We *just* learned about classes yesterday evening, right after needing to hand in that assignment. :o

just out of curiosity what compiler you guys using to grade the assignments.