isValid method throws error if undefined is passed in

[EnzoMartin]

No check on if string is defined (or is actually a string) is done before invoking indexOf which results in throwing an unhandled error instead of returning false as would be expected

ipaddr.IPv6.isValid = (string) ->
  # Since IPv6.isValid is always called first, this shortcut
  # provides a substantial performance gain.
  if string.indexOf(":") == -1
    return false

[whitequark]

Duplicate of #32

[EnzoMartin]

That PR is closed as not merged, so the fact that the isValid function does not always return a boolean still isn't addressed

[whitequark]

The precondition for isValid is that the argument is a string. That was always the case, and it was working with undefined by mere accident.

[EnzoMartin]

That's the thing though, you can't be sure it's a string, and if the code is specifically tailored to only work with certain kinds of input (in this case a string or array), then it should always default to the fail case, which in this case would be false

[EnzoMartin]

All it would take to fix it and make it bulletproof is check the type is a string:

if typeof string !== "string" || string.indexOf(":") === -1

Otherwise you're intentionally leaving a hole in your code that will generate unhandled exceptions for people if malformed data is passed in. Since there's no callback, there's no way to properly handle it from my code's perspective should unexpected data go in, even more so when it's part of another module

[whitequark]

that doesn't handle String objects though.

[EnzoMartin]

indexOf is only available on arrays or strings

[whitequark]

=> new String("x").indexOf
< indexOf() { [native code] }
=> typeof new String("x")
< "object"

[whitequark]

oh well, nevermind, that wouldn't break the check, just make it slightly slower.

[EnzoMartin]

Awesome!

Sorry to be a bit pedantic about this, but technically that check still isn't right as you should be checking that string is not a string then return false. That was my mistake in my original comment which I edited later but you probably missed it, apologies!

It passes your test now because it never does the 2nd check of indexOf, and then being catch in the try/catch, but it technicaly should be failing early by ensuring that any continuance of the function will only be done with string being a string

[whitequark]

That was done deliberately. You can see that the logical operator is also reversed; this is to handle the case of passing a String object.

[EnzoMartin]

Fair enough

[EnzoMartin]

Thanks for fixing this 👍