Skip to content
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.

Sign up for GitHub

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

Jump to bottom

New feature isNumber & improve type checking for @turf/helpers fixes #914 #920

Merged
merged 2 commits into from
Sep 1, 2017
Merged

New feature isNumber & improve type checking for @turf/helpers fixes #914 #920

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f1e689
Add isNumber & improve type checking for @turf/helpers
DenisCarriere Aug 29, 2017
b93fe9b
Add comment to isNumber type checking
DenisCarriere Aug 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/turf-helpers/bench.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ const {
*/
const suite = new Benchmark.Suite('turf-helpers');
suite
.add('round', () => round(120.123))
.add('point', () => point([5, 10]))
.add('round', () => round(120.123))
.add('lineString', () => lineString([[5, 10], [20, 40]]))
.add('polygon', () => polygon([[[5, 10], [20, 40], [40, 0], [5, 10]]]))
.add('multiPoint', () => multiPoint([[0, 0], [10, 10]]))
Expand Down
5 changes: 5 additions & 0 deletions packages/turf-helpers/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,3 +132,8 @@ export function convertDistance(distance: number, originalUnit: Units, finalUnit
* http://turfjs.org/docs/#convertarea
*/
export function convertArea(area: number, originalUnit?: Units, finalUnit?: Units): number

/**
* http://turfjs.org/docs/#isnumber
*/
export function isNumber(num: any): boolean
21 changes: 20 additions & 1 deletion packages/turf-helpers/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ function point(coordinates, properties, bbox, id) {
if (!coordinates) throw new Error('No coordinates passed');
if (coordinates.length === undefined) throw new Error('Coordinates must be an array');
if (coordinates.length < 2) throw new Error('Coordinates must be at least 2 numbers long');
if (!isNumber(coordinates[0]) || !isNumber(coordinates[1])) throw new Error('Coordinates must contain numbers');

return feature({
type: 'Point',
Expand Down Expand Up @@ -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');
Copy link
Collaborator

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?

Copy link
Collaborator

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)

Copy link
Member Author

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.

Copy link
Member Author

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

Copy link
Member Author

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)

if (ring[ring.length - 1][j] !== ring[0][j]) {
throw new Error('First and last Position are not equivalent.');
}
Expand Down Expand Up @@ -169,6 +171,7 @@ function polygon(coordinates, properties, bbox, id) {
function lineString(coordinates, properties, bbox, id) {
if (!coordinates) throw new Error('No coordinates passed');
if (coordinates.length < 2) throw new Error('Coordinates must be an array of two or more positions');
if (!isNumber(coordinates[0][1]) || !isNumber(coordinates[0][1])) throw new Error('Coordinates must contain numbers');

return feature({
type: 'LineString',
Expand Down Expand Up @@ -497,6 +500,21 @@ function convertArea(area, originalUnit, finalUnit) {
return (area / startFactor) * finalFactor;
}

/**
* isNumber
*
* @param {*} num Number to validate
* @returns {boolean} true/false
* @example
* turf.isNumber(123)
* //=true
* turf.isNumber('foo')
* //=false
*/
function isNumber(num) {
return !isNaN(num) && num !== null && !Array.isArray(num);
}

module.exports = {
feature: feature,
geometry: geometry,
Expand All @@ -516,5 +534,6 @@ module.exports = {
bearingToAngle: bearingToAngle,
convertDistance: convertDistance,
convertArea: convertArea,
round: round
round: round,
isNumber: isNumber
};
35 changes: 34 additions & 1 deletion packages/turf-helpers/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ const {
bearingToAngle,
convertDistance,
convertArea,
round
round,
isNumber
} = require('./');

test('point', t => {
Expand Down Expand Up @@ -411,3 +412,35 @@ test('turf-helpers -- Handle Id & BBox properties', t => {
t.throws(() => featureCollection([pt], [0], {invalid: 'id'}), 'throws invalid id');
t.end();
});

test('turf-helpers -- isNumber', t => {
t.throws(() => point(['foo', 'bar']), /Coordinates must contain numbers/, 'Coordinates must contain numbers');
t.throws(() => lineString([['foo', 'bar'], ['hello', 'world']]), /Coordinates must contain numbers/, 'Coordinates must contain numbers');
t.throws(() => polygon([[['foo', 'bar'], ['hello', 'world'], ['world', 'hello'], ['foo', 'bar']]]), /Coordinates must contain numbers/, 'Coordinates must contain numbers');

// true
t.true(isNumber(123));
t.true(isNumber(1.23));
t.true(isNumber(-1.23));
t.true(isNumber(-123));
t.true(isNumber('123'));
t.true(isNumber(+'123'));
t.true(isNumber('1e10000'));
t.true(isNumber(1e10000));
t.true(isNumber(Infinity));
t.true(isNumber(-Infinity));

// false
t.false(isNumber(+'ciao'));
t.false(isNumber('foo'));
t.false(isNumber('10px'));
t.false(isNumber(NaN));
t.false(isNumber(undefined));
t.false(isNumber(null));
t.false(isNumber({a: 1}));
t.false(isNumber({}));
t.false(isNumber([1, 2, 3]));
t.false(isNumber([]));
t.false(isNumber(isNumber));
t.end();
});
25 changes: 25 additions & 0 deletions packages/turf-helpers/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,28 @@ helpers.geometryCollection([pt.geometry], properties, bbox, 1)
helpers.point(pt.geometry.coordinates, {foo: 'bar'})
helpers.point(pt.geometry.coordinates, {1: 2})
helpers.point(pt.geometry.coordinates, {1: {foo: 'bar'}})

// isNumber -- true
helpers.isNumber(123)
helpers.isNumber(1.23)
helpers.isNumber(-1.23)
helpers.isNumber(-123)
helpers.isNumber('123')
helpers.isNumber(+'123')
helpers.isNumber('1e10000')
helpers.isNumber(1e10000)
helpers.isNumber(Infinity)
helpers.isNumber(-Infinity)

// isNumber -- false
helpers.isNumber(+'ciao')
helpers.isNumber('foo')
helpers.isNumber('10px')
helpers.isNumber(NaN)
helpers.isNumber(undefined)
helpers.isNumber(null)
helpers.isNumber({a: 1})
helpers.isNumber({})
helpers.isNumber([1, 2, 3])
helpers.isNumber([])
helpers.isNumber(helpers.isNumber)
6 changes: 4 additions & 2 deletions packages/turf/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ import {
degrees2radians,
round,
convertDistance,
convertArea} from '@turf/helpers';
convertArea,
isNumber} from '@turf/helpers';
import {
getGeom,
getGeomType,
Expand Down Expand Up @@ -243,5 +244,6 @@ export {
clone,
segmentEach,
segmentReduce,
cleanCoords
cleanCoords,
isNumber
};
1 change: 1 addition & 0 deletions packages/turf/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ var turf = {
degrees2radians: helpers.degrees2radians,
radians2degrees: helpers.radians2degrees,
convertDistance: helpers.convertDistance,
isNumber: helpers.isNumber, // 4.7.0
round: helpers.round,
convertArea: helpers.convertArea,
getCoord: invariant.getCoord,
Expand Down
6 changes: 4 additions & 2 deletions packages/turf/module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ import {
degrees2radians,
round,
convertDistance,
convertArea} from '@turf/helpers';
convertArea,
isNumber} from '@turf/helpers';
import {
getGeom,
getGeomType,
Expand Down Expand Up @@ -243,5 +244,6 @@ export {
clone,
segmentEach,
segmentReduce,
cleanCoords
cleanCoords,
isNumber
};