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

Background and Purpose

For those unaccustomed to Ruby, Ruby supports range literals i.e. 1..4 meaning 1, 2, 3, 4 and 1...4 meaning 1, 2, 3. This makes a Ruby for loop pretty sweet:

for i in 1..3
    doSomething(i)
end

It works for non-numeric types and that's pretty cool but out of our scope.

A similarly convenient thing exists in Python, range, with some additional features and drawbacks. range(1, 4) evaluates to [1, 2, 3]. You can also provide a step parameter via range(0, 8, 2) which evaluates to [0, 2, 4, 6]. There is no option to make the last element inclusive (as far as I know). range calculates its elements the moment it is invoked, but there is also a generator version to avoid unnecessary object creation. In combination with Python's list construction style, you can do all sorts of cool stuff like [x*x for x in range(0, 8, 2)], which is right on the border of this question's scope.

Now that ES6 has generators and the for-of statement (and most platforms support them), iteration in JavaScript is quite elegant; there is generally only one truly right (or at least best) way to write the loop for your task. However, if you are iterating over a series of numbers, ES6 offers nothing new. That's not just in comparison to ES5, but to pretty much every curly-bracket-based language before it. Python and Ruby (probably other languages I don't know, too) have proven that we can do it better and eliminate stroke-inducing code like:

while (i--) {
    sillyMistake(array[i--]);
}

for (
    var sameThing = youHaveWrittenOut;
    verbatimFor >= decades;
    thinkingAboutDetails = !anymore
) {
    neverInspectAboveForErrors();
    assumeLoopVariantIsNotModifiedInHere();
    if (modifyLoopVariant()) {
        quietlyScrewUp() || ensureLoopCondition() && causeInfiniteLoop();
    }
}

ES6 for-of, spread operator, and this generator can eliminate all manual loop counter fiddling, unreadable three-part for, and smelly loops that initialize arrays. Please explain how I have succeeded or failed to do so, including any use cases this doesn't cover.

Code for Review

const sign = Math.sign;

/**
 * Returns true if arguments are in ascending or descending order, or if all
 * three arguments are equal.  Importantly returns false if only two arguments
 * are equal.
 */
function ord(a, b, c) {
  return sign(a - b) == sign(b - c);
}

/**
 * Three overloads:
 *
 *  range(start, end, step)
 *    yields start, start + step, start + step*2, ..., start + step*n
 *    where start + step*(n + 1) would equal or pass end.
 *    if the sign of step would cause the output to be infinite, e.g.
 *    range(0, 2, -1), range(1, 2, 0), nothing is produced.
 *
 *  range(start, end)
 *    as above, with step implicitly +1 if end > start or -1 if start > end
 *
 *  range(end)
 *    as above, with start implicitly 0
 *
 *  In all cases, end is excluded from the output.
 *  In all other cases, start is included as the first element
 *  For details of how generators and iterators work, see the ES6 standard
 */
function* range(start, end, step) {
  if (end === undefined) {
    [start, end, step] = [0, start, sign(start)];
  } else if (step === undefined) {
    step = sign(end - start) || 1;
  }
  if (!ord(start, start + step, end)) return;
  var i = start;
  do {
    yield i;
    i += step;
  } while (ord(start, i, end));
}

/*
  Use Cases.  Feedback on output method is not necessary.  Insights on use
  cases themselves are welcome.
*/
(function shortFor(ol) {

  for (var i of range(0, 16, 2)) ol.append($("<li>").text(i.toString()));

})($("#short-for .vals"));

(function spreadMap(ol) {

  for (var el of [...range(4)].map(x => x * x)) {
    ol.append($("<li>").text(el.toString()));
  }

})($("#spread-map .vals"));

(function correspondingIndex(ol) {

  var a = [1, 2, 3],
    b = [4, 5, 6];
  for (var i of range(a.length)) a[i] += b[i];

  for (var el of a) ol.append($("<li>").text(el.toString()));

})($("#corresponding-index .vals"));
<!-- 
  jQuery is included here to conveniently emit results of test cases;
  the code to be reviewed has no dependencies.
  Feedback on HTML is not necessary.
-->
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>

<body>
  <div class="use-case" id="short-for">
    <h1>Use Case: Less verbose simple for loop</h1>
    <code>for (var i of range(0, 16, 2))</code>
    <p>Values of i:
      <ol class="vals"></ol>
  </div>
  <div class="use-case" id="spread-map">
    <h1>Use Case: Initialize an array without push()</h1>
    <code>[...range(4)].map(x => x*x)</code>
    <p>Array contents:
      <ol class="vals"></ol>
  </div>
  <div class="use-case" id="corresponding-index">
    <h1>Use Case: Add corresponding elements of two arrays</h1>
    <code>
      var a = [1, 2, 3], b = [4, 5, 6];
      for (var i of range(a.length)) a[i] += b[i];
    </code>
    <p>Contents of array a:
      <ol class="vals"></ol>
  </div>

</body>

share|improve this question

