Displaying rainfall totals

Question

My program works, but I'm really new at programming and am just curious if anything I wrote could be written better or more efficiently. It's suppose to display rainfall totals for the previous 12 months and say if they are + or - the average of those months. My teacher this semester gives no feedback or criticism on projects so I feel this is my best chance to learn something. It basically has two parts, calculate the previous 12 months and gets the average and then displays the months in a table format. I was wondering if I should bother trying to make each part it's own separate function or if it's fine written all in the main function.

int main() {
bool endProgram = 0, validInput = 0;
int currentMonth = 0, printedMonth = 0;
float avgRainfall = 0.0, tempTotal = 0.0;
string monthArray[MONTHS] =
    { "January","February", "March","April","May","June","July","August","September","October","November","December" };
float rainPerMonth[2][12] = {
    { 2.88, 3.89, 3.34, 3.35, 2.81, 1.18, 0.87, 1.84, 1.35, 6.14, 3.02, 3.22 }, //2016
    { 2.49, 0.28, 3.01, 3.09, 2.46, 4.01, 2.27, 2.24, 3.75, 2.06, 2.15, 4.41 }  //2015
};

while (!endProgram) {
    cout.setf(ios::fixed);
    cout.setf(ios::showpoint);
    cout.precision(2);
    bool validInput = 0;
    //User Inputs
    while (!validInput) {
        cout << "Please enter the number of the current month: ";
        cin >> currentMonth;
        if (currentMonth >= 1 && currentMonth <= 12) {
            validInput = true;
            //Calc the avg rainfall for the range we've selected
            for (int i = 0; i < MONTHS; i++) {
                if (currentMonth == 0) { //if we've hit jan
                    int ml = MONTHS - i;
                    currentMonth = MONTHS; //calculate how many months we have left
                    for (int j = 0; ml > 0; j++) {
                        tempTotal += rainPerMonth[0][currentMonth - 1];
                        currentMonth -= 1;
                        ml -= 1;
                        i++;
                    }break;
                }
                tempTotal += rainPerMonth[1][currentMonth - 1];
                currentMonth -= 1;
            }
            currentMonth == 0 ? currentMonth = 12 : currentMonth = currentMonth;
            avgRainfall = tempTotal / MONTHS;
            cout << "Average rainfall for the last 12 months was: " << avgRainfall << std::endl;
            cout << endl;
            //Print the column headers
            cout << "|" << setw(10) << ("Month   ") <<
                "|" << setw(10) << ("2016   ") <<
                "|" << setw(10) << ("2015   ") <<
                "|" << setw(22) << ("     Difference      ") <<
                "|" << endl;
            cout << setfill('=') << setw(56) << " " << setfill(' ') << endl;
            //Print the monthly rainfall for 2016
            for (int i = 0; i < MONTHS; i++) {
                float diff = 0.0;
                string diff2 = "";
                //calc + or - average
                diff = rainPerMonth[0][currentMonth - 1] - avgRainfall;
                diff2 = (diff < 0) ? "Below average" : "Above average";
                //Prints display
                cout << "|" << setw(10) << left << monthArray[currentMonth - 1] <<
                    "|   " << setw(7) << left << rainPerMonth[0][currentMonth - 1] <<
                    "|   " << setw(7) << left << rainPerMonth[1][currentMonth - 1] <<
                    "|   " << setw(0) << abs(diff) << " " << diff2 <<
                    " |" << std::endl;
                currentMonth -= 1;
                //If we need to go to 2015
                if (currentMonth == 0) {
                    int ml = MONTHS - (i + 1); //months left to include
                    currentMonth = MONTHS;
                    //Prints the monthly rainfall for 2015
                    for (int j = 0; ml > 0; j++) {
                        //calc for + or - average
                        diff = rainPerMonth[1][currentMonth - 1] - avgRainfall;
                        diff2 = (diff < 0) ? "Below average" : "Above average";
                        //Prints display
                        cout << "|" << setw(10) << left << monthArray[currentMonth - 1] <<
                            "| " << setw(9) << left << "NO DATA" <<
                            "|   " << setw(7) << left << rainPerMonth[1][currentMonth - 1] <<
                            "|   " << setw(0) << abs(diff) << " " << diff2 <<
                            " |" << std::endl;
                        currentMonth -= 1;
                        ml -= 1;
                        i++;
                    }
                }
            }
        } else {
            cout << "Please enter a number between 1 and 12" << endl;
        }
        endProgram = RestartProgram();
    }
}
return 0;

Answer 1

I'm not sure how much C++ you've learned, so I'll stick to what I'm sure you know.

In this code:

while (!endProgram) {
    cout.setf(ios::fixed);
    cout.setf(ios::showpoint);
    cout.precision(2);

You have a very non-specific loop, but a highly-detailed set of operations involving your output stream.

I'd suggest that you find cases like this - where you are really drilling down to handle some very fiddly detail - and abstract them into a function whose name reflects what you are doing. Like this:

while (!endProgram) {
    configureOutputStream();

Similarly, you have lots of places where you are fiddling with the width of your output fields. That might be another sign that you should have a function instead of multiple lines of calls to setw(some-constant). (In fact, perhaps you could compute the width value?)

Next, it took me a moment to realize what you were doing here:

        for (int i = 0; i < MONTHS; i++) {
            if (currentMonth == 0) { //if we've hit jan
                int ml = MONTHS - i;
                currentMonth = MONTHS; //calculate how many months we have left
                for (int j = 0; ml > 0; j++) {
                    tempTotal += rainPerMonth[0][currentMonth - 1];
                    currentMonth -= 1;
                    ml -= 1;
                    i++;
                }break;
            }
            tempTotal += rainPerMonth[1][currentMonth - 1];
            currentMonth -= 1;
        }

you have a for loop nested inside another for loop, specifically to handle the case where your month counter rolls over. Why don't you just reset your counter to the top, and handle the code in your main for loop? I promise, it's possible. Of course, you might not be aware of this, but the C++ % operator (modulo, or remainder-after-division) is perfect for use in this scenario.

Next, this code doesn't do what you think it does:

currentMonth == 0 ? currentMonth = 12 : currentMonth = currentMonth;

You're trying to get cute, and use a ternary expression instead of an if statement. You need to study the relationship between statements and expressions in C, and learn about assignment expressions. You want to fix this either by saying this:

currentMonth = currentMonth ? currentMonth : 12;

Or this:

if (currentMonth == 0) currentMonth = 12;

I vote for the latter.

(Of course, see my comment about the % operator above, and you won't have that line of code at all.)

Answer 2

This looks like a really interesting problem to solve! Here are some ways you could improve it.

Include All The Code

You have left out some of the code. In particular, there's no code for the RestartProgram() function. You haven't declared MONTHS anywhere. None of your #includes are shown. It's difficult to do a fair code review when we can't see all of the code.

Indentation and Style

Most of your indentation is good, particularly when you have long lines that you've manually broken. However, I'd expect the entire body of a function to by indented relative to the function declaration. So I'd expect it to look like this:

int main() {
    bool endProgram = 0 // ... etc.

You should also use a little more whitespace in your code. It's somewhat difficult to read because there's only a single blank line in the entire program.

You also seem to be missing a closing }. This would not compile for me.

It's best to avoid declaring multiple variables on a single line. It makes it harder to debug issues with incorrect initializations and doesn't save you anything beyond typing the typename again.

Functions

You asked:

was wondering if I should bother trying to make each part it's own separate function or if it's fine written all in the main function.

Yes, you should definitely try to make each part it's own separate function. This reduces the cognitive load of someone reading the code (including you 6 months from now). If I'm debugging a problem with it, and I see there are a few functions named things like getCurrentMonth(), processData(), displayResults(), then I know where to start looking if there's problem with the way the data is displayed, vs. if there's problem reading the user's input. It may also help you to organize the code, and makes it possible to swap out one piece for another if you figure out a better way to do something.

Avoid "Magic Numbers"

There are several places in your code where there are arbitrary values. For example, you use the value 12 quite frequently. You call setw() with values like 10, 9, 7, 56, and 2. Why those values? What do they represent? I would declare named constants for these values. Something like this:

const int kNumMonths = 12;
const int kRainfallOutputWidth = 2;
const int kMonthOutputWidth = 10;
// ... etc.

It looks like you may have a macro that #defines the MONTH as 12, but you only use it sporadically. For example, you use it in defining monthArray, but not in defining randPerMonth.

Naming

Some of your variable names, like currentMonth, and rainPerMonth are quite good. Others could use some work. You have a float named diff. What is it the difference off? Then you have a std::string named diff2. It's not a difference. A better name might be something like differenceLabel, or differenceDisplay.

When you find yourself doing something like this:

int ml = MONTHS - (i + 1); //months left to include

it's an indication that you've named your variable poorly. You should just name it monthsLeft.

Error Handling

I'm really impressed that you validate the user's input and display an appropriate message if it's not valid. That's something that most beginners skip. Nice work!