dbarbalato / magellan

JavaScript Latitude and Longitude Validation and Formatting
18 stars 12 forks source link

Longitude parsing and validation fails in specific cases #4

Closed markerikson closed 8 years ago

markerikson commented 9 years ago

Saw some intermittent failures when trying to turn a decimal number into a result using .longitude(). Sample case:

var l = magellan(-120.1).longitude(); // returns null

Finally tracked it down. The issue is that you're using .toFixed() , which is rounding the seconds value of 59.99999 to 60. That causes the longitude validation check of "seconds < 60" to fail.

I was able to resolve the issue by pulling in a numerical truncation function, and use that instead of .toFixed():

    // per http://stackoverflow.com/a/16696848/62937
    function truncateNumber(numToTruncate, intDecimalPlaces) {
        var numPower = Math.pow(10, intDecimalPlaces);
        return ~~(numToTruncate * numPower)/numPower;
    }   

    // Magellan factory
    function magellan() {

// .....

            // Handle function call when magellan( 123.4567 ) or similar
            else if (args.length >= 1 && typeof args[0] == 'number') {

                // Degrees is the integer portion of the input
                coordinate.degrees = parseInt(args[0]);

                var decimal = Math.abs(parseFloat(args[0]) - coordinate.degrees);
                var d60 = decimal * 60;
                coordinate.minutes = parseInt(d60);
                var m60 = (d60 - coordinate.minutes) * 60;
                coordinate.seconds = truncateNumber(m60, 4);
            } 
dbarbalato commented 9 years ago

Hi Mark,

Good find. How do you feel about making the fix yourself? I would accept the pull request from you containing these changes, provided all tests are passing.

Let me know, thanks!

aeinbu commented 8 years ago

Hi!

I had the same problem as Mark.

I tried out Mark's fix, but found some cases with a rounding problem, so I solved the problem in a different way.

I've submitted my solution in a pull request with this fix, passing all tests (including 2 new ones)

dbarbalato commented 8 years ago

Sorry about the delay guys... I've finally merged your PR

Thanks for contributing!

markerikson commented 8 years ago

And as it turns out, one of my coworkers noticed today that my "fix" was indeed truncating in some cases where it shouldn't. So yeah, this change ought to improve things. Thanks.