STL template function to return a pair

Question

I'm trying to write a function that returns a pair of values from an STL container.

template <typename T>
std::pair<typename T::value_type,typename T::value_type> getMinMax(T &container) {

    auto min = *(container.begin());
    auto max = *(container.begin());

    for (auto it : container) {
        if (min > (*it) ) {
            min = (*it);
        }
        if (max < (*it) ) {
            max = (*it);
        }
    }
    return std::make_pair(min, max);
};

int main() {
    std::vector<int> c{1, 2, 3, 4, 5};
    auto p = getMinMax(c);
    std::cout << "min: " << p.first << " max: " << p.second << "\n";
}

I'm getting an error:

error: indirection requires pointer operand ('int' invalid)
        if (min > (*it) ) {

I don't know how to deal with that.

Besides that error, is there a better way to implement the desired behavior?

*Is there a better way to implement the desired behavior?*. Yes, [`std::minmax_element`](http://en.cppreference.com/w/cpp/algorithm/minmax_element) — NathanOliver, Sep 15 '17 at 19:53
@user1211030 In this code snippet for (auto it : container) { if (min > (*it) ) { min = (*it); } it is not an iterator or a pointer. It has the value type. So remove the dereferencing. — Vlad from Moscow, Sep 15 '17 at 19:54
@user1211030 In general the function has undefined behavior because the container can be empty.:) — Vlad from Moscow, Sep 15 '17 at 19:55

Jarod42 · Accepted Answer · 2017-09-15T19:57:24.810

4

for range returns element, not iterator. So your loop should be something like:

for (const auto& e : container) {
    if (min > e) {
        min = e;
    }
    if (max < e) {
        max = e;
    }
}

edited Sep 15 '17 at 19:57

answered Sep 15 '17 at 19:55

Jarod42

203,559
14
181
302

score 2 · Answer 2 · answered Sep 15 '17 at 20:03

2

template <typename T>
std::pair<typename T::value_type, typename T::value_type> getMinMax(T &container) {

    auto min = *(container.begin());
    auto max = *(container.begin());

    for (const auto& element : container) { /* ERROR WAS HERE, FOR RANGE LOOPS RETURN AN ELEMENT */
        if (min > element) {
            min = element;
        }
        if (max < element) {
            max = element;
        }
    }
    return std::make_pair(min, max);
};

Hey! This should work, you were setting min and max to a dereferenced element, which of course, isn't what we want. :)

Also, you can get undefined behavior with this, for example, if the container was empty. Perhaps you should add some checks that check for that.

answered Sep 15 '17 at 20:03

Graham Best

153
6

1

`std::pair` has to have values in it, it can't be empty. If the container is empty, returning a default `std::pair` would contain values that might confuse the caller as being valid when they really are not. Maybe return a `std::pair` instead, where the `bool` indicates success/fail (similar to `std::map::insert()`)? Otherwise, just throw an exception instead. – Remy Lebeau Sep 15 '17 at 20:14
I didn't know this. Thank you – Graham Best Sep 15 '17 at 20:15
@Remy Lebeau If the container is empty the presented function has undefined behavior.:) – Vlad from Moscow Sep 15 '17 at 20:49
@VladfromMoscow: yes, I know. Graham's said as much in his answer: "*you can get undefined behavior with this, for example, if the container was empty. **Perhaps you should add some checks that check for that***", to which I replied by providing ways in which the function could report to the caller that it isn't able to return a valid `std::pair` for an empty container. – Remy Lebeau Sep 15 '17 at 20:52

Vlad from Moscow · Answer 3 · 2017-09-15T20:54:50.660

For starters the function can have undefined behavior in case when the container is empty because there can be an attempt of dereferencing the iterator of an empty container.

In loops like this

for (auto it : container) {
    if (min > (*it) ) {
        min = (*it);
    }

there is incorrectly used dereferencing.

You could use the standard algorithm std::minmax_element. However it does not do the same as your code. It returns the first minimum element and the last maximum element. So you should rewrite the algorithm std::minmax_element such a way that ir would return the first minimum element (the iterator pointing to the first minimum element) and the first maximum element (the iterator pointing to the first maximum element).

The function can be defined for example the following way

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

template <typename T>
auto getMinMax( T &container ) 
    -> std::pair<decltype( container.begin() ), decltype( container.begin() )> 
{
    auto min = container.begin();
    auto max = container.begin();

    if ( !container.empty() )
    {
        for ( auto it = container.begin(); ++it != container.end();  )
        {
            if ( *it < *min ) min = it;
            else if ( *max < *it ) max = it;
        }
    }

    return { min, max };
}

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

    auto minmax = getMinMax( v );

    std::cout << "Minimum = " << *minmax.first 
              << " at position " << std::distance( v.begin(), minmax.first )
              << " and maximum = " << *minmax.second
              << " at position " << std::distance( v.begin(), minmax.second )
              << std::endl;

    return 0;
}

The program output is

Minimum = 1 at position 4 and maximum = 9 at position 6

STL template function to return a pair

3 Answers3