Skip to content
This repository has been archived by the owner on Oct 12, 2023. It is now read-only.

_isNumerical implementation doesn't make sense #48

Closed
billinghamj opened this issue Apr 4, 2018 · 4 comments
Closed

_isNumerical implementation doesn't make sense #48

billinghamj opened this issue Apr 4, 2018 · 4 comments

Comments

@billinghamj
Copy link

billinghamj commented Apr 4, 2018

I was just reviewing this code, and noticed that the following doesn't seem to make sense:

humps/humps.js

Lines 94 to 98 in d612998

// Performant way to determine if obj coerces to a number
var _isNumerical = function(obj) {
obj = obj - 0;
return obj === obj;
};

As, obviously, this code could never return false. You could replace the entire implementation with return true and it would behave the same way.

Perhaps this would be more in-line with the original intent?

var _isNumerical = function(obj) { 
  var num = obj - 0; 
  return num === obj; 
}; 

That would check if the input was precisely that number (including the type), but the comment states that it checks whether the value coerces to a number, so perhaps it should also really be double equals, rather than triple? Or perhaps the comment should be corrected?

@seansfkelley
Copy link

As, obviously, this code could never return false. You could replace the entire implementation with return true and it would behave the same way.

Not quite:

> NaN === NaN
false

Which I think is the intent, in which case it would perhaps be clearer for this function to be implemented as return !isNaN(obj - 0); or perhaps return !isNaN(+obj);.

@billinghamj
Copy link
Author

Ah yes I guess that could be it. Definitely should be using the explicit function for that - perhaps the Number.isNaN one though.

@billinghamj
Copy link
Author

billinghamj commented Apr 4, 2018

Yep, there we go: #13 (diff)

Looks like that review comment was not addressed.

@billinghamj
Copy link
Author

Closing due to inactivity

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants