Become a MacRumors Supporter for $50/year with no ads, ability to filter front page stories, and private forums.

Josh

macrumors 68000
Original poster
Mar 4, 2004
1,640
1
State College, PA
Hi,

I'm writing a small program that takes an input file such as this:
Code:
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:
Code:
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):
Code:
#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);
}
 

Palad1

macrumors 6502a
Feb 24, 2004
647
0
London, UK
Let's add some logging, shall we?
Code:
#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:
Code:
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

macrumors 68000
Original poster
Mar 4, 2004
1,640
1
State College, PA
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

macrumors 68030
Nov 7, 2003
2,872
0
Josh said:
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

macrumors 68000
Original poster
Mar 4, 2004
1,640
1
State College, PA
superbovine said:
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 :eek:

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:
Code:
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:
Code:
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

macrumors regular
Apr 11, 2003
240
0
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

macrumors 68000
Original poster
Mar 4, 2004
1,640
1
State College, PA
Thom_Edwards said:
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:
Code:
/* 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);
}
 
Josh said:
Code:
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

macrumors 68000
Mar 6, 2003
1,559
36
Toronto, Canada
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:

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

instead do:

Code:
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.

Code:
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

macrumors 68000
Original poster
Mar 4, 2004
1,640
1
State College, PA
^^ 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. :eek:
 

MarkCollette

macrumors 68000
Mar 6, 2003
1,559
36
Toronto, Canada
Josh said:
^^ 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. :eek:

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

macrumors 68030
Nov 7, 2003
2,872
0
Josh said:
^^ 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. :eek:

just out of curiosity what compiler you guys using to grade the assignments.
 
Register on MacRumors! This sidebar will go away, and you'll see fewer ads.