current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 3 down vote favorite

So I have written a custom iterator for std::map that allows access to just the "values" of the map and I was wondering if it is the "correct" way to implement std::iterator (acts identically to std::iterator).

Here is the implementation I have written:

template<typename K, typename V>
class MapValueIterator : public std::iterator<std::forward_iterator_tag, V> {
public:
    MapValueIterator();

    MapValueIterator(typename std::map<K, V>::iterator iterator) : iterator(iterator) { }

    MapValueIterator(const MapValueIterator<K, V> &iterator) : iterator(iterator.iterator) { }

    MapValueIterator<K, V> &operator=(const MapValueIterator<K, V> &iterator) {
        this->iterator = iterator.iterator;
    }

    bool operator==(const MapValueIterator<K, V> &iterator) const {
        return this->iterator == iterator.iterator;
    }

    bool operator!=(const MapValueIterator<K, V> &iterator) const {
        return this->iterator != iterator.iterator;
    }

    V &operator*() const {
        return (iterator->second);
    }

    V *operator->() const {
        return &(iterator->second);
    }

    MapValueIterator<K, V> &operator++() {
        ++iterator;

        return *this;
    }

    MapValueIterator<K, V> operator++(int) {
        return (*this)++;
    }

private:
    typename std::map<K, V>::iterator iterator;
};

And an example of how it can be used:

std::map<int, int> values;

values.insert(std::make_pair(0, 1));
values.insert(std::make_pair(1, 2));
values.insert(std::make_pair(2, 5));

MapValueIterator<int, int> begin(values.begin());
MapValueIterator<int, int> end(values.end());

for (auto iterator = begin; iterator != end; ++iterator) {
    printf("%d\n", *iterator);
}
share|improve this question
    
Nice, but you could also template on the map type to be able to use it with an unordered_map or a multimap. You could arguably use a template-template parameter in this case. – glampert Mar 7 at 4:12
    
@glampert, I have looked into doing this, however it seems unordered_map and map have differing numbers of template parameters and all of the parameters are required when defining the template (i.e. template<template<class, class, class, class> class M, typename K, typename V> only works for map while template<template<class, class, class, class, class> class M, typename K, typename V> only works for unordered_map (note the extra class in the template). – Jack Wilsdon Mar 7 at 12:19
1  
As it turns out, I can use template<template<class...> M, typename K, typename V> which works perfectly! – Jack Wilsdon Mar 7 at 13:33
    
You don't need template/template parameters. You can just use the template M. Then the constructor parameter is M::iterator and the value type can be retrieved from M::mapped_type. – Loki Astari Mar 7 at 20:56
up vote 5 down vote accepted

It looks about perfect.

You got the post increment wrong though:

MapValueIterator<K, V> operator++(int) {
    return (*this)++;
}

I believe this goes into an infinite recursion.

Thus this is normally implemented as:

MapValueIterator<K, V> operator++(int) {
    MapValueIterator<K, V>  result(*this);  // get a copy for return
                                            // so this can be used
                                            // unaltered in the expression

    // Now implement the current object.
    ++(*this);

    // Returned the saved copy.
    return result;
}

The only difference is that the std::map<K,V>::iterator type actually implements the Bidirectional iterator concept. The question is why is your iterator only implement the Forward iterator concept.

share|improve this answer
up vote 2 down vote

I have one other addition to what Loki has already pointed out.

The C++ committee has voted to deprecate std::iterator as of C++17. The current recommendation is apparently to write those typedefs yourself.

template<typename K, typename V>
class MapValueIterator {
public:
    typedef V value_type;
    typedef ptrdiff_t difference_type;
    typedef V *pointer;
    typedef V &reference;
    typedef std::ForwardIteratorTag iterator_category;

// ...

Feel free to read the proposal to deprecate it, if you care about why they're doing it (and such).

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.