1

I know just a little about programming but I'm pretty new to C++. I just started to study it one week ago and I'm posting a question about the code I wrote since I can't solve few small problems.

The program I wrote asks how many elements you'd like to analyze and it returns the sum of the elements, how many even and odd numbers you inserted, the sum of the odd and even numbers only and it should returns the maximum (and minimum) value.

This is the code:

#include <iostream>
using namespace std;

int elements, sum = 0, x, arr[10], even = 0, odd = 0, evenSum = 0, oddSum = 0,
        mx; //Variable declaration

int main() {

    cout << "Type how many elements you would like to sum and analyze" << endl; //Input of the numbers of elements to analyze
    cin >> elements;

    if (elements > 10)
        cout << "Too many elements" << endl; //If the elements are more than 10, the program quit
    else {
        for (x = 1; x <= elements; x++) {
            cout << "Type the element number " << x << endl; //Input of elements to assign to the array
            cin >> arr[x];
        }
        for (x = 1; x <= elements; x++) { //Sum of the elements of the array
            sum += arr[x];
        }
        mx = arr[0];
        for (x = 1; x <= elements; x++) { //Find the maximum value of the array
            if (arr[0] >= mx) {
                mx = arr[0];
            }
        }
        for (int x = 1; x <= elements; x++) { //Count and sum of the even elements
            if (arr[x] % 2 == 0) {
                even++;
                evenSum += arr[x];
            }
            if (arr[x] % 2 != 0) { //Count and sum of the odd elements
                odd++;
                oddSum += arr[x];
            }
        }
        cout << "The sum of the elements of the array is: " << sum << endl; //Outputs
        cout << "The max value of the array is: " << mx << endl;
        cout << "Even numbers are: " << even << endl;
        cout << "Odd numbers are: " << odd << endl;
        cout << "The sum of even numbers is: " << evenSum << endl;
        cout << "The sum of odd numbers is: " << oddSum << endl;
    }
    return 0;
}

My questions are:

  1. Do I have to repeat the "for" loop 4 times or, since the conditions of the loops are the same, I can write it only one time followed by the code to execute?
  2. To calculate the maximum value, I used as a variable name "max" but the editor doesn't compile. I changed the name from "max" to "mx" and it compiles. Is "max" a reserved word for C++? What is wrong?
  3. Why the program always gives "0" as the maximum value of the array? I can't understand what is wrong in the algorithm and I can get the maximum value of the listed elements.

Thank you very much for the support. Matteo

matteo-g
  • 213
  • 2
  • 9
  • 1. No you don't have to. You can do all of your operations in the body of one loop – litelite Aug 10 '17 at 13:36
  • At first you should note that array indices start at zero. – user0042 Aug 10 '17 at 13:36
  • You *do* remember that array indexes are zero-based? I.e. an array of 10 elements have indexes from `0` to `9` (inclusive). – Some programmer dude Aug 10 '17 at 13:36
  • 2
    You shouldn't even need an array for this. You should be able to do all these calculations while getting the input. – NathanOliver Aug 10 '17 at 13:36
  • 2. you can't use max because it's defined as a function in std and you are using `using namespace std` so they name clash. Also , it's considered [bad practice](https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice) to use `using namespace std` – litelite Aug 10 '17 at 13:37
  • 3. `if (arr[0] >= mx)`  you are always comparing the max with the first element. And after in the body of the if you have `mx = arr[0];` which means max can only ever be the first element – litelite Aug 10 '17 at 13:38

4 Answers4

1
  1. You should be able to calculate everything inside a single for-loop, yes.

  2. This might be because you have the line

    using namespace std;

    at the top of your file. When you write max the compiler assumes that you mean the function std::max: http://en.cppreference.com/w/cpp/algorithm/max

    You could try removing the using namespace std; and instead insert std:: where needed, e.g. std::cin, std::cout and std::endl.

  3. There are two problems. First of all you have typos here:

    if (arr[0] >= mx) {, mx = arr[0];

    Replace 0 with x. The second problem occurs if you only enter negative numbers. You don't have an explicit initialization for mx, so it will be initialized to zero. If you only enter negative numbers then your program will incorrectly say that zero is greatest number. You should probably initialize mx to the lowest possible number for its type, e.g.: std::numeric_limit<int>::min().

simon
  • 2,042
  • 2
  • 20
  • 31
1

Code cleansing 101:

There are several things to make your code much cleaner.

1) You don't need that many for loops and since you don't need that many for loops you can get rid of your array.

2) Your variables should be local not global.

3) Several of your variables were not initialized to 0 thus you can't do odd++; (for example)

Look through my changes, understand them, read the comments, and ask questions if you have them :)

#include <iostream>
using namespace std; //probably don't want to use this here

