current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 7 down vote favorite

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;
}
share|improve this question
    
You should validate that the user actually entered numbers. –  Michael McGriff 3 mins ago

2 Answers 2

up vote 7 down vote accepted
  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;



}
share|improve this answer
2  
Please always use curly braces after for/if/else etc.. Read this. –  elmo 7 hours ago
up vote 2 down vote

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

  • I use copy_n instead of copy to read the values, because it was ask 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 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 count_if and accumulate is much more readable than a for loop. If a template makes sens, depends 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;
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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