HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Cyclic iterator in both directions

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
cyclicdirectionsiteratorboth

Problem

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;
        }
    };
};

Solution

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();
        }
    };
};

Code Snippets

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

iterator.getPrevious();
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();
var array = [1, 2, 3];
var iterator = cyclicIterator(array);
iterator.getNext(); // index is now 1
array.splice(3);    // clear the array
iterator.getNext();
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();
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();
        }
    };
};

Context

StackExchange Code Review Q#68069, answer score: 6

Revisions (0)

No revisions yet.