11
\$\begingroup\$

I posted this code in an answer earlier today, but thought it was worth seeing if somebody could suggest further improvements:

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

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;
}

int main() { 
    std::vector<int> inputs { 1, 2, 3, 4, 5, 10, 9, 8, 7, 6};

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

    std::cout << "\n";

    std::transform(inputs.begin(), inputs.begin()+inputs.size()/2, 
        inputs.rbegin(), 
        std::ostream_iterator<int>(std::cout, "\t"), 
        std::plus<int>());
}

Here I've placed fixed input into the vector inputs. The intent is to first produce the sums of adjacent items in the input, and second produce the sum of the first item added to the last item, second item added to the second to last, and so on.

\$\endgroup\$
1
  • \$\begingroup\$ Your std::transform() code misses the middle number on an odd-sized input. \$\endgroup\$
    – jliv902
    Jun 20 '14 at 20:12
12
\$\begingroup\$

You make the assumption of random accesses iterators.
It would be nice to be able to do this with any iterator (including input iterator)

template <class InIt, class OutIt>
OutIt adjacent_sum(InIt begin, InIt end, OutIt result)
{ 
    if (begin == end)
    {     return result;    // From @Corbin
    }

    auto  last = (*begin);  // Get the value now before incrementing the iterator.
    ++begin;     
    while (begin != end)
    {
        auto next = (*begin);
        *result   = next + last;
        last      = next;

        ++result;
        ++begin;
    }
    return result;
}

Now the following works:

adjacent_sum(std::istream_iterator<int>(std::cin), std::istream_iterator<int>(), 
             std::ostream_iterator<int>(std::cout, "\t"));
\$\endgroup\$
8
\$\begingroup\$

You're likely aware of these limitations and they're implicit pre-conditions to adjacent_sum, but just for the sake of it being CodeReview, I have two criticisms with regards to adjacent_sum, one fairly major, and the other minor.

An explicit check for empty ranges should be done in adjacent_list. If begin == end when calling adjacent_sum, ++begin will almost certainly not equal end, and that's going to result in a very nasty loop.

Also, there's no real reason to require a random access iterator, and it's very trivial to implement this on a basic forward iterator. (Although one can easily argue that it's not just worthless but actually counter-productive to focus on generality before the functionality is even needed. It's just clear that some form of generality has already been desired, so might as well consider going as far as easily possible.)

\$\endgroup\$
3
\$\begingroup\$

I would just use std::adjacent_difference from the <numeric> library. It's designed to accept custom binary ops such as plus<> and multiplies<> from the <functional> library.

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

If you are going to use your own adjacent sum, please include handling for empty/single element lists.

template <class InIt, class OutIt>
OutIt adjacent_sum(InIt begin, InIt end, OutIt result) {
    if (begin == end)    // Corbin
        return result;

    auto last = *begin;  // Loki
    for (*result = last; ++begin != end;) { // Me
        auto next = *begin;
        *++result = last + next;
        last = std::move(next);
    }

    return result;
}
\$\endgroup\$

Your Answer

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

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