2
\$\begingroup\$

I have this view class for constant time rotations of a std::vector:

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

template<typename T>
class vector_view {

public:
    vector_view(std::vector<T>& vec) : vec{&vec}, offset{0} {}
    const T& operator[](int index) const {
        return (*vec)[(index + offset) % vec->size()];
    }

    T& operator[](int index) {
        return (*vec)[(index + offset) % vec->size()];
    }

    void rotate(int rotation_length) {
        offset = (offset + vec->size() - rotation_length) % vec->size();
    }

    friend std::ostream& operator<<(std::ostream& os, vector_view<T>& view) {
        os << "[";
        std::string separator = "";

        for (int index = 0; index < view.vec->size(); ++index)
        {
            os << separator << view[index];
            separator = ", ";
        }

        return os << "]";
    }

private:
    std::vector<T>* vec;
    int offset;
};

int main() {
    std::vector<char> v{'a', 'b', 'c', 'd', 'e'};
    vector_view<char> view{v};

    for (int i = 0; i < v.size(); ++i) {
        std::cout << view << std::endl;
        view.rotate(1);
    }

    std::cout << std::endl;

    for (int i = 0; i < v.size(); ++i) {
        std::cout << view << std::endl;
        view.rotate(-1);
    }
}

The demo output is:

[a, b, c, d, e]
[e, a, b, c, d]
[d, e, a, b, c]
[c, d, e, a, b]
[b, c, d, e, a]

[a, b, c, d, e]
[b, c, d, e, a]
[c, d, e, a, b]
[d, e, a, b, c]
[e, a, b, c, d]

Since I am still in the process of learning modern C++, please tell me anything I could improve here; such as:

  • Naming,
  • coding conventions,
  • API,
  • ...
\$\endgroup\$
2
\$\begingroup\$

I'm assuming that you want your vector elements to be modifiable through this view, as vec is non-const.

You store a pointer to the underlying vector, but never test it isn't null. I think it would be better to have it as a reference member.

If you want your view to be as usable as the standard vector, you'll want to provide iterators, and begin() and end() to obtain them. You may also want implement front(), back() and at(). Methods such as size(), reserve, capacity etc. can simply forward to the contained vector.


Like std::vector, you should override operator[] on const:

const T& operator[](int index) const {
    return (*vec)[(index + offset) % vec->size()];
}

T& operator[](int index) {
    return (*vec)[(index + offset) % vec->size()];
}

Once we've implemented size() or begin()/end(), the streaming operator << uses only the public interface, so it no longer needs to be friend.

Its loop compares (signed) int against (unsigned) size_t (g++ warns about this with -Wall). Prefer to avoid implicit promotions, so declare i as size_type to match size(). If you're pedantic, that should be vector_view<T>::size_type, which you'll forward from std::vector<T>. My code below avoids that entirely, by using iterators.


Beware the gotcha when T is bool. The only thing preventing your code working on vector<bool> is:

T& operator[](int index);

If you return a std::vector<T>::reference instead, that will do the right thing there. (std::vector<bool>::reference is a proxy object, not a bool&).

My modified class:

#include <iterator>
#include <vector>

template<typename T>
class vector_view {
    using vector_type = std::vector<T>;

public:
    using value_type = typename vector_type::value_type;
    using allocator_type = typename vector_type::allocator_type;
    using size_type = typename vector_type::size_type;
    using difference_type = typename vector_type::difference_type;
    using reference = typename vector_type::reference;
    using const_reference = typename vector_type::const_reference;
    using pointer = typename vector_type::pointer;
    using const_pointer = typename vector_type::const_pointer;

    // Iterators: I've implemented little more than the minimum to
    // make the test program work.  You could expand this, and also
    // provide a specialization of std::iterator_traits<> for each
    // one.
    struct iterator {
        vector_view& v;
        size_type i;
        iterator& operator++() { ++i; return *this;}
        iterator& operator-- () { --i; return *this;}
        reference operator*() const { return v[i]; }
        bool operator!=(const iterator other) const
            { return &v != &other.v || i != other.i;  }
    };

    struct const_iterator {
        const vector_view& v;
        size_type i;
        const_iterator& operator++() { ++i; return *this;}
        const_iterator& operator-- () { --i; return *this;}
        const_reference operator*() const { return v[i]; }
        bool operator!=(const const_iterator other) const
            { return &v != &other.v || i != other.i;  }
    };

    using reverse_iterator = std::reverse_iterator<iterator>;
    using const_reverse_iterator = std::reverse_iterator<const_iterator>;

    explicit vector_view(vector_type& vec, int offset = 0)
        : vec{vec},
          offset{offset}
    {}

    const reference operator[](size_type index) const {
        return vec[(index + offset) % vec.size()];
    }

    reference operator[](size_type index) {
        return vec[(index + offset) % vec.size()];
    }

    void rotate(int rotation_length) {
        auto size = vec.size();
        offset = (offset + size - rotation_length) % size;
    }

    void set_rotation(int rotation_length) {
        offset = rotation_length % vec.size();
    }

    int rotation() const {
        return offset;
    }

    // Iterators
    iterator begin() {
        return iterator{*this, 0};
    }
    const_iterator begin() const {
        return const_iterator{*this, 0};
    }
    const_iterator cbegin() const {
        return begin();
    }
    iterator end() {
        return iterator{*this, size()};
    }
    const_iterator end() const {
        return const_iterator{*this, size()};
    }
    const_iterator cend() const {
        return end();
    }
    // You may wish to implement rbegin/rend and crbegin/crend

    bool empty() const { return vec.empty(); }
    size_type size() const { return vec.size(); }

    // You may also want:
    // reserve(), capacity()
    // clear(), insert(), emplace(), erase(), ..., etc.

private:
    vector_type& vec;
    int offset;
};
// Consider implementing operator==(), operator<() and other
// comparisons (as non-member functions, probably).  You could
// probably use std::mismatch() for that.
// Printing: greatly simplified using range-based `for`.
#include <iostream>
#include <string>
template<typename T>
std::ostream& operator<<(std::ostream& os, const vector_view<T>& view) {
    os << "[";

    std::string separator = "";
    for (auto element: view) {
        os << separator << element;
        separator = ", ";
    }

    return os << "]";
}
// Test program: exercises both `normal` and boolean vectors.
int main() {
    std::vector<char> v{'a', 'b', 'c', 'd', 'e'};
    vector_view<char> view{v};

    for (size_t i = 0; i < view.size(); ++i) {
        std::cout << view << std::endl;
        view.rotate(1);
    }
    std::cout << std::endl;

    for (size_t i = 0; i < view.size(); ++i) {
        std::cout << view << std::endl;
        view.rotate(-1);
    }
    std::cout << std::endl;

    std::vector<bool> bvec{{false, true, true, false, false}};
    auto bview = vector_view<bool>{bvec};

    for (size_t i = 0; i < bview.size(); ++i) {
        bview.set_rotation(i);
        std::cout << bview << std::endl;
    }
}
\$\endgroup\$
0
\$\begingroup\$

This looks pretty nice already. Here are some minor nits

  1. You can declare vec const in the constructor and add explicit so that there is no other possible constructor.

  2. I personally dislike using int for variables that are strict non negative, but this is purely opinion based.

  3. You can use range based loops in your implementation

    for (auto &elem : *vec)
    
  4. You should write a getOffset function

\$\endgroup\$
  • 1
    \$\begingroup\$ Note that 3 does not apply: I need to be able to iterate the rotation of the vector and not the vector itself. \$\endgroup\$ – coderodde Oct 13 '16 at 18:34

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.