7
\$\begingroup\$

I am learning C++. This is a simple program I had to write for class. Is there anything I should fix?

#include <iostream>
using namespace std;


int main()
{
    const int ListSize = 5;
    int ValuesProcessed = 0;
    float ValueSum = 0;

    cout << "Please enter " << ListSize << " numbers" << endl;

    while(ValuesProcessed < ListSize){
        float Value;
        cin >> Value;
        ValueSum += Value;
        ++ValuesProcessed;
    }
    float Average = ValueSum / ValuesProcessed;
    cout << "Average: " << Average << endl;

    return 0;
}
\$\endgroup\$

3 Answers 3

Reset to default
3
\$\begingroup\$
  1. Do not use using namespace std;. See this
  2. Declare constants outside of main().
  3. Usually, the first letter of a variable name is not capitalized. But if you're going to do that, keep it consistent throughout the code (which you did).

  4. What happens if the user wants to input more than 5 numbers? I would put the calculation into a separate function. And then call it from a while loop.

    //Inside some function  
while(cin >> value){
    valueSum += value;
    ++valuesProcessed;
}
\$\endgroup\$
0
5
\$\begingroup\$

This is quite good for a homework exercise.

I would recommend a for loop instead of the while:

for (int ValuesProcessed = 0; ValuesProcessed < ListSize; ++ValuesProcessed) {

This puts all the logic of iteration together where you are less likely to overlook some detail, unlike the while loop where the initial value of ValuesProcessed, the test for termination of the loop, and setting ValuesProcessed for the next iteration are done in three different places.

I tend to follow the philosophy of declaring things as close to where they are used as possible, so I would move the declaration and initialization of ValueSum after the "Please enter" printout. But I don't think there's a strong argument for doing this. (It would merely swap the order of two lines of code, not a big deal.)

Contrary to another answer, I like your decision to declare ListSize in your main() function. (It's another example of "declaring things as close to where they are used as possible".) Although ListSize is a "constant" in the sense that you have declared it const (which I think is a good idea), it is really just the end condition of a single loop in the program, or to put it another way, the length of a list of inputs that the program is about to read.

\$\endgroup\$
2
  • \$\begingroup\$ Lower case the first letter of non types. That looks really ugly. \$\endgroup\$ Commented Oct 9, 2014 at 18:57
  • \$\begingroup\$ @LokiAstari I agree, the first letter should be lower-case except for types, as recommended in other answers. In the answer text I was trying on one hand not to give redundant advice, and on the other hand to use names from the existing code when I was trying to make points not related to naming. \$\endgroup\$ Commented Oct 9, 2014 at 22:51
4
\$\begingroup\$

Stop doing this:

using namespace std;

Its a really bad habit that is hard to break later and can cause some real problems. The reason the standard namespace is called std and not standard is so that prefix them members with std:: is not that much of a burden.

Usually user defined types start with an initial uppercase letter.
While anything that can be an object starts with a lower case letter. Its a pretty standard convention for C++.

const int ListSize = 5;
int ValuesProcessed = 0;
float ValueSum = 0;

Prefer to use "\n" in preference to std::endl. The difference is that "\n" does not force a flush. Which can be very costly. Let the system handle that for you and flush at optimal times (it does that automatically). Even with std::cin/std::cout were you think you need to flush before asking for input; you don't if you read from std::cin then std::cout is automatically flushed to make sure that the user sees the question they are trying to answer. 

cout << "Please enter " << ListSize << " numbers" << endl;

Its would be nice to give some user feedback here.

    float Value;
    cin >> Value;

Also you don't check and fix bad input. What happens if I accedently typed "fred" What do you think would happen?

Don't bother to return 0 from main(). By not returning a value you are indicating that there are no possible error conditions. Note: main() is special in that it is the only function that the compiler will implicitly plant a return 0; for you if you miss it out.

\$\endgroup\$

You must log in to answer this question.