current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite
2

In my current project I have objects of the following structure:

CanvasBundle - Object

  • Some elements...
  • gridCanvas
  • axesCanvas
  • functions[] (contains unknown number of functionCanvases)
  • overlayCanvas

I'd like to write an iterator which would iterative over all the canvases (grid/axes/func/func1/func2/.../overlay).

I've currently implemented it like this:

function makeCanvasBundleIterator(canvasBundle) {
  var nextIndex = 0;
  var functionIndex = 0;

  return {
    next: function() {
      var value;

      switch (nextIndex) {
      case 0:
        value = canvasBundle.grid;
        break;
      case 1:
        value = canvasBundle.axes;
        break;
      case 2:
        value = canvasBundle.functions[functionIndex];
        break;
      case 3:
        value = canvasBundle.overlay;
        break;
      default:
        break;
      }

      if (nextIndex != 2 || functionIndex == canvasBundle.functions.length -1) { 
        nextIndex++; 
      } else { 
        functionIndex++ 
      }

      return nextIndex <= 4 ? 
        {value: value, done: false} :
        {done: true};
    }
  };
}

This allows me to do the following: 

var it = makeCanvasBundleIterator(canvasBundle);

var next;
while (!(next = it.next()).done) {
  console.log(next.value);
}

Coming from C, this code does not look clean at all. Since I'm a real JS beginner, I'd like a review with suggestions/advice how I can clean it up. Maybe there even is a completely different/better approach.

share|improve this question
    
This is indeed a rather odd iterator. Could you tell us more about what you're trying to accomplish, so that we may advise you properly? – 200_success♦ yesterday
1  
@200_success Each canvas element (like gridCanvas, axesCanvas, etc.) corresponds to an HTML-canvas element. These HTML-canvases all lay on top of each other (overlapping) and together they create a full picture. All these canvases are necessary to draw multiple layers, etc. The complete picture is stored in one of the CanvasBundles shown above. When I need to redraw a whole picture, I need to iterate over all the contained canvases (to redraw every single one of them). The code above does the job, but I just think the code looks kind of dirty. – LastSecondsToLive yesterday
up vote 2 down vote accepted

If you're implementing the iteration protocol - use a generator: 

function* makeCanvasBundleIterator(canvasBundle) { // note the *
   yield canvasBundle.grid;
   yield canvasBundle.axes;
   yield* canvasBundle.functions; // note the * for inner yield
   yield canvasBundle.overlay;
}

And that's your entire function. Generators give us a declarative way to deal with the iteration protocol.

Now, you can also consume the result better - this is regardless if you want to use the generator or your own solution:

for(const value of makeCanvasBundleIterator(...)) {
    // access value here
}

If you'd like to take a more OOP approach and signal that a CanvasBundle is an iterable, you can implement the iterable protocol with Symbol.iterator :

CanvasBundle.prototype[Symbol.iterator] = function*() {
   yield canvasBundle.grid;
   yield canvasBundle.axes;
   yield* canvasBundle.functions; // note the * for inner yield
   yield canvasBundle.overlay;
}

Which would allow you to iterate the canvas bundle itself:

for(const item of canvasBundle) { 
   // iterate item
}
Array.from(canvasBundle); // get iteration result as array
[...canvasBundle]; // also works and converts to an array.

And use it anywhere you'd use a regular iterator.

share|improve this answer
    
This seems really cool as well, thank you! – LastSecondsToLive yesterday
    
@LastSecondsToLive this is what generators do - their whole purpose is to declaratively create iterations. Note that unlike the other answer this code does not create an extra copy of the array at any point, no closures are involved and is efficient. Engines will probably (might) not even allocate a {done, value} since under the hood they know that it'll be consumed with a for... of loop. – Benjamin Gruenbaum yesterday
    
Great, awesome explanation :) Can I also add it to CanvasBundle.prototype? I'll probably go with it. – LastSecondsToLive yesterday
    
Yes, I'll edit it into my answer. – Benjamin Gruenbaum yesterday
    
Thanks! This is great. – LastSecondsToLive yesterday
up vote 4 down vote

The switch is awkward, and the nextIndex != 2 special case is cumbersome. It would be a bit simpler if you regularized everything:

function makeCanvasBundleIterator(canvasBundle) {
  var i = 0, j = 0;
  var items = [
    [canvasBundle.grid],
    [canvasBundle.axes],
    canvasBundle.functions,
    [canvasBundle.overlay],
  };
  return {
    next: function() {
      if (i >= items.length) {
        return {done: true};
      }
      if (j >= items[i].length) {
        i++;
        j = 0;
      }
      return {done: false, value: items[i][j++]};
  };
}

But that could be simplified further by linearizing the list up front using Array.concat():

function makeCanvasBundleIterator(canvasBundle) {
  var i = 0;
  var items = [].concat(
    canvasBundle.grid,
    canvasBundle.axes,
    canvasBundle.functions,
    canvasBundle.overlay
  );
  return {
    next: function() {
      return (i >= items.length) ? {done: true}
                                 : {done: false, value: items[i++]};
  };
}

For that matter, why bother making an iterator at all? Just return the linearized array. The caller can iterate through it conventionally using a for loop or Array.forEach().

Note that the behaviour of these alternate solutions is different from the original if the canvasBundle is modified during iteration. (You wouldn't want to do that, I hope?)

share|improve this answer
    
Exactly what I was looking for. Thank you once again - you did already review some of my C projects :) I don't want to modify the content of the canvasBundle. I'll probably add a function getCanvases to the canvasBundle-prototype, which returns the linearized array. – LastSecondsToLive yesterday

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.