5
\$\begingroup\$

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;
        }
    };
};
\$\endgroup\$
  • \$\begingroup\$ What about your current code makes you think it needs improvement? To be honest, I don't see any glaring problems. \$\endgroup\$ – Greg Burghardt Oct 27 '14 at 14:13
  • \$\begingroup\$ @GregBurghardt there's still things to point out. \$\endgroup\$ – Pimgd Oct 27 '14 at 14:23
  • 3
    \$\begingroup\$ What you can and cannot do after receiving answers \$\endgroup\$ – Brythan Oct 27 '14 at 14:47
  • \$\begingroup\$ Please leave the original code intact, now that answers have been posted. If you'd like further review, post a new follow-up question. \$\endgroup\$ – Jamal Oct 27 '14 at 14:52
5
\$\begingroup\$

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();
        }
    };
};
\$\endgroup\$
  • \$\begingroup\$ var a = [1,2,3,4]; var d = cyclicIterator(a); in your code d.getNext() returns 2. \$\endgroup\$ – django Oct 27 '14 at 15:02
  • \$\begingroup\$ @django Sure, but getCurrent will return 1. Question is what you consider "next" and "previous" when you haven't actually done anything yet. If you want, you can replace it with your increment-after solution. \$\endgroup\$ – Flambino Oct 27 '14 at 15:08
  • \$\begingroup\$ so in your code i will use first getNext() and getPrevious() to move through items and use getCurrent() to get the current item. \$\endgroup\$ – django Oct 27 '14 at 15:12
  • \$\begingroup\$ @django Yes, if you want. I'm not saying my code is the only way to do it, though. \$\endgroup\$ – Flambino Oct 27 '14 at 15:14
1
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$
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).

\$\endgroup\$

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.