patterncppMinor
Several math calculation functions
Viewed 0 times
severalcalculationfunctionsmath
Problem
The code I have created calculates an arrangement of math calculation functions, and I would like to see if there is any way I could improve it, especially the calculator function. The guide function also only works if you have the file on your computer so don't try using it.
#include
#include
#include
#include
#include
using namespace std;
double getAverage(int amount, int numbers[]){
// Declare the variables
double total = 0;
double avg = 0;
// Find each number in the array then add it to the total
for (int i = 0; i > choice;
check();
if (choice > totVar;
check();
// Check if totVar is to big or to small
if (totVar >= 1000 || totVar > userNums[i];
check();
varNum++;
}
//Set the avg variable to the average (for the deviation function)
avg = getAverage(totVar, userNums);
//Sort the find the median
sort(userNums, userNums + totVar);
median = (userNums[totVar / 2] + userNums[(totVar / 2) + 1]) / 2;
// Call the average function and store the output in the avg variable for use in the deviation
if (choice == 1)
cout > sym;
if (sym == "+" || sym == "-" || sym == "*" || sym == "/") {
//Ask user for the first variable then record it and check it
cout > ch1;
check();
//Ask user for the second variable then record it and check it
cout > ch2;
check();
//Print the total of the calculation
cout > dec;
if (dec == 'y')
return main();
return 0;
}Solution
Overall I'd say this is pretty easy to read and understand, which is good. I think you could make a few improvements described below.
Functions
Your
This way the logic is more clear and uncluttered with details of getting input, running calculations and displaying output.
Magic Numbers
Right now, there are some magic numbers in your code. For example, the number
You should make named constants for each of these. For example:
Then in your functions, you'd use those constants instead of the bare numbers. Now, if you ever want to change the value of one, or add a new one in the middle of the list, you just put a new named constant in there, and the rest of your code is adjusted automatically the next time you compile.
C++
Even better than using a constant for the maximum number of user inputs would be to use
Furthermore, if you use a standard container, such as a
Separation of Concerns
In addition to breaking things into functions for readability, you should also break things out based on the type of work done. Most of your code mixes together getting input, doing calculations, and displaying results. Actually, you've broken out the calculations nicely. But the input and output should probably be separated, too. This seems natural because you have
Now, if you ever want to translate your program into another (human) language, you can simply change the functions that display user strings and not even touch the calculations or the input. This is sometimes referred to as "separating the business logic from the display logic."
Errors
Your median calculation is incorrect for odd numbers of inputs. You should check whether there were an even or odd number of values input and do the appropriate thing for each case. When there is an odd number of inputs, simply return the middle value.
Also your input validation is lacking. Checking that there is still input available is good, but you should also check to make sure the user didn't input something invalid, and if they did, you should alert them and get input again. Normally you'd have a loop that looks something like:
Functions
Your
main() function is very large. It should be broken up into functions that do specific things. For example, the main menu could be its own function that returns the user's choice. Getting the list of input values could be another function, and so on. It would make your main() look something like this:int main() {
int choice = getUserChoice();
vector userNums;
if (choice <= 5) {
getUserNums(userNums);
}
switch (choice) {
case 1:
performAverage(userNums);
break;
case 2:
performDeviation(userNums);
break;
// ... etc.
}
}This way the logic is more clear and uncluttered with details of getting input, running calculations and displaying output.
Magic Numbers
Right now, there are some magic numbers in your code. For example, the number
1000 is used twice. The values 1-7 are used in the various if statements in main(). If you ever want to change the number of values allowed in the array or you want to add more functions in the middle of your list, it's going to be painful to update the program.You should make named constants for each of these. For example:
const size_t kMaxUserNums = 1000;
enum {
kUC_Average = 1, // UC for user choice, for example
kUC_Deviation,
kUC_Sort,
kUC_Median,
kUC_All,
kUC_Calculator,
kUC_Guide
};Then in your functions, you'd use those constants instead of the bare numbers. Now, if you ever want to change the value of one, or add a new one in the middle of the list, you just put a new named constant in there, and the rest of your code is adjusted automatically the next time you compile.
C++
Even better than using a constant for the maximum number of user inputs would be to use
std::vector. It grows automatically and you can check its size by calling userNums.size(). It eliminates the possibility that you'll create an error in passing an incorrect size with the array to one of the calculation functions.Furthermore, if you use a standard container, such as a
std::vector, you can use the std::sort() function to sort it.Separation of Concerns
In addition to breaking things into functions for readability, you should also break things out based on the type of work done. Most of your code mixes together getting input, doing calculations, and displaying results. Actually, you've broken out the calculations nicely. But the input and output should probably be separated, too. This seems natural because you have
cin and cout, but really those are 3 distinct things. Those functions that I encouraged you to write should look something like this:int getUserChoice()
{
displayUserInstructions();
int choice = 0;
cin >> choice; // Only 1 line so doesn't need a separate function
check(); // This function could use a more descriptive name
return choice;
}Now, if you ever want to translate your program into another (human) language, you can simply change the functions that display user strings and not even touch the calculations or the input. This is sometimes referred to as "separating the business logic from the display logic."
Errors
Your median calculation is incorrect for odd numbers of inputs. You should check whether there were an even or odd number of values input and do the appropriate thing for each case. When there is an odd number of inputs, simply return the middle value.
Also your input validation is lacking. Checking that there is still input available is good, but you should also check to make sure the user didn't input something invalid, and if they did, you should alert them and get input again. Normally you'd have a loop that looks something like:
int choice = 0;
do {
displayUserInstructions();
cin >> choice;
if ((choice kUC_Guide)) {
displayInputError();
}
} while ((choice kUC_Guide));Code Snippets
int main() {
int choice = getUserChoice();
vector<int> userNums;
if (choice <= 5) {
getUserNums(userNums);
}
switch (choice) {
case 1:
performAverage(userNums);
break;
case 2:
performDeviation(userNums);
break;
// ... etc.
}
}const size_t kMaxUserNums = 1000;
enum {
kUC_Average = 1, // UC for user choice, for example
kUC_Deviation,
kUC_Sort,
kUC_Median,
kUC_All,
kUC_Calculator,
kUC_Guide
};int getUserChoice()
{
displayUserInstructions();
int choice = 0;
cin >> choice; // Only 1 line so doesn't need a separate function
check(); // This function could use a more descriptive name
return choice;
}int choice = 0;
do {
displayUserInstructions();
cin >> choice;
if ((choice < kUC_Average) || (choice > kUC_Guide)) {
displayInputError();
}
} while ((choice < kUC_Average) || (choice > kUC_Guide));Context
StackExchange Code Review Q#154661, answer score: 6
Revisions (0)
No revisions yet.