4
\$\begingroup\$

I implemented an enumerate() method for C++ containers similar to Python's enumerate to iterate through a range with an index and the actual value.

I have the following questions:

  • Does the usage of the Const template parameter make sense for this scenario?
  • Do I need additional iterator methods depending on the type of iterator, e.g. operator+(int) or operator--?
  • Is the construction of the std:pair on dereferencing the iterator too much?
  • Should the operator!= method also compare the current index?
  • Is there something else important missing here?

Thanks!

Usage:

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

auto main() -> int {
    const std::vector<std::string> names{ "Adam", "Ben", "Chris" };
    for (const auto&& [i, name] : enumerate(names))
        std::cout << i << ") " << name << '\n';
}

Implementation:

#include <cstddef>
#include <iterator>
#include <utility>

template <class Iter, class T, bool Const = false>
class enumerate_iterator {
public:
    using pointer = typename std::conditional_t<Const, const T* const, T*>;
    using reference = typename std::conditional_t<Const, const T&, T&>;

    enumerate_iterator(Iter iterator, std::size_t index);

    auto operator!=(const enumerate_iterator& other) -> bool;
    void operator++();
    auto operator*() const -> std::pair<std::size_t, reference>;

private:
    Iter iterator;
    std::size_t index;
};

template <class Container, bool Const = false>
struct enumerate_range {
    using value_type = typename Container::value_type;
    using container_iterator =
        typename std::conditional_t<Const, typename Container::const_iterator,
                                    typename Container::iterator>;
    using iterator = enumerate_iterator<container_iterator, value_type, Const>;
    using reference = typename std::conditional_t<Const, const Container&, Container&>;

    explicit enumerate_range(reference container);

    enumerate_range() = delete;

    auto begin() -> iterator;
    auto end() -> iterator;

private:
    reference container;
};

template <class Container>
auto enumerate(const Container& container) -> enumerate_range<Container, true> {
    return enumerate_range<Container, true>(container);
}

template <class Container>
auto enumerate(Container& container) -> enumerate_range<Container, false> {
    return enumerate_range<Container, false>(container);
}

template <class Container, bool Const>
enumerate_range<Container, Const>::enumerate_range(reference container) : container(container) {}

template <class Container, bool Const>
auto enumerate_range<Container, Const>::begin() -> iterator {
    return iterator(container.begin(), 0);
}

template <class Container, bool Const>
auto enumerate_range<Container, Const>::end() -> iterator {
    return iterator(container.end(), std::distance(container.begin(), container.end()));
}

template <class Iter, class T, bool Const>
enumerate_iterator<Iter, T, Const>::enumerate_iterator(Iter iterator, std::size_t index)
    : iterator(iterator), index(index) {}

template <class Iter, class T, bool Const>
auto enumerate_iterator<Iter, T, Const>::operator!=(const enumerate_iterator& other) -> bool {
    return iterator != other.iterator;
}

template <class Iter, class T, bool Const>
void enumerate_iterator<Iter, T, Const>::operator++() {
    iterator++;
    index++;
}

template <class Iter, class T, bool Const>
auto enumerate_iterator<Iter, T, Const>::operator*() const -> std::pair<std::size_t, reference> {
    return std::pair<std::size_t, reference>{index, *iterator};
}
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

Answers to your questions

Does the usage of the Const template parameter make sense for this scenario?

It makes sense to have some way to get the right type for container_iterator and the other type aliases, but you don't need to do it with a template parameter. For example, you could instead just deduce whether the template parameter Container is const or not, and store it in a static constexpr bool:

template <class Container>
struct enumerate_range {
    static constexpr bool Const = std::is_const_v<Container>;
    ...
};

It might also be possible to avoid having a Const altogether, and derive the correct container_iterator, reference and pointer types using correct application of decltype and other type trait utilities, but I think apart from removing Const as a template parameter, the current code is fine and quite readable.

Do I need additional iterator methods depending on the type of iterator, e.g. operator+(int) or operator--?

You can make it more generic by supporting as many methods as you would expect from containers. Not just the iterators, but you might also consider adding member functions like size() and operator[] to enumerate_range. Whether you need it depends on what you want to use this class for.

Is the construction of the std:pair on dereferencing the iterator too much?

I'm not sure what you mean by that? You need to construct it for sure. However, you can avoid repeating the type name, like so:

template <class Container>
auto enumerate_range<Container>::iterator::operator*() const -> std::pair<std::size_t, reference> {
    return {index, *it};
}

