patterncppMinor
Reading bank account numbers and balances from file, sorting in asccending order, using binary search to find balances accompanied by account number
Viewed 0 times
sortingreadingfileordernumbersearchnumbersbalancesaccountasccending
Problem
After writing this program and not having to much code written with functions I was looking for some advice. So how'd I do readability wise? Are the functions organized? Should they be organized differently? etc.
```
#include
#include
using namespace std;
void readData(int[], double[], ifstream & inputFile);
double assortData(int[], double[]);
void newFile(int[], double[], ofstream & outputFile);
int binarySearch(const int[], int, int);
void getAccNum(int &);
const int ARRAY_SIZE = 1000; // Global Array Size
int main()
{
int results; // BinarySearch result
int accNUM; // Account number entered by user
int accountNumbers[ARRAY_SIZE]; // Array with 1000 elements. Stores bank account numbers
double accountBalances[ARRAY_SIZE]; // Array with 1000 elements. Stores bank account balances
ifstream inputFile; // Input file stream object
ofstream outputFile; // Output file stream object
// Open the file storing bank account numbers and corresponding account balances. Read the data
inputFile.open("FinalNumbers.txt");
readData(accountNumbers, accountBalances, inputFile);
// Close the file when done with it.
inputFile.close();
// Assorts account numbers from file into ascending order
assortData(accountNumbers, accountBalances);
// Opens another file and puts sorted data into the file
outputFile.open("Final_Random_Accounts_Sorted_Mazzei_Chris.txt");
newFile(accountNumbers, accountBalances, outputFile);
// Close the file when done with it.
outputFile.close();
// Gets account number from user input.
getAccNum(accNUM);
// Gets account balance corresponding to users bank account number
results = binarySearch(accountNumbers, ARRAY_SIZE, accN
```
#include
#include
using namespace std;
void readData(int[], double[], ifstream & inputFile);
double assortData(int[], double[]);
void newFile(int[], double[], ofstream & outputFile);
int binarySearch(const int[], int, int);
void getAccNum(int &);
const int ARRAY_SIZE = 1000; // Global Array Size
int main()
{
int results; // BinarySearch result
int accNUM; // Account number entered by user
int accountNumbers[ARRAY_SIZE]; // Array with 1000 elements. Stores bank account numbers
double accountBalances[ARRAY_SIZE]; // Array with 1000 elements. Stores bank account balances
ifstream inputFile; // Input file stream object
ofstream outputFile; // Output file stream object
// Open the file storing bank account numbers and corresponding account balances. Read the data
inputFile.open("FinalNumbers.txt");
readData(accountNumbers, accountBalances, inputFile);
// Close the file when done with it.
inputFile.close();
// Assorts account numbers from file into ascending order
assortData(accountNumbers, accountBalances);
// Opens another file and puts sorted data into the file
outputFile.open("Final_Random_Accounts_Sorted_Mazzei_Chris.txt");
newFile(accountNumbers, accountBalances, outputFile);
// Close the file when done with it.
outputFile.close();
// Gets account number from user input.
getAccNum(accNUM);
// Gets account balance corresponding to users bank account number
results = binarySearch(accountNumbers, ARRAY_SIZE, accN
Solution
This is a straightforward way to implement this. I'm able to read it and understand what it does. I appreciate that you made
Objects
C++ is an object-oriented language. The beauty of object oriented programming is that it allows you to come up with sensible abstractions for the things (objects) that you want to represent and the messages (methods) they pass to each other to accomplish a task. You can see this with the
For example, you have parallel arrays for the account numbers and the account balances. But wouldn't it make more sense to have an account class that holds the account number and the balance? Then you could have a single array of accounts. It would make your
Additionally, C++ comes with a wide variety of very useful classes. You're already using some of them from the standard template library. I recommend you look into
Avoid
There are many people who can explain this far better than I can. Here's a few.
Error Handling
Your
Don't Put Too Many Things In a Conditional
In your
Also, in your version, if a balance or account number is 0, will that end the loop? (I haven't tried it, but it seems like it could.) I mistakenly thought that the
Update: So, as you can see, it's hard to get these things right. As Loki Astari pointed out, I messed it up not once, but twice (so far!). My main point, though, is that while putting a stream read into the condition of the
Use Braces Even For 1-Line Expressions
In
ARRAY_SIZE a constant and not a #define! You've nicely broken things like reading the data from the files into separate functions, which is great. I think you could make it better by doing some of the following things.Objects
C++ is an object-oriented language. The beauty of object oriented programming is that it allows you to come up with sensible abstractions for the things (objects) that you want to represent and the messages (methods) they pass to each other to accomplish a task. You can see this with the
ifstream and ofstream classes that have methods for opening and reading data from files. You should do something similar.For example, you have parallel arrays for the account numbers and the account balances. But wouldn't it make more sense to have an account class that holds the account number and the balance? Then you could have a single array of accounts. It would make your
assortData() function much simpler because you wouldn't be trying to keep 2 different arrays in synch with one another.Additionally, C++ comes with a wide variety of very useful classes. You're already using some of them from the standard template library. I recommend you look into
std::vector. They are similar to C arrays, but can be sized dynamically. There are also functions for sorting them so you don't have to write your own. (Though it's always good to write these things at least once to make sure you have a solid understanding of how they work.)Avoid
using namespace stdThere are many people who can explain this far better than I can. Here's a few.
Error Handling
Your
binarySearch function returns an error if it doesn't find the account. That's great! Now what about the other functions? What if readData() runs into an error with the input file? (Like maybe there aren't enough entries in the file, or there are too many?) What if the user enters an invalid account number (like one that has letters or symbols in it)?Don't Put Too Many Things In a Conditional
In your
readData() function the condition on your while statement is fairly confusing. It is checking that the count is within range, and it's reading the input into the numbers array, and into the balances array. I would make this clearer by checking that we haven't reached the end of the array, and that the input is valid by doing something like this:bool inputValid = true;
while ((count > numbers [ count ]);
inputValid &= (input >> balances [ count ]);
inputValid &= (balances [ count ] != 0);
if (inputValid)
{
count++;
}
}Also, in your version, if a balance or account number is 0, will that end the loop? (I haven't tried it, but it seems like it could.) I mistakenly thought that the
operator>>() would return the value in question, but in fact it returns a reference to the stream you're reading from. As such, if you want to check the validity, you need to explicitly check that the balance you read in was not 0.Update: So, as you can see, it's hard to get these things right. As Loki Astari pointed out, I messed it up not once, but twice (so far!). My main point, though, is that while putting a stream read into the condition of the
while loop may work, it's going to get cumbersome quickly as your data model grows. Right now it's only 2 pieces of data, but in the future, you might have names and addresses or phone numbers, or linked accounts, etc. Also, if you want to report back specific errors, it can get hard if all you know is that 1 of 2 or 5 or 10 pieces of data was bad. Which one? Is it recoverable?Use Braces Even For 1-Line Expressions
In
assortData() you don't have curly braces around either for loop. While this is legal, it can lead to bugs when some future programmer (including you) decides to add 1 more statement to one of the loops. If they don't realize there are no braces, they will likely forget to add them, thus changing the meaning of the loop. Everything after the new statement will not be part of the loop. Or if they try to put it at the end, it will execute only once, and not on every iteration.Code Snippets
bool inputValid = true;
while ((count < ARRAY_SIZE) && (inputValid))
{
inputValid = (input >> numbers [ count ]);
inputValid &= (input >> balances [ count ]);
inputValid &= (balances [ count ] != 0);
if (inputValid)
{
count++;
}
}Context
StackExchange Code Review Q#162800, answer score: 6
Revisions (0)
No revisions yet.