Adding a zip method to goog.array

[nnaze, 2010-06-13 05:32:11]

This also needs a unit test.  Please edit array_test.html and add a function
called testZip(), like the others.  You can run the test by brining it up in
your browser.  There need to be test cases for the following:

* normal expected results  -- zip([1, 2], [3, 4]) -> [[1, 3], [2, 4]])
* passing no arrays
* passing arrays of different sizes

http://codereview.appspot.com/1668041/diff/16001/17002
File closure/goog/array/array.js (right):

http://codereview.appspot.com/1668041/diff/16001/17002#newcode1189
closure/goog/array/array.js:1189: 
two newlines between top-level defnitions.

http://codereview.appspot.com/1668041/diff/16001/17002#newcode1196
closure/goog/array/array.js:1196: * array provided;
comment should fill out to 80 characters.

Description needs to be more precise.  Along the lines of:

Creates an new array for which the element at position i is an array of the ith
element of the provided arrays.  The returned array will only be as long as the
shortest array provided; additional values are ignored.  For example, the result
of zipping [1, 2] and [3, 4, 5] is [[1,3], [2, 4]].  

This is similar to the zip() function in Python.  See {@link
http://docs.python.org/library/functions.html#map}

http://codereview.appspot.com/1668041/diff/16001/17002#newcode1199
closure/goog/array/array.js:1199: or more arrays that are to be combined.
type is goog.array.ArrayLike

"Arrays to be combined." for the description (one should be able to pass in no
arrays).

http://codereview.appspot.com/1668041/diff/16001/17002#newcode1200
closure/goog/array/array.js:1200: * @return {!Array} A new array of arrays
created from provided arrays.
type is !Array.<Array>

Description A zipped array of the given array.

http://codereview.appspot.com/1668041/diff/16001/17002#newcode1222
closure/goog/array/array.js:1222: return res;
the whole body can be simpler and easier to read if it uses the functional array
methods provided in this file. 

goog.array.zip = function(var_args) {
  var minLength = Math.min.apply(null, goog.array.map(arguments, function(arr) {
    return arr.length;
  });

  var result = [];
  for (var i = 0; i < minLength; i++) {
    result.push(goog.array.map(arguments, function(arr) {
      return arr[i];
    }));
  }
  return result;
};

[bjorn.tipling, 2010-06-13 15:17:32]

I also added the requested test cases.

http://codereview.appspot.com/1668041/diff/16001/17002
File closure/goog/array/array.js (right):

http://codereview.appspot.com/1668041/diff/16001/17002#newcode1189
closure/goog/array/array.js:1189: 
On 2010/06/13 05:32:12, nnaze wrote:
> two newlines between top-level defnitions.

Done.

http://codereview.appspot.com/1668041/diff/16001/17002#newcode1196
closure/goog/array/array.js:1196: * array provided;
On 2010/06/13 05:32:12, nnaze wrote:
> comment should fill out to 80 characters.
> 
> Description needs to be more precise.  Along the lines of:
> 
> Creates an new array for which the element at position i is an array of the
ith
> element of the provided arrays.  The returned array will only be as long as
the
> shortest array provided; additional values are ignored.  For example, the
result
> of zipping [1, 2] and [3, 4, 5] is [[1,3], [2, 4]].  
> 
> This is similar to the zip() function in Python.  See {@link
> http://docs.python.org/library/functions.html#map%7D

Done.

http://codereview.appspot.com/1668041/diff/16001/17002#newcode1196
closure/goog/array/array.js:1196: * array provided;
On 2010/06/13 05:32:12, nnaze wrote:
> comment should fill out to 80 characters.
> 
> Description needs to be more precise.  Along the lines of:
> 
> Creates an new array for which the element at position i is an array of the
ith
> element of the provided arrays.  The returned array will only be as long as
the
> shortest array provided; additional values are ignored.  For example, the
result
> of zipping [1, 2] and [3, 4, 5] is [[1,3], [2, 4]].  
> 
> This is similar to the zip() function in Python.  See {@link
> http://docs.python.org/library/functions.html#map%7D

Done.

http://codereview.appspot.com/1668041/diff/16001/17002#newcode1199
closure/goog/array/array.js:1199: or more arrays that are to be combined.
See below.
On 2010/06/13 05:32:12, nnaze wrote:
> type is goog.array.ArrayLike
> 
> "Arrays to be combined." for the description (one should be able to pass in no
> arrays).

http://codereview.appspot.com/1668041/diff/16001/17002#newcode1200
closure/goog/array/array.js:1200: * @return {!Array} A new array of arrays
created from provided arrays.
On 2010/06/13 05:32:12, nnaze wrote:
> type is !Array.<Array>
> 
> Description A zipped array of the given array.

Done.

http://codereview.appspot.com/1668041/diff/16001/17002#newcode1222
closure/goog/array/array.js:1222: return res;
Are you sure? There are a couple of points why I believe my original body is
preferable:

1. It follows the convention throughout the arrays.js file. If you look through
this file you will see that instead of using the functional array methods,
for(;;) is used just about everywhere. For example in flatten on line 1150, in
repeat on line 1134, the same in bucket, equals, stableSort, extend, etc. In
fact map is not used at all anywhere in the file, and forEach is only used in
reduce.
2. This new suggested body uses an unnecessary extra loop plus a call to apply
and min which presumably loops through the same array all over again, albeit not
in JavaScript.

If readability is the main concern with my patch I will be happy to provide more
comments and clearer variable names.

Oh and as for the use of call for push, I was looking at 1184 where apply is
used in a similar vein, but that is not the normal convention so I will change
that to simply push.
On 2010/06/13 05:32:12, nnaze wrote:
> the whole body can be simpler and easier to read if it uses the functional
array
> methods provided in this file. 
> 
> goog.array.zip = function(var_args) {
>   var minLength = Math.min.apply(null, goog.array.map(arguments, function(arr)
{
>     return arr.length;
>   });
> 
>   var result = [];
>   for (var i = 0; i < minLength; i++) {
>     result.push(goog.array.map(arguments, function(arr) {
>       return arr[i];
>     }));
>   }
>   return result;
> };

[bjorn.tipling, 2010-06-13 15:21:08]

http://codereview.appspot.com/1668041/diff/16001/17002
File closure/goog/array/array.js (right):

http://codereview.appspot.com/1668041/diff/16001/17002#newcode1222
closure/goog/array/array.js:1222: return res;
I also wanted to mention that the patch I provided uses an assert, which seems
to follow conventions in this file, is that not preferable? Also because of this
assert, arrays are required and not ArrayLike, although I could change it to
ArrayLike, yet I believe that may break because I may be wrong but I do not
think that some array like objects such as a NodeList have a length property.
On 2010/06/13 05:32:12, nnaze wrote:
> the whole body can be simpler and easier to read if it uses the functional
array
> methods provided in this file. 
> 
> goog.array.zip = function(var_args) {
>   var minLength = Math.min.apply(null, goog.array.map(arguments, function(arr)
{
>     return arr.length;
>   });
> 
>   var result = [];
>   for (var i = 0; i < minLength; i++) {
>     result.push(goog.array.map(arguments, function(arr) {
>       return arr[i];
>     }));
>   }
>   return result;
> };

[nnaze, 2010-06-13 19:12:17]

http://codereview.appspot.com/1668041/diff/16001/17002
File closure/goog/array/array.js (right):

http://codereview.appspot.com/1668041/diff/16001/17002#newcode1222
closure/goog/array/array.js:1222: return res;
On 2010/06/13 15:21:09, bjorn.tipling wrote:
> I also wanted to mention that the patch I provided uses an assert, which seems
> to follow conventions in this file, is that not preferable? Also because of
this
> assert, arrays are required and not ArrayLike, although I could change it to
> ArrayLike, yet I believe that may break because I may be wrong but I do not
> think that some array like objects such as a NodeList have a length property.

Your function should take ArrayLike and operate on anything with a length
property.  NodeLists have length properties.  

https://developer.mozilla.org/en/DOM/NodeList

http://codereview.appspot.com/1668041/diff/16001/17002#newcode1222
closure/goog/array/array.js:1222: return res;
On 2010/06/13 15:17:32, bjorn.tipling wrote:
> Are you sure? There are a couple of points why I believe my original body is
> preferable:

At present, your function body is hard to read and can be simplified.

> 1. It follows the convention throughout the arrays.js file. If you look
through
> this file you will see that instead of using the functional array methods,
> for(;;) is used just about everywhere. For example in flatten on line 1150, in
> repeat on line 1134, the same in bucket, equals, stableSort, extend, etc. In
> fact map is not used at all anywhere in the file, and forEach is only used in
> reduce.

It's not a big issue, but if it was really of concern, the map could be replaced
with a for in both cases.  There are a number of ways of implementing this.  My
primary concern is that it's simple and makes sense to somebody else reading it.
 Here's a simplification of what you wrote:

goog.array.zip = function(var_args) {
  if (!arguments.length) {
    return [];
  }

  var result = [];
  for (var i = 0; true; i++) {
    var value = [];
    for (var j = 0; j < arguments.length; j++) {
      var arr = arguments[j];

      // If i is larger than the array length, this is the shortest array.
      goog.asserts.assertNumber(arr.length);
      if (i >= arr.length) {
	return result;
      }

      value.push(arr[i]);
    }
    result.push(value);
  }
};

> 2. This new suggested body uses an unnecessary extra loop plus a call to apply
> and min which presumably loops through the same array all over again, albeit
not
> in JavaScript.

The first loop to find the min length is done to precalculate the minimum length
rather than having to do length checks in the middle of the zipping operation. 
To be changing the length of the for loop (as you are by modifying the value of
"l" -- a bad variable name -- at present) is very unclear.


> Oh and as for the use of call for push, I was looking at 1184 where apply is
> used in a similar vein, but that is not the normal convention so I will change
> that to simply push.

You know that "t_arr" is an array at that point and has a push() method. 
Regardless, "t_arr' is a bad variable name and not camelCase per convention.

> On 2010/06/13 05:32:12, nnaze wrote:
> > the whole body can be simpler and easier to read if it uses the functional
> array
> > methods provided in this file. 
> > 
> > goog.array.zip = function(var_args) {
> >   var minLength = Math.min.apply(null, goog.array.map(arguments,
function(arr)
> {
> >     return arr.length;
> >   });
> > 
> >   var result = [];
> >   for (var i = 0; i < minLength; i++) {
> >     result.push(goog.array.map(arguments, function(arr) {
> >       return arr[i];
> >     }));
> >   }
> >   return result;
> > };
> 
>

[bjorn.tipling, 2010-06-13 21:53:47]

Thank you for the code review. I think I have addressed all the points
addressed. Please let me know if there is anything else.

http://codereview.appspot.com/1668041/diff/16001/17002
File closure/goog/array/array.js (right):

http://codereview.appspot.com/1668041/diff/16001/17002#newcode1222
closure/goog/array/array.js:1222: return res;
On 2010/06/13 19:12:17, nnaze wrote:
> On 2010/06/13 15:17:32, bjorn.tipling wrote:
> > Are you sure? There are a couple of points why I believe my original body is
> > preferable:
> 
> At present, your function body is hard to read and can be simplified.
> 
> > 1. It follows the convention throughout the arrays.js file. If you look
> through
> > this file you will see that instead of using the functional array methods,
> > for(;;) is used just about everywhere. For example in flatten on line 1150,
in
> > repeat on line 1134, the same in bucket, equals, stableSort, extend, etc. In
> > fact map is not used at all anywhere in the file, and forEach is only used
in
> > reduce.
> 
> It's not a big issue, but if it was really of concern, the map could be
replaced
> with a for in both cases.  There are a number of ways of implementing this. 
My
> primary concern is that it's simple and makes sense to somebody else reading
it.
>  Here's a simplification of what you wrote:
> 
> goog.array.zip = function(var_args) {
>   if (!arguments.length) {
>     return [];
>   }
> 
>   var result = [];
>   for (var i = 0; true; i++) {
>     var value = [];
>     for (var j = 0; j < arguments.length; j++) {
>       var arr = arguments[j];
> 
>       // If i is larger than the array length, this is the shortest array.
>       goog.asserts.assertNumber(arr.length);
>       if (i >= arr.length) {
> 	return result;
>       }
> 
>       value.push(arr[i]);
>     }
>     result.push(value);
>   }
> };
> 
> > 2. This new suggested body uses an unnecessary extra loop plus a call to
apply
> > and min which presumably loops through the same array all over again, albeit
> not
> > in JavaScript.
> 
> The first loop to find the min length is done to precalculate the minimum
length
> rather than having to do length checks in the middle of the zipping operation.

> To be changing the length of the for loop (as you are by modifying the value
of
> "l" -- a bad variable name -- at present) is very unclear.

Well there again, I was following the convention in the file. If you look at the
forEach definition on line 169, :

"      var l = arr.length;  // must be fixed during loop... see docs$

I agree it's not a great variable name, but I was following convention. I agree
however that your more recent example is better and I will update the patch with
this.
> 
> 
> > Oh and as for the use of call for push, I was looking at 1184 where apply is
> > used in a similar vein, but that is not the normal convention so I will
change
> > that to simply push.
> 
> You know that "t_arr" is an array at that point and has a push() method. 
> Regardless, "t_arr' is a bad variable name and not camelCase per convention.

Sorry, I get mixed up sometimes since I'm not used to camelCase. Thanks for the
clarification.

> 
> > On 2010/06/13 05:32:12, nnaze wrote:
> > > the whole body can be simpler and easier to read if it uses the functional
> > array
> > > methods provided in this file. 
> > > 
> > > goog.array.zip = function(var_args) {
> > >   var minLength = Math.min.apply(null, goog.array.map(arguments,
> function(arr)
> > {
> > >     return arr.length;
> > >   });
> > > 
> > >   var result = [];
> > >   for (var i = 0; i < minLength; i++) {
> > >     result.push(goog.array.map(arguments, function(arr) {
> > >       return arr[i];
> > >     }));
> > >   }
> > >   return result;
> > > };
> > 
> > 
> 
>

http://codereview.appspot.com/1668041/diff/16001/17002#newcode1222
closure/goog/array/array.js:1222: return res;
Ok, will make this change. Thanks.

On 2010/06/13 19:12:17, nnaze wrote:
> On 2010/06/13 15:21:09, bjorn.tipling wrote:
> > I also wanted to mention that the patch I provided uses an assert, which
seems
> > to follow conventions in this file, is that not preferable? Also because of
> this
> > assert, arrays are required and not ArrayLike, although I could change it to
> > ArrayLike, yet I believe that may break because I may be wrong but I do not
> > think that some array like objects such as a NodeList have a length
property.
> 
> Your function should take ArrayLike and operate on anything with a length
> property.  NodeLists have length properties.  
> 
> https://developer.mozilla.org/en/DOM/NodeList

[pallosp, 2010-06-14 11:34:29]

http://codereview.appspot.com/1668041/diff/28001/24004
File closure/goog/array/array.js (right):

http://codereview.appspot.com/1668041/diff/28001/24004#newcode1197
closure/goog/array/array.js:1197: *  This is similar to the zip() function in
Python.  See {@link
Indentation should be the same as above.

http://codereview.appspot.com/1668041/diff/28001/24004#newcode1198
closure/goog/array/array.js:1198: * 
http://docs.python.org/library/functions.html#map}
This link points to the map function.

http://codereview.appspot.com/1668041/diff/28001/24004#newcode1200
closure/goog/array/array.js:1200: * @param {...goog.array.ArrayLike} var_args
Arrays to be combined.
Please restrict the type to non-nullable ({...!goog.array.ArrayLike} I guess).

http://codereview.appspot.com/1668041/diff/28001/24004#newcode1201
closure/goog/array/array.js:1201: * @return {!Array.<Array>} A new array of
arrays created from provided arrays.
The inner arrays aren't nullable either. The precise type annotation is
{!Array.<!Array>}.

http://codereview.appspot.com/1668041/diff/28001/24004#newcode1213
closure/goog/array/array.js:1213: goog.asserts.assertNumber(arr.length);
This type checking is done by the compiler, no assertion is necessary.

[bjorn.tipling, 2010-06-14 15:26:36]

Thank you. I have made the requested changes.

http://codereview.appspot.com/1668041/diff/28001/24004
File closure/goog/array/array.js (right):

http://codereview.appspot.com/1668041/diff/28001/24004#newcode1197
closure/goog/array/array.js:1197: *  This is similar to the zip() function in
Python.  See {@link
On 2010/06/14 11:34:29, pallosp wrote:
> Indentation should be the same as above.

Done.

http://codereview.appspot.com/1668041/diff/28001/24004#newcode1198
closure/goog/array/array.js:1198: * 
http://docs.python.org/library/functions.html#map}
On 2010/06/14 11:34:29, pallosp wrote:
> This link points to the map function.

Done.

http://codereview.appspot.com/1668041/diff/28001/24004#newcode1200
closure/goog/array/array.js:1200: * @param {...goog.array.ArrayLike} var_args
Arrays to be combined.
On 2010/06/14 11:34:29, pallosp wrote:
> Please restrict the type to non-nullable ({...!goog.array.ArrayLike} I guess).

Done.

http://codereview.appspot.com/1668041/diff/28001/24004#newcode1201
closure/goog/array/array.js:1201: * @return {!Array.<Array>} A new array of
arrays created from provided arrays.
On 2010/06/14 11:34:29, pallosp wrote:
> The inner arrays aren't nullable either. The precise type annotation is
> {!Array.<!Array>}.

Done.

http://codereview.appspot.com/1668041/diff/28001/24004#newcode1213
closure/goog/array/array.js:1213: goog.asserts.assertNumber(arr.length);
On 2010/06/14 11:34:29, pallosp wrote:
> This type checking is done by the compiler, no assertion is necessary.

Done.

[bjorn.tipling, 2010-06-15 20:10:01]

Is there anything else required or is this review finished?

Thank you,

Bjorn

On 2010/06/14 15:26:36, bjorn.tipling wrote:
> Thank you. I have made the requested changes.
> 
> http://codereview.appspot.com/1668041/diff/28001/24004
> File closure/goog/array/array.js (right):
> 
> http://codereview.appspot.com/1668041/diff/28001/24004#newcode1197
> closure/goog/array/array.js:1197: *  This is similar to the zip() function in
> Python.  See {@link
> On 2010/06/14 11:34:29, pallosp wrote:
> > Indentation should be the same as above.
> 
> Done.
> 
> http://codereview.appspot.com/1668041/diff/28001/24004#newcode1198
> closure/goog/array/array.js:1198: * 
> http://docs.python.org/library/functions.html#map%7D
> On 2010/06/14 11:34:29, pallosp wrote:
> > This link points to the map function.
> 
> Done.
> 
> http://codereview.appspot.com/1668041/diff/28001/24004#newcode1200
> closure/goog/array/array.js:1200: * @param {...goog.array.ArrayLike} var_args
> Arrays to be combined.
> On 2010/06/14 11:34:29, pallosp wrote:
> > Please restrict the type to non-nullable ({...!goog.array.ArrayLike} I
guess).
> 
> Done.
> 
> http://codereview.appspot.com/1668041/diff/28001/24004#newcode1201
> closure/goog/array/array.js:1201: * @return {!Array.<Array>} A new array of
> arrays created from provided arrays.
> On 2010/06/14 11:34:29, pallosp wrote:
> > The inner arrays aren't nullable either. The precise type annotation is
> > {!Array.<!Array>}.
> 
> Done.
> 
> http://codereview.appspot.com/1668041/diff/28001/24004#newcode1213
> closure/goog/array/array.js:1213: goog.asserts.assertNumber(arr.length);
> On 2010/06/14 11:34:29, pallosp wrote:
> > This type checking is done by the compiler, no assertion is necessary.
> 
> Done.

[nnaze, 2010-06-16 18:00:36]

LGTM. pallosp, any other comments?

[pallosp, 2010-06-17 06:44:16]

LGTM too.

On Wed, Jun 16, 2010 at 8:00 PM, <nnaze@google.com> wrote:

> LGTM. pallosp, any other comments?
>
>
> http://codereview.appspot.com/1668041/show
>

[nnaze, 2010-06-21 15:12:46]

I've pulled this patch.  Please close this issue.

[bjorn.tipling, 2010-06-21 15:24:37]

On 2010/06/21 15:12:46, nnaze wrote:
> I've pulled this patch.  Please close this issue.

Done.