7
\$\begingroup\$

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 <iostream>
#include <cmath>
#include <string>
#include <algorithm>
#include <fstream>

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 < amount; ++i) {
        total += numbers[i];
    }
    // Divide the total by the amount to get the average
    avg = total / amount;
    // Return the average
    return avg;
}

double getDeviation(int amount, int numbers[], double avg){
    // Declare the variables and arrays
    double tempNum = 0;
    double total = 0;
    double deviation = 0;

    // Search for each number, subtract the mean, make it an absolute value, then store it in
    // another array
    for (int i = 0; i < amount; ++i) {
        tempNum = abs(numbers[i] - avg);
        total += tempNum;
    }

    // Divide the total by the average to get the deviation
    if (amount == 0) {
        return 0;
    }
    else {
        deviation = total / amount;
    }
    // Return the deviation
    return deviation;
}

double getCalculation(double firVar, double secVar, string typeSym){
    //Declare the total variable
    double total = 0;
    //Detect which symbol the user piked then execute the expression
    if (typeSym == "+")
        total += firVar + secVar;
    if (typeSym == "-")
        total += firVar - secVar;
    if (typeSym == "*")
        total += firVar * secVar;
    if (typeSym == "/")
        total += firVar / secVar;
    //return the total
    return total;
}

int check()
{
    //Check if cin failed
    if (cin.fail()) {
        cout << "ERROR, INPUT IS NOT VALID! \n";
        return 0;
    }

    return 0;
}

int main(){
    // Declare the variables and arrays
    int varNum = 1;
    int totVar = 0;
    int choice = 0;
    int userNums[1000] = {};
    double ch1 = 0;
    double ch2 = 0;
    double avg = 0;
    double median = 0;
    string sym = "";
    char dec = 'y';

    //Ask the user for their choice and then record it
    cout << "Which function would you like to use? \n";
    cout << "1) Average Function \n";
    cout << "2) Mean Absolute Deviation Function \n";
    cout << "3) Number Sorting Function \n";
    cout << "4) Median Function \n";
    cout << "5) All of the above \n";
    cout << "6) Calculator Function \n";
    cout << "7) Guide (use only if you have the guide file) \n";
    cout << "Your choice: ";
    cin >> choice;
    check();
    if (choice <= 5) {
        // Ask user for how many variables they want then record it
        cout << "How many variables would you like to have? (Max 999) ";
        cin >> totVar;
        check();
        // Check if totVar is to big or to small
        if (totVar >= 1000 || totVar <= 0) {
            cout << "ERROR, INPUT EITHER TOO LARGE OR TOO SMALL \n";
            return 0;
        }

        // Ask the user for each variable, then record it into the array
        for (int i = 0; i < totVar; ++i) {
            cout << "Please input variable " << varNum << ": ";
            cin >> 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 << "The Average is: " << getAverage(totVar, userNums) << endl;
        // Call the deviation function
        if (choice == 2)
            cout << "The Mean Absolute Deviation is: " << getDeviation(totVar, userNums, avg) << endl;
        // Display the contents of the array
        if (choice == 3) {
            cout << "Numbers in order from least to greatest: ";
            for (int i = 0; i < totVar; i++) {
                cout << userNums[i] << ", ";
            }
            cout << endl;
        }
        if (choice == 4)
            cout << "The Median is " << median << "\n";
        //Print all of the above
        if (choice == 5) {
            cout << "The Average is: " << getAverage(totVar, userNums) << "\n";
            cout << "The Mean Absolute Deviation is: " << getDeviation(totVar, userNums, avg) << "\n";
            cout << "The Median is " << median << "\n";
            cout << "Numbers in order from least to greatest: " << endl;
            for (int i = 0; i < totVar; i++) {
                cout << userNums[i] << ", ";
            }
        }
    }
    if (choice == 6) {
        cout << "Welcome to the calculator. You can calculate simple two variable equations using  + - * / symbols \n";
        //Ask the user for which operator they would like to use, record it then check if it is a valid symbol
        cout << "Which operator would you like to use? ";
        cin >> sym;
        if (sym == "+" || sym == "-" || sym == "*" || sym == "/") {
            //Ask user for the first variable then record it and check it
            cout << "Please enter your first variable: ";
            cin >> ch1;
            check();
            //Ask user for the second variable then record it and check it
            cout << "Please enter your second variable: ";
            cin >> ch2;
            check();
            //Print the total of the calculation
            cout << "The total is: " << getCalculation(ch1, ch2, sym) << "\n";

        } 
    }

    if (choice == 7) {
        ifstream f("guide.txt");

        if (f.is_open())
            cout << "\n" << f.rdbuf();
    }
    cout << "\nWould you like to run this program again? (y/n): ";
    cin >> dec;
    if (dec == 'y')
        return main();
    return 0;
}
\$\endgroup\$
6
\$\begingroup\$

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<int> 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_Average) || (choice > kUC_Guide)) {
        displayInputError();
    }
} while ((choice < kUC_Average) || (choice > kUC_Guide));
\$\endgroup\$
3
\$\begingroup\$

Iterators:

Probably most of the algorithms that work on containers can be implemented in terms iterators. Some might need tweaking, but most actually work with the iterators provided by, say, std::vector<>. If the code is changed to work in terms of iterators, it will be possible to change the container without changing the algorithm at all. They will also make the algorithm generic, which can work on any type as long as the iterators conform the concept required.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.