11
\$\begingroup\$

I am working with an array-like data container through a 3rd party API. So I can not change the container itself to add iterators. There are many different array-like classes, and each have a specific data type it will store. There are two unknowns in this case, the class of the array container and the data type stored in the container. Though all of them have the same interface by getting a value by index - operator[]. So I wrote a random access iterator to wrap around these classes so I don't have to specialize or define the container class and data return types.

I would like some feedback on my code and any suggestions for improvements or errors. I have tested this and it works as expected so far.

template<typename T>
class MayaArrayRange {
protected:
    typedef typename std::remove_reference<decltype(std::declval<T>()[0])>::type item_type;

public:
    template<typename C, typename V>
    class MayaArrayIter : public std::iterator<std::random_access_iterator_tag, V, int> {
    friend class MayaArrayRange;

    protected:
        C* c;
        unsigned int i;

        MayaArrayIter(C* c) : c(c), i(0) {}
        MayaArrayIter(C* c, unsigned int i) : c(c), i(i) {}

    public:
        typedef typename std::iterator<std::random_access_iterator_tag, V, int>::pointer pointer;
        typedef typename std::iterator<std::random_access_iterator_tag, V, int>::reference reference;
        typedef typename std::iterator<std::random_access_iterator_tag, V, int>::difference_type difference_type;

        template<typename C2, typename V2>
        MayaArrayIter(const MayaArrayIter<C2, V2>& other) : c(other.c), i(other.i) {}

        template<typename C2, typename V2>
        MayaArrayIter& operator=(const MayaArrayIter<C2, V2>& other) {
            c = other.c;
            i = other.i;
            return *this;
        }

        reference operator*() const {
            return (*c)[i];
        }

        pointer operator->() const {
            return &(*c)[i];
        }

        MayaArrayIter& operator++() {
            ++i;
            return *this;
        }

        MayaArrayIter& operator--() {
            --i;
            return *this;
        }

        MayaArrayIter operator++(int) {
            return MayaArrayIter(c, i++);
        }

        MayaArrayIter operator--(int) {
            return MayaArrayIter(c, i--);
        }

        MayaArrayIter operator+(const difference_type& n) const {
            return MayaArrayIter(c, (i + n));
        }

        MayaArrayIter& operator+=(const difference_type& n) {
            i += n;
            return *this;
        }

        MayaArrayIter operator-(const difference_type& n) const {
            return MayaArrayIter(c, (i - n));
        }

        MayaArrayIter& operator-=(const difference_type& n) {
            i -= n;
            return *this;
        }

        reference operator[](const difference_type& n) const {
            return (*c)[i + n];
        }

        bool operator==(const MayaArrayIter& other) const {
            return i == other.i;
        }

        bool operator!=(const MayaArrayIter& other) const {
            return i != other.i;
        }

        bool operator<(const MayaArrayIter& other) const {
            return i < other.i;
        }

        bool operator>(const MayaArrayIter& other) const {
            return i > other.i;
        }

        bool operator<=(const MayaArrayIter& other) const {
            return i <= other.i;
        }

        bool operator>=(const MayaArrayIter& other) const {
            return i >= other.i;
        }

        difference_type operator+(const MayaArrayIter& other) const {
            return i + other.i;
        }

        difference_type operator-(const MayaArrayIter& other) const {
            return i - other.i;
        }
    };

    typedef MayaArrayIter<T, item_type> iterator;
    typedef MayaArrayIter<const T, const item_type> const_iterator;

    MayaArrayRange(T* mayaArray) : a(mayaArray) {}
    MayaArrayRange(T& mayaArray) : a(&mayaArray) {}

    iterator begin() {
        return iterator(a);
    }

    const_iterator begin() const {
        return const_iterator(a);
    }

    const_iterator cbegin() const {
        return const_iterator(a);
    }

    iterator end() {
        return iterator(a, a->length());
    }

    const_iterator end() const {
        return const_iterator(a, a->length());
    }

