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.

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

Issue with isNumeric #127

Closed
xogeny opened this issue Jan 18, 2018 · 3 comments · Fixed by #129
Closed

Issue with isNumeric #127

xogeny opened this issue Jan 18, 2018 · 3 comments · Fixed by #129

Comments

@xogeny
Copy link
Contributor

xogeny commented Jan 18, 2018

I'm not sure what is going on in this function:

/**
 * Check if value is a finite number
 * @param {float} n - number to evaluate
 * @returns {boolean} True if n is a finite number
 */
export function isNumeric(n) {
    var isNum = false;
    if (typeof n === "number") {
        var num = parseFloat(n);
        isNum = !isNaN(num);
        if (isNum && !isFinite(num)) {
            throw {
                code: "D1001",
                value: n,
                stack: new Error().stack,
            };
        }
    }
    return isNum;
}

According to the JSDoc, n is presumed to be a number (not a string?). This is substantiated by the typeof n === "number" conditional. While parseFloat can take a number as an argument, what is the point if we know that n is already a number (which we know is the case because of the type check)?

@andrew-coleman
Copy link
Member

The JSDoc is wrong here, n could be a value of any type, this function is testing to see if it is a number and in the value space of a JSON number - i.e. not infinity or NaN.

@xogeny
Copy link
Contributor Author

xogeny commented Jan 19, 2018

Sorry, I perhaps buried the lede here. That call to parseFloat seems completely superfluous here. We know (because of the typeof that n is a number. Calling parseFloat with a number just returns the argument (as far as I can tell). So you don't need the variable num here...you could just use n since they should be identical. Or am I missing something?

@andrew-coleman
Copy link
Member

Ah yes, good point. I'll remove that.

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

Successfully merging a pull request may close this issue.

2 participants