add isNumber function to util

[mahmoud-alragabi]

this function is much safer than just using isNaN with Number because it rejects values like true, false,"" while Number accepts them as valid numbers

[TylerVigario]

Thanks for bringing this to our attention. We could use ES6 Number.isNaN() instead of a custom function. Should be on-par with your fix as well.

[mahmoud-alragabi]

no because isNaN('287349') is false and isNaN('foobar') is also false , which shouldn't be since '287349' is a string of a number and 'foobar' isn't.
should i rename the function to isNumeric ? since I intend for it to allow string inputs that represent a number ?

[TylerVigario]

Apologies, I should have made it clear that ES6's Number.isNaN() is different from isNaN(). It does not coerce a value to a number (like ES5's isNaN) and instead does strict type checking prior.

https://www.w3schools.com/jsref/jsref_isnan_number.asp#:~:text=Difference%20Between%20isnan()%20and%20Number.&text=isNaN()%20method%20returns%20true,is%20Not%2Da%2DNumber.

Personally, I enforce strict type checking (via typeof) for my functions.

[TylerVigario]

@MedoAngel After looking at the source again (it's been a while) I like the changes. It would be nice to get valid numbers from strings while rejecting invalid ones. Could we move the type checking to the entry function? I think it would be better to validate user input further up if possible.

n2words/lib/n2words.mjs

Line 67 in 8f87b9a

export default function(n, options = {lang: 'en'}) {

[mahmoud-alragabi]

still returns false for normal strings, even if it returned true based on the type not even being a number that would disallow numeric strings from passing . so in summary using isNaN would give false for true and false for '234324' and true for 'foobar' while Number.isNaN will give false on all of them

[TylerVigario]

still returns false for normal strings, even if it returned true based on the type not even being a number that would disallow numeric strings from passing . so in summary using isNaN would give false for true and false for '234324' and true for 'foobar' while Number.isNaN will give false on all of them

I understand and agree that numeric values (even strings) should be accepted.

[mahmoud-alragabi]

@MedoAngel After looking at the source again (it's been a while) I like the changes. It would be nice to get valid numbers from strings while rejecting invalid ones. Could we move the type checking to the entry function? I think it would be better to validate user input further up if possible.

n2words/lib/n2words.mjs

Line 67 in 8f87b9a

export default function(n, options = {lang: 'en'}) {

yes I agree will get right on it , would you like me to change the name as suggested to isNumeric ? and also add typeof checks to the function ?

[TylerVigario]

I agree that isNumeric seems more appropriate. We shouldn't need to enforce type checking anywhere else as long as we validate and convert to a number.

EDIT: Apologies, I just remembered languages can be called without the entry file. We would have to keep the number validation within the floatToCardinal() function.

Apart from the function name change, this LGTM.

[mahmoud-alragabi]

@TylerVigario done, changed and pushed

[TylerVigario]

@MedoAngel What would happen if we use "3px" as input? Since we use parseFloat() in the type checking but Number() for conversion there are discrepancies.

It seems the type checking would pass for "3px" (since parseFloat() strips text if it finds a valid number) but fail for the actual number initialization (since Number() fails on any text).

[mahmoud-alragabi]

@TylerVigario you're absolutely right, idk how i missed it, will modify the code later and look into other places where Number is used in the code base and see if it would be better using parseFloat instead, or should i check for non numeric chars and exit early , this wouldn't allow 3px to even pass while parseFloat would give 3

[TylerVigario]

Sounds good 👍

EDIT: I too wonder if we should allow "3px" and similar inputs.

[mahmoud-alragabi]

Sounds good 👍

EDIT: I too wonder if we should allow "3px" and similar inputs.

I think we shouldn't , this wouldn't be number to words this would be parsing text for numbers then convert to words

[TylerVigario]

@MedoAngel Agreed, I think it will just cause issues outside our scope like with decimals and money.

[mahmoud-alragabi]

@TylerVigario decimals should still be parsed since the lib support that, but symbols unrelated to numbers should be rejected

[TylerVigario]

@MedoAngel I meant how decimals take on a bit of a different meaning when used in currencies. So disallowing all text (including currency symbols) would certainly help steer users away from trying to the use the library for currencies (which it's not currently designed to handle).

[mahmoud-alragabi]

@MedoAngel I meant how decimals take on a bit of a different meaning when used in currencies. So disallowing all text (including currency symbols) would certainly help steer users away from trying to the use the library for currencies (which it's not currently designed to handle).

Yes Exactly

[mahmoud-alragabi]

@TylerVigario done with the changes we discussed earlier and added type checking just in case and for some speed improvement in case it's used on large data

[TylerVigario]

@MedoAngel Wouldn't it be more efficient to do this directly in floatToCardinal() (since we don't accept user input anywhere else)? I was thinking something more like this so we don't have to do both Number() and parseFloat() checks.

    if (value == NaN || typeof value != 'number') {
      if (typeof value != 'string' || isNaN(value = Number(value))) {
        throw new TypeError(`Invalid number: ${value}, of type: ${typeof value}`);
      }
    }

That would take care of the boolean issue and reduce the need to check for text in the value.

EDIT: Updated
EDIT 2: Updated
EDIT 3: Added check for NaN

[mahmoud-alragabi]

No because typeof NaN is number which returns true

[TylerVigario]

@MedoAngel Added check for NaN

EDIT: Hmm, nevermind, I guess that wouldn't work out as cleanly as I had thought.

[mahmoud-alragabi]

@MedoAngel Added check for NaN

@TylerVigario Would still fail because '' is a string and Number('') is 0 so would return true

[TylerVigario]

@MedoAngel Added check for NaN

Would still fail because '' is a string and Number('') is 0 so would return true

True, however parseFloat doesn't parse leading white spaces.

EDIT: I wonder what's the use-case for allowing string types at all?

[mahmoud-alragabi]

@MedoAngel Added check for NaN

Would still fail because '' is a string and Number('') is 0 so would return true

True, however parseFloat doesn't parse leading white spaces.

EDIT: I wonder what's the use-case for allowing string types at all?

It does parse leading and trailing white space doesn't parse white space between two numbers which again isn't in our scope

And i see that allowing strings would allow users to use the result directly from an input

Edit: and it's not even about allowing string types , it's about giving a more safe test for the value which previously allowed strings, null , undefined ..etc

[TylerVigario]

It does parse leading and trailing white space doesn't parse white space between two numbers which again isn't in our scope

I didn't know that but I am glad to hear and I agree that spaces between isn't within our scope.

And i see that allowing strings would allow users to use the result directly from an input

We aren't dealing with user input as this is only a library. It's the responsibility of the application developer to ensure user input isn't malformed before passing it along. When we allow string types, we now have to check for thousands of things we don't allow.

Edit: and it's not even about allowing string types , it's about giving a more safe test for the value which previously allowed strings, null , undefined ..etc

It was already "safe" as it either inferred a number from the value or gave a TypeError. This PR reduces that inference and does a bunch of checks to make sure text doesn't have any illegal characters.

EDIT: Improved grammar
EDIT 2: Expanded on responsibility beliefs & input tolerance

[mahmoud-alragabi]

It was already "safe" as it either inferred a number from the value or gave a TypeError. This PR reduces that inference and does a bunch of checks to make sure the text doesn't have any illegal characters.

Sorry, should've made it clear what I mean by type safe, I don't mean that the program crashes, what I mean is that it doesn't accept illogical input like null, true ... etc, I think that if this PR doesn't get merged we should at least have a note at the read me page saying that this lib automatically infer type number from input and it's their responsibility to check for the input given

[TylerVigario]

@MedoAngel I totally agree that feeding n2words an empty string or false would produce "zero" in return which it should not and I concede this PR would improve that.

However, it probably won't change much down the line for developers.

For example; an application developer hooks their user input straight to this library without validating. Previously they'll get "zero" from an empty string but now it will throw a TypeError. Either way though, the application developer needs to handle an empty string situation within their application. Nothing we can do will absolve the application developer the responsibility of validating their user input.

It seems to me the best way to reduce improper use is to disable string types.

EDIT 4/24: This is probably a breaking change since previously handled illogical input will produce different results now.

[TylerVigario]

Resolved in #84. I wanted to merge this in but ultimately decided against it. I kept the logic within the floatToCardinal function and could avoid the overlapping parseFloat by trimming strings. Hopefully, there are no hard feelings. Thanks anyway!