-
Notifications
You must be signed in to change notification settings - Fork 962
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
New feature isNumber & improve type checking for @turf/helpers fixes #914 #920
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/turf-helpers/index.js
Outdated
@@ -125,6 +126,7 @@ function polygon(coordinates, properties, bbox, id) { | |||
throw new Error('Each LinearRing of a Polygon must have 4 or more Positions.'); | |||
} | |||
for (var j = 0; j < ring[ring.length - 1].length; j++) { | |||
if (i === 0 && j === 0 && !isNumber(ring[0][0]) || !isNumber(ring[0][1])) throw new Error('Coordinates must contain numbers'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DenisCarriere maybe you could add a comment to briefly explain why this test only on the first coords, or just a reference to the issue. It might seem odd otherwise for somebody who looks at the code for the first time, don't you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we check a single random coordinate per ring instead of just the first one? That way we would introduce a slightly better check with more or less the same effort. No?
Like (not tested):
var randomRing = Math.floor(Math.random() * coordinates.length);
for (var i = 0; i < coordinates.length; i++) {
var randomCoord = Math.floor(Math.random() * ring.length);
for (var j = 0; j < ring[ring.length - 1].length; j++) {
if (i === randomRing && j === randomCoord && !isNumber(ring[randomCoord][0]) || !isNumber(ring[randomCoord][1])) throw new Error('Coordinates must contain numbers');
...
}
}
(naming too long, but just to give an idea)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Good train of thought, however testing the first point would be plenty, this would most likely catch 99% of all typing errors, this type of type check validation shouldn't even be included since this type of error is already being caught in the Typescript definition as Position[][]
.
As for benchmark performance, simply adding isNumber
is a 2x speed reduction, introducing a Math.random
+ Math.floor
would slow things down even more.
Doing 1 point validation should be plenty enough to catch those silly errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe you could add a comment to briefly explain why this test only on the first coords
👍 yep sounds good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stebogit Actually the Math.random
+ Math.floor
didn't slow things down as much as I thought:
// Using isNumber + Random Point selection
polygon x 5,305,908 ops/sec ±0.99% (87 runs sampled)
// Only using isNumber
polygon x 6,006,338 ops/sec ±1.07% (86 runs sampled)
// No number Type checking
polygon x 11,742,649 ops/sec ±0.83% (90 runs sampled)
New feature
isNumber
& improve type checking for@turf/helpers
isNumber
feature to@turf/helpers
isNumber
to@turf/turf
Ref: #914
CC: @stebogit @markmeyerphoto