Skip to content

Use array.length = 0 instead of goog.array.clear #3136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 16, 2015

Conversation

fredj
Copy link
Member

@fredj fredj commented Jan 15, 2015

And goog.object.clear(obj) should probably changed to obj = {}

@gberaudo
Copy link
Member

Looks good, +1.

@tschaub
Copy link
Member

tschaub commented Jan 15, 2015

Looks good to me. Please merge.

I'd also be in favor of replacing goog.array.clone(array) with array.slice().

@elemoine
Copy link
Member

+1 for this change. Thanks @fredj.

For people's information, this is what goog.array.clear does:

/**
 * Clears the array.
 * @param {goog.array.ArrayLike} arr Array or array like object to clear.
 */
goog.array.clear = function(arr) {
  // For non real arrays we don't have the magic length so we delete the
  // indices.
  if (!goog.isArray(arr)) {
    for (var i = arr.length - 1; i >= 0; i--) {
      delete arr[i];
    }
  }
  arr.length = 0;
};

It also works for ArrayLike (goog.array.ArrayLike), whatever that is.

@tschaub
Copy link
Member

tschaub commented Jan 15, 2015

The only justification I can imagine for using these functions is if you're dealing with "array like" arguments (arguments or a node list).

@elemoine
Copy link
Member

Looks good to me. Please merge.

I'd also be in favor of replacing goog.array.clone(array) with array.slice().

Me too.

goog.array.clone is an alias to goog.array.toArray, which looks like this:

/**
 * Converts an object to an array.
 * @param {Array.<T>|goog.array.ArrayLike} object  The object to convert to an
 *     array.
 * @return {!Array.<T>} The object converted into an array. If object has a
 *     length property, every property indexed with a non-negative number
 *     less than length will be included in the result. If object does not
 *     have a length property, an empty array will be returned.
 * @template T
 */
goog.array.toArray = function(object) {
  var length = object.length;

  // If length is not a number the following it false. This case is kept for
  // backwards compatibility since there are callers that pass objects that are
  // not array like.
  if (length > 0) {
    var rv = new Array(length);
    for (var i = 0; i < length; i++) {
      rv[i] = object[i];
    }
    return rv;
  }
  return [];
};

There is also goog.array.slice that we might want to use:

/**
 * Returns a new array from a segment of an array. This is a generic version of
 * Array slice. This means that it might work on other objects similar to
 * arrays, such as the arguments object.
 *
 * @param {Array.<T>|goog.array.ArrayLike} arr The array from
 * which to copy a segment.
 * @param {number} start The index of the first element to copy.
 * @param {number=} opt_end The index after the last element to copy.
 * @return {!Array.<T>} A new array containing the specified segment of the
 *     original array.
 * @template T
 */
goog.array.slice = function(arr, start, opt_end) {
  goog.asserts.assert(arr.length != null);

  // passing 1 arg to slice is not the same as passing 2 where the second is
  // null or undefined (in that case the second argument is treated as 0).
  // we could use slice on the arguments object and then use apply instead of
  // testing the length
  if (arguments.length <= 2) {
    return goog.array.ARRAY_PROTOTYPE_.slice.call(arr, start);
  } else {
    return goog.array.ARRAY_PROTOTYPE_.slice.call(arr, start, opt_end);
  }
};

but if we know we have an Array and not an goog.array.ArrayLike then it makes sense to use Array.prototype.slice directly.

@elemoine
Copy link
Member

The only justification I can imagine for using these functions is if you're dealing with "array like" arguments (arguments or a node list).

It sounds like we're in agreement.

@elemoine
Copy link
Member

We only have 7 occurrences of goog.array.clone, so changing them to using slice should be straightforward :-)

fredj added a commit that referenced this pull request Jan 16, 2015
Use array.length = 0 instead of goog.array.clear
@fredj fredj merged commit 1033171 into openlayers:master Jan 16, 2015
@fredj fredj deleted the coding-style branch January 16, 2015 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants