Cyclic iterator in both directions

Question

I am trying to make a cyclic iterator that works backwards and forwards. getNext() gets the next array item, getPrevious() gets the item before the last returned one from getNext(). This seems to work, but there must be a better way. How can this be refactored?

var cyclicIterator = function (arr) {
    var index = 0;
    var length = arr.length;
    var current;
    return {
        getNext: function () {
            current = arr[index];
            index = (index + 1) % length;
            return current;
           },
        getPrevious: function () {
            index = arr.indexOf(current);
            if (index === 0) {
                index = length - 1;
            }
            else {
                index = index - 1;
            }
            current = arr[index]
            return current;
        }
    };
};

Answer 1

Is there any reason why getPrevious resorts to using indexOf? As far as I can tell, it can only cause problems.

For instance:

var array = [1, 2, 3];
var iterator = cyclicIterator(array);

iterator.getPrevious();

Since current is declared but hasn't been assigned a value, this will call arr.indexOf(undefined) and then proceed to subtract 1 from that. I doubt that's going to work out.

Alternatively, I might do this:

var array = [1, 2, 3];
var iterator = cyclicIterator(array);
iterator.getNext(); // current is now 1
array[0] = 2;       // change the first element in the array
iterator.getPrevious();

Now, getPrevious will call arr.indexOf(1) and get -1. And it will then subtract 1 from that. Again, I doubt it'll work out.

I'd also wonder why there's no getCurrent function. In fact that's what getNext does. It for some reason returns the current index, only incrementing the index afterwards. Meanwhile getPrevious decrements the index before returning anything.

You also store the length of the array when you receive it, which could lead to some weird stuff if the array is later modified.

For instance:

var array = [1, 2, 3];
var iterator = cyclicIterator(array);
iterator.getNext(); // index is now 1
array.splice(3);    // clear the array
iterator.getNext();

That last call will calculate 2 % 3 which is, well, 2. But that index doesn't exist anymore.

You could simply skip caching the length, but that could still get weird. getNext would start skipping places when "looping around", if the array's been truncated after the iterator was created.

var array = [1, 2, 3];
var iterator = cyclicIterator(array);
iterator.getNext(); // index is now 1
iterator.getNext(); // index is now 2
array.pop();        // remove one item from the array
iterator.getNext();

With no caching of the length, the last call will calculate (2 + 1) % 2 which is 1. But that'll skip index 0.

So I'd simply copy the array, to make sure there's always something to work with.

I'd do this instead:

var cyclicIterator = function (array) {
    var index = 0;
    var copy = array.slice(0);

    return {
        getCurrent: function () {
            return copy[index];
        },

        getNext: function () {
            index = ++index % copy.length;
            return this.getCurrent();
        },

        getPrevious: function () {
            if(--index < 0) {
              index += copy.length;
            }
            return this.getCurrent();
        }
    };
};

Answer 2

If you retrieve the iterator, then call getPrevious on it, it fails.

To fix this, you should set current to arr[0].

Consider returning no iterator for empty arrays too.

Answer 3

var Iterator = (function () {
    function Iterator(toIterate) {
        this.index = -1;
        this.toIterate = toIterate;
        this.length = toIterate.length;
    }
    Iterator.prototype.getNext = function () {
        this.index = (this.index + 1) % this.length;
        return this.toIterate[this.index];
    };
    Iterator.prototype.getPrevious = function () {
        if (this.index < 0) {
            this.index = this.index + this.length;
        } else if (this.index === 0) {
            this.index = this.length - 1;
        } else {
            this.index = this.index - 1;
        }
        return this.toIterate[this.index];
    };
    return Iterator;
})();

My last version is this. I have changed it in to a class. As before I am keeping track of the index, to return the next or previous item, in the index property so here there is no need for the var current current item variable and index = arr.indexOf(current).