    const_iterator cend() const {
        return const_iterator(a, a->length());
    }

protected:
    T* a;
};
\$\endgroup\$
  • \$\begingroup\$ Great first question and very well written code! Welcome to Code Review ;) \$\endgroup\$ – glampert Dec 14 '15 at 20:10
5
\$\begingroup\$

Nice code :)

I just have a few suggestions:

  • prefer using using to typedef. A typedef makes it difficult to find the alias (especially with long template instantiations), but with using, the alias is at the beginning:

    typedef typename std::remove_reference<decltype(std::declval<T>()[0])>::type item_type;
    

    vs:

    using item_type = typename std::remove_reference<decltype(std::declval<T>()[0])>::type;
    
  • Your iterator's regular-type operators are symetrical, so define them based on each other; this will minimize the effort for maintenance later (in case you decide to add something to what it means for two iterators to be equal - for example) and show their connected nature in the code:

    bool operator==(const MayaArrayIter& other) const {
        return i == other.i; // <-- define equality here
    }
    
    bool operator!=(const MayaArrayIter& other) const {
        return !(*this == other); // <-- define this in terms of operator ==
    }
    
    bool operator<(const MayaArrayIter& other) const {
        return i < other.i; // <-- define this
    }
    
    bool operator>(const MayaArrayIter& other) const {
        return !(*this < other || *this == other); // <-- define this
                                                   // in terms of == and <
    }
    

    (same goes for the other operators).

  • Consider doing some range checking (check that your indexes (i) do not exceed the limits of the arrays, and that the iterators you compare are iterating the same pointers). If you do not want the performance penalty that comes with these checks, simply assert them.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your advice! I'm still trying to get more in the habbit of writing C++11 code, hence the typedefs :-) I'll have to get use to the "using" statement going forward. You make a good point about the comparison operators and using an assert to help catch for going beyond the range. \$\endgroup\$ – Scott Dec 15 '15 at 0:50
5
\$\begingroup\$

The code looks good, and I see nothing to suggest that it wouldn't work well - it's just that there's a lot of code that doesn't need to exist. So here's a review full of suggested deletions.

Unnecessary Typedefs

Inheriting from std::iterator already provides you with the pointer, reference, and difference_type typenames. That's the entire point of inheriting from std::iterator. You do not need to provide them yourself.

Unnecessary index

Your MayaArrayIter contains both a C* and an unsigned int. Why? You don't need both. Just take a pointer that already points to the correct index. Constructing a MayaArrayIterator(cnt, 5) would act equivalently to MayaArrayIterator(&cnt[0] + 5), so let's just do that.

Extend that out to all the member functions.

Unnecessary functions

MayaArrayIter is a simple pod, so the compiler-generated defaults for the copy constructor and copy assignment already do the right thing - you do not need to reimplement them. If you want to write them out, you should = default them.

Unnecessary Second Template Argument

Once we fix MayaArrayIter to just hold a pointer to the data, it effectively just holds a V*. So we just need the one template argument.

Unnecessary Pointer Prefer a Reference

Had to break the theme here, ugh. Anyway, if MayaArrayRange just exists to wrap a random access range - you should keep a reference to the container (and name it something more meaningful than a) rather than a pointer.

Even better would be to make the range container-agnostic so that a MayaArrayRange<int> can either refer to an int[30] or a std::array<int, 30> or a std::vector<int> or ...

\$\endgroup\$
  • \$\begingroup\$ Thank you Barry for the very well broken down suggestions. I appreciate it! I want to follow up with what you wrote. \$\endgroup\$ – Scott Dec 14 '15 at 21:36
  • \$\begingroup\$ The unnecessary index. The reason was to not assume the array was in continuous memory and safer to use the [] operator. So I didn't want to make the assumption of get in trouble later, safer to go straight through through their member functions. This was the only way I felt it was safe. Would you agree? \$\endgroup\$ – Scott Dec 14 '15 at 21:54
  • \$\begingroup\$ Additionally, I saw on another example to do the overloaded copy/assignment to support copying from a non-const iterator to a const iterator. Using the default's would that only support copy/assignment from only the same iterator type? \$\endgroup\$ – Scott Dec 14 '15 at 21:58

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.