Follow

Keep Up to Date with the Most Important News

By pressing the Subscribe button, you confirm that you have read and are agreeing to our Privacy Policy and Terms of Use
Contact

How to fix my implementation of multiple-vector ZigZag Iterator?

This is my implementation of multiple-vector ZigZagIterator problem:
(Full task description here:
https://ttzztt.gitbooks.io/lc/content/zigzag-iterator.html)
However, it does not work properly and I clearly try to iterate over non-existing elements.

#include <deque>
#include <iostream>
#include <iterator>
#include <type_traits>
#include <vector>

using namespace std;

class ZigZagIterator {
    int k;
    deque<pair<vector<int>::iterator, vector<int>::iterator>>
        order;   // it.begin(), it.end()

  public:
    template <typename T, typename... Args> ZigZagIterator(T t, Args... args) {
        init(t, args...);
    }

    template <typename T> void init(T t) {
        if constexpr (is_same_v<T, int>) {
            cout << "solo-init | t is int\n";
            k = t;
        } else {
            cout << "solo-init | t is NOT int\n";
            order.push_back(make_pair(t.begin(), t.end()));
        }
    }

    template <typename T, typename... Args> void init(T t, Args... args) {
        if constexpr (is_same_v<T, int>) {
            cout << "multi-init | t is int\n";
            k = t;
        } else {
            cout << "multi-init | t is NOT int\n";
            order.push_back(make_pair(t.begin(), t.end()));
        }
        init(args...);
    }

    int next_elem() {
        cout << "DEBUG | order.size() == " << order.size() << "\n";
        while (!order.empty() && order.front().first == order.front().second) {
            order.pop_front();
            cout << "DEBUG | empty element deleted!\n";
        }
        if (order.empty()) {
            cout << "Queue ended!\n";
            return -1;
        }
        auto current = order.front();
        order.pop_front();
        cout << "TEST  | " << *current.first << "\n";
        /*
        Next or it++ tyrade
            Although the expression ++c.begin() often compiles, it is not
            guaranteed to do so: c.begin() is an rvalue expression, and
            there is no LegacyInputIterator requirement that specifies that
            increment of an rvalue is guaranteed to work. In particular,
            when iterators are implemented as pointers or its operator++ is
            lvalue-ref-qualified,
            ++c.begin() does not compile, while std::next(c.begin()) does.
            TODO: next == advance (?)
        */
        if (next(current.first) != current.second) {
            order.push_back(make_pair(next(current.first), current.second));
        }
        return *current.first;
    }
};

However, testing it like this I see that I clearly did something wrong.


int
main() {
    vector<int> first = {1, 4, 7};
    vector<int> second = {2, 5, 8};
    vector<int> third = {3, 6, 9};
    ZigZagIterator zigzag(3, first, second, third);
    while (zigzag.next_elem() != -1) {
        // ...
    }
}

Output:

MEDevel.com: Open-source for Healthcare and Education

Collecting and validating open-source software for healthcare, education, enterprise, development, medical imaging, medical records, and digital pathology.

Visit Medevel

multi-init | t is int
multi-init | t is NOT int
multi-init | t is NOT int
solo-init | t is NOT int
DEBUG | order.size() == 3
TEST  | 4
DEBUG | order.size() == 3
TEST  | 0
DEBUG | order.size() == 3
TEST  | 2
DEBUG | order.size() == 3
TEST  | 1073741824
DEBUG | order.size() == 3
TEST  | 1073741824
DEBUG | order.size() == 3
TEST  | 1073741824
DEBUG | order.size() == 3
TEST  | 326502032
DEBUG | order.size() == 2
TEST  | 0
DEBUG | order.size() == 1
TEST  | 0
DEBUG | order.size() == 0
Queue ended!

The correct output is expected to be:
1, 2, 3, 4, 5, 6, 7, 8, 9

>Solution :

Here

template <typename T, typename... Args> ZigZagIterator(T t, Args... args) {
    init(t, args...);
}

And everywhere else you are making copies of the vectors that are passed. Hence the iterators you store in the queue are invalid by the time you dereference them.

Changing the constructor and the other methods to pass the vectors by reference

template <typename T, typename... Args> ZigZagIterator(T t, Args&... args) {
    init(t, args...);
}

fixes the output: https://godbolt.org/z/z6KMo4EYP.


While this fixes the immediate problem with the code, there is more to fix. For example the recursive templates are unnecessary complexity. Your code anyhow only works for std::vector<int> (thats the type of iterator you store in the queue). You could use a initializer list constructor to accept arbitrary number of vectors of int.

Add a comment

Leave a Reply

Keep Up to Date with the Most Important News

By pressing the Subscribe button, you confirm that you have read and are agreeing to our Privacy Policy and Terms of Use

Discover more from Dev solutions

Subscribe now to keep reading and get access to the full archive.

Continue reading