JS style guide: NaN checks

[jquery]

JS style guide: NaN checks

[scottgonzalez]

I'd really prefer the clarity of 2 over the compressibility of 1.

[mgol]

Options x !== x and isNaN(x) are different since isNaN('a') === true etc. I'd prefer the first one since it's not broken as isNaN. We can even implement it as jQuery.isNaN for clarity.

EDIT: @gibson042 has updated his post so I updated mine as well. ;)

[gibson042]

Right @mzgol; I updated to the full form for an apples-to-apples comparison.

[Krinkle]

Agree with @scottgonzalez.

Also, isNaN is not broken. It just casts it to a number first:

Number('a')
// returns: NaN

+'a'
// returns: NaN

+'a' === +'a' // -> NaN === NaN
// returns: false

isNaN('a')
// returns: true

So you shouldn't call isNaN on values that aren't numbers in general, except when ...

• There is a typeof check first (e.g. when one really needs a strict answer to whether it is NaN)
• You don't need the typeof check (e.g. when one can reasonably assume it to be a number, like from parseFloat, or if you only care about it not being a number).
  Literally speaking both strings and NaN are "not a number", in the case of old IE length issue, the condition only needed was if ( !isNaN( len ) { (both the typeof check and number casting would be unneeded)

[mgol]

Agree with @scottgonzalez https://github.com/scottgonzalez.
Also, isNaN is not broken. It is just casts it to a number first:

That's exactly why it's broken. i'd understand if it just returned true on
non-numbers but having isNaN('a') and isNaN([2, 3]) true and isNaN(''),
isNaN([2]) and isNaN([]) false doesn't make sense from a semantic point of
view.

• There is a typeof check first (e.g. when one really needs a strict
  answer to whether it is NaN)

I.e. when we make sure we're outside of isNaN insanity region first. In
many cases using x !== x eliminates the need for a typeof check.

Literally speaking both strings and NaN are "not a number",

An empty string is "not a number" and yet isNaN('') is false.

[rwaldron]

In browsers that support it, we should just use Number.isNaN(n)

Edit: Chrome's Number.isNaN() is wrong

An empty string is "not a number" and yet isNaN('') is true.

I think you meant false

[rwaldron]

http://wiki.ecmascript.org/doku.php?id=harmony:number.isnan

[mgol]

@rwaldron What's wrong about Chrome's Number.isNaN? :/

(and I meant false, obviously, thx for pointing out my mistake; post corrected)

[mikesherov]

Since option 1 and option 2 are 2 different things, I'm not sure why this needs to be in the style guide. If it's unclear, either we should introduce a function that provides clarity, or add a clarifying comment.

[arschmitz]

I'm not in favor of option 1 i think this is confusing and since this is used very very little i cant see how the size difference is significant enough to warrant how confusing it is. I don't think the average dev knows that NaN is the only thing thats not equal to it self .isNaN is much clearer

[dmethvin]

The strict equality check feels like a trivia challenge since most people could live their life as JS programmers not knowing NaN is unequal to itself. Since we know it's a number at this point, it seems like isNaN() is the most straightforward way even if it's a few more bytes. I don't think we need a style guide issue for this though, the system worked if this was discussed in code review.

[scottgonzalez]

At this point it's safe to say that this won't go into our style guide. I would also not expect option 1 to be added in any more places than the one place it currently exists in Sizzle.