_.size doesn't check if obj is Object #650

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
@alexandernst

_.size("foo") will TypeError.
This is a small patch, but it could be extended to check for other type of argument.
In any case, _.size() should return anything instead of crashing.

_.size doesn't check if obj is Object
_.size("foo") will TypeError.
This is a small patch, but it could be extended to check for other type
of argument.
In any case, it should return *anything* instead of crashing.
@alexandernst

This comment has been minimized.

Show comment
Hide comment
@alexandernst

alexandernst Jun 22, 2012

Please ignore the diff (wrong line end).

The patch is at line 309:

// Return the number of elements in an object.
_.size = function(obj) {
  return _.isArray(obj) ? obj.length : _.isObject(obj) ? _.keys(obj).length : 0;
};

Please ignore the diff (wrong line end).

The patch is at line 309:

// Return the number of elements in an object.
_.size = function(obj) {
  return _.isArray(obj) ? obj.length : _.isObject(obj) ? _.keys(obj).length : 0;
};
@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Jun 22, 2012

Owner

It's a good thing that calling _.size on things that aren't arrays and objects can TypeError. Far better to fail fast on invalid input instead of giving you an arbitrary number.

Owner

jashkenas commented Jun 22, 2012

It's a good thing that calling _.size on things that aren't arrays and objects can TypeError. Far better to fail fast on invalid input instead of giving you an arbitrary number.

@jashkenas jashkenas closed this Jun 22, 2012

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Jun 22, 2012

Collaborator

That doesn't seem consistent with the underscore mentality. For instance, isEmpty returns true for anything that is not in the union type it expects.

Collaborator

michaelficarra commented Jun 22, 2012

That doesn't seem consistent with the underscore mentality. For instance, isEmpty returns true for anything that is not in the union type it expects.

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Jun 22, 2012

Owner

Ah -- good point. I didn't realize we were checking for strings in there. In general, Underscore functions are only supposed to work on their types ... so collections are objects, arrays, array-likes ... but not strings.

Perhaps we should remove string support from isEmpty ... or add it to this.

Owner

jashkenas commented Jun 22, 2012

Ah -- good point. I didn't realize we were checking for strings in there. In general, Underscore functions are only supposed to work on their types ... so collections are objects, arrays, array-likes ... but not strings.

Perhaps we should remove string support from isEmpty ... or add it to this.

@jashkenas jashkenas reopened this Jun 22, 2012

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Jun 22, 2012

Collaborator

Add it to this. Strings should be thought of as lists of characters.

Collaborator

michaelficarra commented Jun 22, 2012

Add it to this. Strings should be thought of as lists of characters.

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Jun 22, 2012

Owner

Add it to this. Strings should be thought of as lists of characters.

That way lies gnarliness in JavaScript. Some engines can index into strings, some can't ... Do you really think it would be a good idea to add string support to all of the enumeration / iteration functions?

Far easier to _.map(string.split('') ...

Owner

jashkenas commented Jun 22, 2012

Add it to this. Strings should be thought of as lists of characters.

That way lies gnarliness in JavaScript. Some engines can index into strings, some can't ... Do you really think it would be a good idea to add string support to all of the enumeration / iteration functions?

Far easier to _.map(string.split('') ...

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Jun 22, 2012

Collaborator

Do you really think it would be a good idea to add string support to all of the enumeration / iteration functions?

Haha, obviously. I'm the one who opened a pull request that did just that.

edit: or maybe I didn't? I can't find it now.

Collaborator

michaelficarra commented Jun 22, 2012

Do you really think it would be a good idea to add string support to all of the enumeration / iteration functions?

Haha, obviously. I'm the one who opened a pull request that did just that.

edit: or maybe I didn't? I can't find it now.

@fellars

This comment has been minimized.

Show comment
Hide comment
@fellars

fellars Aug 5, 2012

upgrading from 1.3.1 to 1.3.3 I realized that the change in size affects when passing in a null value. It used to return 0 but now it gives TypeError. I understand not understanding different types, but what about null's. Too much to ask for a check on null returns 0?

fellars commented Aug 5, 2012

upgrading from 1.3.1 to 1.3.3 I realized that the change in size affects when passing in a null value. It used to return 0 but now it gives TypeError. I understand not understanding different types, but what about null's. Too much to ask for a check on null returns 0?

@jashkenas

This comment has been minimized.

Show comment
Hide comment
@jashkenas

jashkenas Aug 6, 2012

Owner

Yep -- checking on null should preferably be a TypeError.

Owner

jashkenas commented Aug 6, 2012

Yep -- checking on null should preferably be a TypeError.

@DupsBlinq

This comment has been minimized.

Show comment
Hide comment
@DupsBlinq

DupsBlinq Aug 23, 2012

By throwing a TypeError on null values, using the size method for arrays is equivalent to using a .length check. I previously relied on the size method to provide the added functionality of protecting against these cases. (as fellars said above previously null values returned 0)

By throwing a TypeError on null values, using the size method for arrays is equivalent to using a .length check. I previously relied on the size method to provide the added functionality of protecting against these cases. (as fellars said above previously null values returned 0)

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Aug 23, 2012

Contributor

@DupsBlinq I reworked _.size to return 0 for falsey values (consistent with the majority of the API that does not throw errors), support strings, arguments objects, and jQuery/MooTools DOM collections (as they are array-like-objects and not arrays).

Contributor

jdalton commented Aug 23, 2012

@DupsBlinq I reworked _.size to return 0 for falsey values (consistent with the majority of the API that does not throw errors), support strings, arguments objects, and jQuery/MooTools DOM collections (as they are array-like-objects and not arrays).

@noprompt

This comment has been minimized.

Show comment
Hide comment
@noprompt

noprompt Aug 24, 2012

Throwing a TypeError makes sense though. _.size(undefined) == 0 seems a little strange. After all undefined is, well, undefined. How can we know it's length? If you're worried about null values and the like, why not call _.compact beforehand?

Throwing a TypeError makes sense though. _.size(undefined) == 0 seems a little strange. After all undefined is, well, undefined. How can we know it's length? If you're worried about null values and the like, why not call _.compact beforehand?

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Aug 25, 2012

Contributor

@noprompt It's a lib consistency issue. Methods like _.each, _.isEmpty, and _.pick don't error when passing falsey values and methods like _.extend, _.bindAll, and _.defaults don't error when attempting to write to read-only properties. In my experience type checking/guards just add unnecessary complexity and code for incorrect usage of the API.

Contributor

jdalton commented Aug 25, 2012

@noprompt It's a lib consistency issue. Methods like _.each, _.isEmpty, and _.pick don't error when passing falsey values and methods like _.extend, _.bindAll, and _.defaults don't error when attempting to write to read-only properties. In my experience type checking/guards just add unnecessary complexity and code for incorrect usage of the API.

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Aug 25, 2012

Collaborator

@noprompt: You're not going to be successful trying to convince real JavaScripters that type safety is a good thing. These are people that intentionally create heterogenous lists.

Collaborator

michaelficarra commented Aug 25, 2012

@noprompt: You're not going to be successful trying to convince real JavaScripters that type safety is a good thing. These are people that intentionally create heterogenous lists.

@noprompt

This comment has been minimized.

Show comment
Hide comment
@noprompt

noprompt Aug 25, 2012

@michaelficarra Thanks for the tip. I'll stop trolling.

@michaelficarra Thanks for the tip. I'll stop trolling.

@jashkenas jashkenas closed this in 866d244 Aug 31, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment