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.

I'm trying to figure out a way to do the exercise 3.20 in the C++ primer book

I have to create a program that reads input into a vector then use that data to create 2 outputs:

  • one is the sum of each adjacent number
  • one is the sum of the first and last number moving inwards (as in the 2nd and 2nd last, 3rd and 3rd last etc)

I've created the following code, but it feels off somehow. Can I simplify/clean up this stuff a bit?

#include <vector>
#include <string>
#include <iostream>
#include <cctype>

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

int main()
{
    vector<int> nv;
    int n;

    while (cin >> n)
    {
        nv.push_back(n);
    }

    int cnt = 0;
    int output = 0;
    int oldnum = 0;

    //output sum of adjacent
    for (int cn : nv)
    {
        output += cn; 
        output += oldnum;
        ++cnt;

        if (cnt == 2)
        {
            cout << output << " ";
            cnt = 1;
            output = 0;
            oldnum = cn;
        }

    }
    cout << endl;

    //output sum of first and last
    int trigger = 1;
    for (decltype(nv.size()) i = 0; i < (nv.size()/2); ++i)
    {
        cout << (nv[trigger - 1]) + (nv[nv.size() - (1 * trigger)]) << " ";
        ++trigger;
    }
    return 0;
}
share|improve this question

2 Answers 2

Although there's room for argument that it might be an abuse, one way you could do the adjacent sums part of this would be to use std::adjacent_difference, but pass it a function that adds instead of subtracts:

std::adjacent_difference(inputs.begin(), inputs.end(), 
    std::ostream_iterator<int>(std::cout, "\t"),
    std::plus<int>());

Note, however, that the first result you get from this will be just the first item, not added to anything. Each subsequent value in the result will be the sum of the current and previous items.

You might consider, instead, implementing an adjacent_sum that produces the result you want a little more directly:

template <class InIt, class OutIt>
OutIt adjacent_sum(InIt begin, InIt end, OutIt result) { 
     ++begin;     
     while (begin != end) {
          *result = *begin + *(begin-1);
          ++result;
          ++begin;
     }
     return result;
}

// ...

adjacent_sum(inputs.begin(), inputs.end(), 
    std::ostream_iterator<int>(std::cout, " "));

For the second part, I'd probably use std::transform, supplying forward iterators for the first range, and a reverse iterator for the beginning of the second range:

std::transform(inputs.begin(), inputs.begin()+inputs.size()/2, 
    inputs.rbegin(), 
    std::ostream_iterator<int>(std::cout, " "), 
    std::plus<int>());
share|improve this answer

In order to print the sum of consecutive pairs, you sure could make things easier :

  • instead of having cnt take the values [0], then [1, 2], [1, 2], [1, 2], etc (square brackets represents one loop), cnt could take the values [0], [1], [2], etc. Then, you just need to check if cnt > 0 (or cnt != 0 which can be written cnt)
  • in a similar way, you don't need to save output from one loop to another : the only thing you need to keep track of in the previous number oldnum and its value shoud be updated at every iteration including the first one.
  • once you've done this, you can get rid of the output variable alltogether.

That piece of code now becomes :

//output sum of adjacent
int cnt = 0;
int oldnum = 0;
for (int cn : nv)
{
    if (cnt)
    {
        cout << oldnum + cn << " ";
    }
    oldnum = cn;
    ++cnt;
}
cout << endl;

Now, even though it doesn't really matter here, it can be a good idea to iterate over your containers by using (const) references to avoid copies.

Also, I am not quite convinced by the variable name. I'd find num better than cn, prev_num better than oldnum and idx better than cnt.


For your loop of extreme sums : - it is quite easy to check that trigger == i+1 : it holds at the beginning of the loop and it is maintained at each iteration. Thus, we don't need trigger. - I have doubts about the usefulness of 1 * (some_stuff). - you also happen to have useless parenthesis.

Once this is changed, your code becomes :

//output sum of first and last
for (decltype(nv.size()) i = 0; i < (nv.size()/2); ++i)
{
    cout << nv[i] + nv[nv.size() - i - 1] << " ";
}

Now, something I am not quite sure about is how you want to handle the "middle" element (if any) of your input. At the moment, you ignore it which is fair enough depending on your requirements. It would also make sense to add it to itself or to keep it on its own so that the sum of the original list is also the sum of the new list.

A slightly different solution for this would be to handle two different indices (or two different iterators) and to stop when they cross.


Disclaimer : I have never really used C++11

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.