However, if you are iterating over a series of numbers, ES6 offers nothing new.

True, they never put in a range function. But it's pretty easy to build one yourself using the newer APIs. I'm taking about fill and map. My logic may be off here (not really sure about range's intricacies) but the following should give you a general idea of how it looks like.

function range(start, end, step) {
  var _end = end || start;
  var _start = end ? start : 0;
  var _step = step || 1;
  return Array((_end - _start) / _step).fill(0).map((v, i) => _start + (i * _step));
}

document.write(JSON.stringify(range(10)));
// [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]

document.write(JSON.stringify(range(1, 11)));
// [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]

document.write(JSON.stringify(range(0, 30, 5)));
// [0, 5, 10, 15, 20, 25]

Above, we just create an empty array with the length needed then we use map to fill in the values. What this returns is a regular array of numbers, nothing fancy. However, it makes it possible to use array methods like forEach, map etc which are more or less equivalent to what you are looking for.

Array.apply(null, Array(5));

fill is relatively new. The older way to do this was mentioned in this answer. It used the Array.apply to force the array to not have empty slots so array methods like map can traverse through it.

Now, to your test cases. When we use the approach mentioned above, it's relatively easy to chain array methods one after the other.

for (var i of range(0, 16, 2))  ol.append($("<li>").text(i.toString()));

// becomes

range(0, 16, 2).forEach(i => $("<li>").text('' + i).appendTo(ol));

////////////////////////////////////////////////////////////////////////////

for (var el of [...range(4)].map(x => x * x)) {
  ol.append($("<li>").text(el.toString()));
}

// becomes

range(4).map(x => x * x)
        .forEach(i => $('li').text('' + i).appendTo(ol));

////////////////////////////////////////////////////////////////////////////

var a = [1, 2, 3],
    b = [4, 5, 6];
for (var i of range(a.length)) a[i] += b[i];
for (var el of a) ol.append($("<li>").text(el.toString()));

// becomes

range(a.length).map(i => a[i] + b[i])
               .forEach(i => $('li').text('' + i).appendTo(ol));

As for the other code...

for (var i of range(a.length)) a[i] += b[i];

It would be best if you returned a new array containing the results of the added a and b. It's not wrong (hey, it's programming, anything goes), but it will save you from future headaches by not worrying about who mutated a. For instance, you need to do another operation with a somewhere else but oops! Some operation already mangled the values!

ol.append($("<li>").text(i.toString()));

// to

$("<li>").text('' + i).appendTo(ol);

Instead of using containerReference.append(constructedElement) in jQuery, reverse the order into constructedElement.appendTo(containerReference). It will look more streamlined, avoiding the cluttery parens.

Also, a quick way to turn anything into a string is to prepend a blank string. Since we started a string, JS will start to coerce the succeeding values to a string.

eliminate all manual loop counter fiddling, unreadable three-part for, and smelly loops that initialize arrays.

Consider using array methods like forEach, map etc. when running through arrays instead of loops. Most of them provide 3 arguments: current value, current index, and a reference to the original array being traversed. The index is handy for counting and you avoid managing it yourself. The reference to the array traversed is handy especially when you chain together operations and don't want intermediate references to the arrays generated.

range(4).map(x => x * x).forEach((v, i, a) => {
  // a = [0, 1, 4, 9], the original array generated by map
});
share|improve this answer
    
Agree, this is precisely how I was doing it before spread operator and generators. I'm sure that 90% of the times I use my range will look like [...range(number)]. This is what your range returns, right? Still, I think I prefer the ability to have the range as a separate entity, then opt-into having an array. Two reasons. One is that a day-one JS coder can look at [...range(4)] and know exactly what he or she has there, whereas range returning an array, they would have to look it up (as a simple thing, likely defined in-project). – sqykly Jan 3 at 15:07
    
The other is the project this (just the ord and range functions) came out of, which involved animation and image manipulation. If I am going to iterate over two regions of two pixel arrays, I am not going to want to make another array of similar size just as a list of indexes. Animating, the iterator range isn't scoped in a snippet-sized loop - it's in the state of an object and it only needs to produce one number per frame. That's just not what arrays are for. – sqykly Jan 3 at 15:32
    
@sqykly Yup, my range is similar to what you appear to be doing with an array and spread. It's better to add in the purpose of the code in your question so people can orient their answer better. I just went for the simple, general-purpose approach, not a performance-oriented one. – Joseph the Dreamer Jan 3 at 19:08
    
Side question: is there any reason the array prototype iteration methods (given their explicitly intended generality) were not pulled into a common class (say, Iterable) from which Array and all the other collections could extend? I keep needing to write stuff like [...someSet] just to get access to reduce, when it ought to have a reduce of its own. And you have a point, I will try to come up with a way to better explain the breadth of circumstances I expect this to be useful in. – sqykly Jan 3 at 19:33
    
@sqykly For your question, I honestly don't know. You might want to ask people over at the JS channel at StackOverflow chat. – Joseph the Dreamer Jan 3 at 21:12

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.