4
\$\begingroup\$

I had to write a program that accepted five numbers from the user. Then it will display the number of odd and even numbers, and a total sum of all numbers.

#include <iostream>
using namespace std;

int main()
{
    int a;
    int b;
    int c;
    int d;
    int e;
    int sum;
    int remainder;
    int bremainder;
    int cremainder;
    int dremainder;
    int eremainder;
    int even;
    int odd;
    int total = 0;
    int Ototal = 0;

    cout << "Please enter the 5 numbers you would like calculated: \n";
    cin >> a;
    cin >> b;
    cin >> c;
    cin >> d;
    cin >> e;

    remainder = a % 2;
    bremainder = b % 2;
    cremainder = c % 2;
    dremainder = d % 2;
    eremainder = e % 2;

    sum = (a + b + c + d + e);

    if (remainder = 0)
    {
        total+=remainder;
    }
    else {
        Ototal += remainder;
    }
    if (bremainder = 0)
    {
        total += bremainder;
    }
    else {
        Ototal += bremainder;
    }
    if (cremainder = 0)
    {
        total += cremainder;
    }
    else {
        Ototal += cremainder;
    }
    if (dremainder = 0)
    {
        total += dremainder;
    }
    else {
        Ototal += dremainder;
    }
    if (eremainder = 0)
    {
        total += eremainder;
    }
    else {
        Ototal += eremainder;
    }

    cout << total << "\n";

    cout << Ototal << "\n";

    cout << sum << "\n";
    system("PAUSE");
        return 0;
}
\$\endgroup\$
  • \$\begingroup\$ You should validate that the user actually entered numbers. \$\endgroup\$ – Michael McGriff Oct 13 '14 at 15:55
  • \$\begingroup\$ Warning: operator = in C/C++ does NOT serve as comparision! And if it did, then after satisfying e.g. 'cremainder equals zero' it doesn't make sense to add cremainder to total, because adding zero to total does not change the total value. \$\endgroup\$ – CiaPan Dec 6 '17 at 14:43
5
\$\begingroup\$
  1. Do not use system("pause"). Read this
  2. Stop using using namespace std. Read this
  3. You have an unneccessary amount of variables. You don't need to have a variable to hold the remainder for each number.
  4. Just have a function that determines whether the input number is odd or even. I think you might have to rethink the logic of your program.
//Your function should do the following:
//MAX is a const int. In your case it is 5
    for(int i=0; i<MAX; i++){
        if(numbers[i] % 2 == 0)
            evenNumbers++;
        else
            oddNumbers++;

        sum += numbers[i];
    }

Here is how I would have done it:

#include <iostream>
#include <vector>
#include <iterator>

using std::cin;
using std::cout;
using std::endl;
using std::vector;



int main(){

    int value = 0;
    int oddNumbers(0), evenNumbers(0), sum(0);


    std::istream_iterator<int> begin(cin);
    std::istream_iterator<int> end;


    //while(cin >> value)
    //  numbers.push_back(value);

    //Instead of a while loop, I use input stream iterators.
    vector<int> numbers;
    std::back_insert_iterator<std::vector<int>> iter (numbers);

    std::copy(begin,end,iter);


    //Range based for-loop (C++11)
    for(auto i : numbers){
        if(i % 2 == 0){
            evenNumbers++;
        }else {
            oddNumbers++;
        }

        sum += i;

    }

    cout << "# of odd numbers: " << oddNumbers << "\n";
    cout << "# of even numbers: " << evenNumbers << "\n";
    cout << "Sum of all numbers: " << sum << endl;



}
| improve this answer | |
\$\endgroup\$
  • 3
    \$\begingroup\$ Please always use curly braces after for/if/else etc.. Read this. \$\endgroup\$ – elmo Oct 13 '14 at 8:14
  • \$\begingroup\$ @elmo But why? The article you linked explicitly says that 'always using curly braces' blindly does not prevent errors... \$\endgroup\$ – CiaPan Dec 6 '17 at 16:27
4
\$\begingroup\$

Inspired by @EngieOPs answer, I ended up with the following:

  • I use std::copy_n instead of std::copy to read the values, because it was asking for a fixed sized input.
  • I templated everything to enable counting of other types than int.
  • I created a functor is_even for better readability.
  • I use std::count_if and accumulate from the STL to calculate the end results.
  • Instead of counting the odd numbers, I just use the difference between the number of inputs and the number of even variables.

I think the usage of std::count_if and std::accumulate is much more readable than a for loop. Whether a template makes sense would depend on the actual usage.

#include <iostream>
#include <vector>
#include <iterator>
#include <algorithm>
#include <numeric>

struct is_even
{
    template <class T>
    bool operator() (T value)
    {
        return (value % 2) == 0;
    }
};

template<class T>
void processInput(std::size_t count)
{
    std::istream_iterator<T> begin(std::cin);

    std::vector<T> numbers;
    std::back_insert_iterator<std::vector<T>> iter(numbers);
    std::copy_n(begin, count, iter);

    auto evenNumbers = std::count_if(numbers.begin(), numbers.end(), is_even());
    auto oddNumbers = numbers.size() - evenNumbers;
    auto sum = std::accumulate(numbers.begin(), numbers.end(), T());

    std::cout << "# of odd numbers: " << oddNumbers << "\n";
    std::cout << "# of even numbers: " << evenNumbers << "\n";
    std::cout << "Sum of all numbers: " << sum << std::endl;
}

int main(void)
{
    processInput<int>(5);

    return 0;
}
| improve this answer | |
\$\endgroup\$

Your Answer

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