HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Several math calculation functions

Submitted by: @import:stackexchange-codereview··
0
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 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.