Alternatively, don't specify a (trailing) return type, but let it be automatically deduced:

template <class Container>
auto enumerate_range<Container>::iterator::operator*() const {
    return std::pair<std::size_t, reference>{index, *it};
}

Not having to repeat yourself is nice, it avoids mistakes and makes refactoring code later easier. Apart from that there is no performance benefit.

Should the operator!= method also compare the current index?

The value of the container_iterator is already unique, you don't need to compare the index as well.

Is there something else important missing here?

If you want to make it as generic as possible, you should implement all of the interfaces of STL containers where possible. With C++20, the ranges library has some helper classes that make this easier, like std::ranges::view_interface.

Move enumerate_iterator into enumerate_range

The class enumerate_iterator is just an implementation detail of enumerate_range, you can move the former inside the latter, like so:

template <class Container>
struct enumerate_range {
    static constexpr bool Const = std::is_const_v<Container>;
    using value_type = typename Container::value_type;
    using container_iterator =
        typename std::conditional_t<Const,
                                    typename Container::const_iterator,
                                    typename Container::iterator>;
    using reference =
        typename std::conditional_t<Const, const Container&, Container&>;

    class iterator {
    public:
        using pointer =
            typename std::conditional_t<Const, const value_type* const, value_type*>;
        using reference =
            typename std::conditional_t<Const, const value_type&, value_type&>;
    
        iterator(container_iterator it, std::size_t index);
        ...
        private:
        container_iterator it;
        std::size_t index;
    };
    ...
};

This completely avoids having to make the iterator a template. You now have to define the member functions of enumerate_range::iterator like so:

template <class Container>
enumerate_range<Container>::iterator::iterator(container_iterator it, std::size_t index)
    : it(it), index(index) {}

Use of trailing return types

I would avoid writing trailing return types if the return type can be deduced automatically anyway. Apart from that, if you really want to use trailing return types everywhere because you prefer that style, then be consistent; you can also use it for functions that return void:

auto operator++() -> void;

Although that brings me to the following point:

Use the correct interface for iterators

While the return value of operator++() on an iterator might not be used often, it is good practice to follow the interface of regular container iterators to ensure there are no surprises. The return type of operator++() should be a reference to the iterator itself:

template <class Container>
auto enumerate_range<Container>::iterator::operator++() {
    it++;
    index++;
    return *this;
}

And for operator++(int) it should return a copy of the iterator before it was incremented.

Possible performance issue with end()

You try to ensure that end() returns an iterator with index set to the size of the container. However, consider that one should never dereference the end iterator to begin with, and that you don't need to compare index to anything in the comparison functions, you could just set it to zero. This has a benefit, because std::distance(container.begin(), container.end()) might be slow for containers that don't support random access (like std::list, std::map and so on). Some containers might have a fast std::size() even if they are not random access, so you could perhaps use that instead, but I would just avoid it.

\$\endgroup\$
-1
\$\begingroup\$

Your code can likely be replaced by Boost's existing template implementation of the same thing, adapters::indexed. Read their code for an idea of anything you might want to change--it's typically quite efficient and portable, although sometimes at the expense of readability.

https://stackoverflow.com/questions/53542092/pythons-enumerate-for-c https://www.boost.org/doc/libs/1_68_0/libs/range/doc/html/range/reference/adaptors/reference/indexed.html

\$\endgroup\$
4
  • \$\begingroup\$ This is a review of OP's code. The review is "replace it by a standard library". If the OP was aware of this function (I don't think they were) I would review their code instead, but I don't think they're deliberately re-implementing this. \$\endgroup\$ Sep 17, 2021 at 22:23
  • \$\begingroup\$ @G.Sliepen Please do not tell users to post answers as comments. The answer contains one insightful observation and so completely qualifies for an answer. \$\endgroup\$
    – Peilonrayz
    Sep 17, 2021 at 22:41
  • 1
    \$\begingroup\$ @Peilonrayz Ok. Unfortunately it seems I cannot retract my downvote unless this post is edited. I still think this is not a great answer, even if the assumption that OP didn't know about other implementations is true, it just raises a lot of other questions: is Boost really more efficient and more portable, and if so what specifically is wrong in the above code? So many posts on CodeReview could be answered with "look over there, someone else already did it". \$\endgroup\$
    – G. Sliepen
    Sep 18, 2021 at 8:28
  • \$\begingroup\$ @G.Sliepen You've pointed out all fair reasons to downvote the answer. \$\endgroup\$
    – Peilonrayz
    Sep 18, 2021 at 13:22

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.