Incorrect validation of numbers on vulcan.validation.numerics

[gtklocker]

The library incorrectly thinks that a single-element array is a number

var my_vulcan = new vulcan();
var arr = [42];
console.log(my_vulcan.validation.numerics.is_number(arr));

true

[g0d]

Hi @gtklocker. Actually I was mistaken. This is correct. Arrays are relational maps like in PHP in JavaScript. In your test case the zero index is implied and its value is 42. Therefore the test case is not wrong. It is actually correct!

[gtklocker]

Many thanks for your reply. If so, shouldn't the following two cases also be fixed accordingly?

var my_vulcan = new vulcan;
var obj = {0: 42};
console.log(my_vulcan.validation.numerics.is_number(obj));
// false

var map = new Map;
// Map(0) {}
map.set(0, 42);
// Map(1) {0 => 42}
console.log(my_vulcan.validation.numerics.is_number(map));
// false

[g0d]

Hi Kostis, definitely not! The JSon objects and BSONs are not considered arrays in any way. They are generalized abstract objects. Please read ECMA for more details about the previous array "strange" definition and interpretation. Thanks a lot for the comments again!

…

On Dec 4, 2017 21:02, "Kostis Karantias" ***@***.***> wrote: Many thanks for your reply. If so, shouldn't the following two cases also be fixed accordingly? var my_vulcan = new vulcan; var obj = {0: 42}; console.log(my_vulcan.validation.numerics.is_number(obj)); // false var map = new Map; // Map(0) {} map.set(0, 42); // Map(1) {0 => 42} console.log(my_vulcan.validation.numerics.is_number(map)); // false — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#2 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAaX6PH6KsAyqpZfIwJ8r21GqNd1x5wOks5s9EG-gaJpZM4Qz4HR> .

[petrosagg]

@g0d makes sense. There is a tiny detail that the zero index is implied regardless of length. As per the ECMA specification even if you have the array [42] that has a length of 1, the other indices still have a value. You can easily see that by doing console.log(arr[1]). It will print undefined but that's still a primitive value! https://www.ecma-international.org/ecma-262/5.1/#sec-4.3.9

What follows is that [ 42, 52 ] is also numeric since the implied arr[0] is still 42, which is a number. The fact that the second element is 52 instead of undefined is irrelevant. Do you think you can make the library handle this case? It'd be nice to have no edge cases :)

[g0d]

I agree but my problem and everyone's problem in this case, I guess, is that the interpreter is too weak due to those poorly defined specs. Since there are so many variables that are not "well defined" (with the mathematical point of view) then I may fail covering those edge cases. To be more precise, I am a bit reluctant to go forward with this because I am afraid to over do it. You know, if I over use code to do a simple validation then I am actually becoming a weak and slow tool/lib as well. I don't know if you can see my point here. I think it's more wise to leave that out until the most recent ECMA specification gets updated and finds its way into the core of the new JS parsers!

[petrosagg]

I understand your concern about performance but I think you can keep vulcan strong and fast using the following approach:

if (self.validation.misc.is_array(val))
    return !isNaN(val[0]);

[gtklocker]

@petrosagg @g0d Do you believe the zero index should be implied on is_number? isNaN obviously does imply it and the standard does mandate this weird cast (and from what I gather you're both pretty dissatisfied with that); but having any array qualify as a number seems absurd to me, especially when an array can be easily detected with self.validation.misc.is_array.

Here follows a standalone example which accomplishes this and doesn't sacrifice correctness or speed:

const is_number = x => typeof(x) === 'number' && !isNaN(x);

[abresas]

@gtklocker your function is written in versions of ecmascript not supported by older browsers.

But besides that, it doesn't work for Number instances, eg new Number(2)

it's much better to use

var is_number = function(x) {
    return (typeof(x) === "number" || x instanceof Number) && !isNaN(x);
}

[g0d]

Thank you all gents, I truly appreciate your involvement in this. I am not sure yet what is the best course of action. I will have to run several tests and then decide which works best under all exotic scenarios 😁 If you have any ideas I am more than happy to read about them. I will reopen the issue and let's see what we can achieve. Talk later....

…

On Dec 7, 2017 18:07, "Aleksis Brezas" ***@***.***> wrote: @gtklocker <https://github.com/gtklocker> your function is written in versions of ecmascript not supported by older browsers. But besides that, it doesn't work for Number instances, eg new Number(2) it's much better to use var is_number = function(x) { return (typeof(x) === "number" || x instanceof Number) && !isNaN(x) } — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAaX6A9eI1HSrElxR58USPqDVyWdx0KTks5s-A1cgaJpZM4Qz4HR> .

[g0d]

Btw, regarding this:

var is_number = function(x) {
return (typeof(x) === "number" || x instanceof Number) && !isNaN(x);
}

Why do you think "instanceof" is needed in the first place? Isn't "typeof" really enough to make sure a number is an actual... well, number :)

[abresas]

Well, interestingly the type of new Number(2) is "object" :D
even though you can use it normally eg new Number(2) + 2

[g0d]

Hi @abresas, I know that. I meant why do you need the 'x instanceof Number' comparison to evaluate the expression. Isn't typeof(x) === "number" and !isNaN(x) enough for that?
;-)

[g0d]

Fixed. Thanks.