Simplified code

[fearphage]

Used the native functionality instead of external modules. This greatly improved performance as a side effect:

➜  is-positive-integer git:(master) node benchmark.js
original x 1,749,034 ops/sec ±3.87% (87 runs sampled)
bitshift x 56,364,251 ops/sec ±1.65% (88 runs sampled)
Math.floor x 15,037,634 ops/sec ±5.51% (74 runs sampled)
parseInt x 13,990,961 ops/sec ±5.95% (76 runs sampled)
Fastest is bitshift

It's roughly 25 times faster now.

Source: https://gist.github.com/fearphage/3d50dd7a454a37e35392

[fr33kvanderwand]

why not simply return x > 0, ?

[f]

@fr33kvanderwand Because 1.2 is > 0 but not Integer but floating number.

[tjmehta]

Nice, passes all tests. Can you explain how this works exactly? Thanks 👍

[tjmehta]

Going to pull this in, but please do provide me with an explanation. I would love to add it in as comments. Thanks!

[cameroncooper]

This function returns false for Number.MAX_SAFE_INTEGER

[tjmehta]

@fearphage, thoughts on #5 ? EDIT: #6

[cameroncooper]

It's actually #6 I made an odd mistake pulling in the changes from #2 .I personally think #2 is neat though bitshifting in JS makes me nervous mostly for reasons like this.

[fearphage]

Nice, passes all tests.

It also adheres to your lint rules.

>> is a bitwise operator. More importantly it's the sign-propagating right shift operator. It says shift the number this many bits (0 in this case) and maintain the sign (positive or negative). Bit shifting only applies to integers. So bit shifting any non-integer is roughly equivalent to calling Math.floor(x) or parseInt(x, 10) or x | 0 (another bit shift operator).

This function returns false for Number.MAX_SAFE_INTEGER

I've read that MAX_SAFE_INTEGER is the largest number that can be expressed in 53 bits (53^2 - 1). It's an integer written as a double float. Bit shifting only works with integers and in the case of >> signed integers. MAX_SAFE_INTEGER doesn't have space for a sign. That's why MIN_SAFE_INTEGER exists. Going above this number gives strange results:

Math.pow(2, 53) === Math.pow(2, 53) + 1 // returns true

You could add || x === Number.MAX_SAFE_INTEGER to resolve this shortcoming.

[tjmehta]

Added a bunch of broken test cases here: https://github.com/tjmehta/is-positive-integer/pull/7/files

[tjmehta]

Looks like it breaks for more than just the "MAX" cases

[mitsuhiko]

Not sure if anyone cares, but from looking at the code the behavior was changed with regards to what happens if you pass non numeric objects in that define valueOf.

[tjmehta]

@mitsuhiko, which non numeric objects functionality changed? Is there a missing test case: https://github.com/tjmehta/is-positive-integer/blob/master/test/test.js?

[mitsuhiko]

@tjmehta for instance an object like this:

let foo = {
  valueOf: function() { return 42; }
};

In the old code this was not considered an integer, in this code I assume it will be one due to the coercion. I did not actually test any of this. Just saw the PR pop up on twitter.

[mitsuhiko]

Also pretty sure passing in true will have similar problems as it coerces into 1 in some contexts.

[twistor]

Why not?

Number.isInteger(x) && x > 0

[tjmehta]

@twistor, Number.isInteger is es6

[tjmehta]

@mitsuhiko, just tested, both are false. added test cases: https://github.com/tjmehta/is-positive-integer/pull/7/files

[mitsuhiko]

@tjmehta indeed. the === x check should catch those cases.

I have nothing further to add :)