int main() 
{
    // keep these variables local instead of global    
    int elements, sum , even, odd, evenSum, oddSum, mx;
    sum = even = odd = evenSum = oddSum = mx = 0;  //set them all to 0

    cout << "Type how many elements you would like to sum and analyze" << endl; //Input of the numbers of elements to analyze
    cin >> elements;

    if (elements > 10)
        cout << "Too many elements" << endl; //If the elements are more than 10, the program quit
    else 
    {
        // An array is based on the 0 index not the 1 index.
        // so the first value in array as at x = 0 and the second is at x = 1 and so on
        int currentNum;
        for (int x = 0; x < elements; x++) //int x = 0 and x < elements
        {
            cout << "Type the element number " << x << endl; //Input of elements to assign to the array
            cin >> currentNum;
            sum += currentNum;
            if (currentNum >= mx) 
                mx = currentNum;
            if (currentNum % 2 == 0) 
            {
                even++;
                evenSum += currentNum;
            }
            if (currentNum % 2 != 0) { //Count and sum of the odd elements
                odd++;
                oddSum += currentNum;
            }
        }

        cout << "The sum of the elements of the array is: " << sum << endl; //Outputs
        cout << "The max value of the array is: " << mx << endl;
        cout << "Even numbers are: " << even << endl;
        cout << "Odd numbers are: " << odd << endl;
        cout << "The sum of even numbers is: " << evenSum << endl;
        cout << "The sum of odd numbers is: " << oddSum << endl;
    }
    return 0;
}
  • May I ask why I should declare my variables inside the main() function and not globally? What is the difference? – matteo-g Aug 10 '17 at 19:50
  • Its an extremely bad practice to create non static global variables and even static ones should only be used if needed. Read this for an example [Why global variables are evil](http://www.learncpp.com/cpp-tutorial/4-2a-why-global-variables-are-evil/) – Error - Syntactical Remorse Aug 10 '17 at 19:54
  • Thank you very much! Another question: why sum = even = odd = evenSum = oddSum = mx = 0 and the other variables are not initialized? And why sum = even = odd = evenSum = oddSum = mx = 0 don't have "int" at the beginning? Thanks again! – matteo-g Aug 10 '17 at 20:40
  • They are declared on the line `int elements, ...` on one line and initialized to `0` in the next line. That is what all the `=` are for. If it worked for you make sure to mark it as an answer :) – Error - Syntactical Remorse Aug 10 '17 at 20:53
0

First, if you have index based-0, you don't read arr[0] in first loop

for (x = 1(error, it should be 0); x <(only '<') elements; x++) {
        cout << "Type the element number " << x << endl; //Input of elements to assign to the array
        cin >> arr[x];
    }

Now, you can do all in two loops.

#include <iostream>
using namespace std;

int elements, sum = 0, x, arr[10], even = 0, odd = 0, evenSum = 0, oddSum = 
0,
    mx; //Variable declaration

int main() {

cout << "Type how many elements you would like to sum and analyze" << endl; //Input of the numbers of elements to analyze
cin >> elements;

if (elements > 10)
    cout << "Too many elements" << endl; //If the elements are more than 10, the program quit
else {
    for (x = 0; x < elements; x++) {
        cout << "Type the element number " << x << endl; //Input of elements to assign to the array
        cin >> arr[x];
    }
    mx = arr[0];
    for (x = 0; x < elements; x++) { //Find the maximum value of the array
        sum += arr[x];
        if (arr[x] >= mx) {
            mx = arr[x];
        }
        if (arr[x] % 2 == 0) {
            even++;
            evenSum += arr[x];
        }
        if (arr[x] % 2 != 0) { //Count and sum of the odd elements
            odd++;
            oddSum += arr[x];
        }
    }

    cout << "The sum of the elements of the array is: " << sum << endl; //Outputs
    cout << "The max value of the array is: " << mx << endl;
    cout << "Even numbers are: " << even << endl;
    cout << "Odd numbers are: " << odd << endl;
    cout << "The sum of even numbers is: " << evenSum << endl;
    cout << "The sum of odd numbers is: " << oddSum << endl;
}
return 0;

}

Alto you can do all in one loop

#include <iostream>
using namespace std;

int elements, sum = 0, x, arr[10], even = 0, odd = 0, evenSum = 0, oddSum = 
0,
    mx; //Variable declaration

int main() {

cout << "Type how many elements you would like to sum and analyze" << endl; //Input of the numbers of elements to analyze
cin >> elements;

if (elements > 10)
    cout << "Too many elements" << endl; //If the elements are more than 10, the program quit
else {
    mx = arr[0];
    for (x = 0; x < elements; x++) {
        cout << "Type the element number " << x << endl; //Input of elements to assign to the array
        cin >> arr[x];

        sum += arr[x];
        if (arr[x] >= mx) {//error here, you need compare arr[x], not arr[0]
            mx = arr[x];
        }
        if (arr[x] % 2 == 0) {
            even++;
            evenSum += arr[x];
        }
        if (arr[x] % 2 != 0) { //Count and sum of the odd elements
            odd++;
            oddSum += arr[x];
        }
    }

    cout << "The sum of the elements of the array is: " << sum << endl; //Outputs
    cout << "The max value of the array is: " << mx << endl;
    cout << "Even numbers are: " << even << endl;
    cout << "Odd numbers are: " << odd << endl;
    cout << "The sum of even numbers is: " << evenSum << endl;
    cout << "The sum of odd numbers is: " << oddSum << endl;
}
return 0;

}

0

Good point about the array indexing! I know an array is 0-indexed but even starting from 1 the program works fine! I type 5 as a number of elements to analyze and a I can insert, for instance, 1 2 3 4 5, having the correct outputs:

  • Sum: 15
  • Odd elements: 3
  • Even elements: 2

I can't understand why!

matteo-g
  • 213
  • 2
  • 9