Colleague asked me to review his code. I honor this top tier email validation.

LGTM!

I thought that only JS can come with shit like returning false from a function that’s supposed to find the position of a character in a string.

At least it doesn't invalidate valid addresses.

LGTM, so geht's auch.

Well. He ain't wrong.

It's just as easy to come up with a false valid for this as it is for any other offline check, and at least it doesn't have false invalids.

@Lensflare PHP's standard library is the worst. It is all over the place in termas of naming and argument order and error signaling. Most of it are just thinnest-possible wraps around commonly used C libraries.

That basically is how i check for "valid" email addresses too. Actual verification is happening by using it - most often by just sending an email containing a confirmation link.

I saw regexps spanning over almost a full page trying to get as clever as possible - all they actually do is rejecting totally valid addresses because the writer didn't read the spec carefully or made an error. Simpler is better when it comes to email address verification.

@horus Exception would be wrong and impractical.
If the language supports optionals, I would prefer null.
If it doesn’t, I would prefer a magic number stored in an appropriately named constant. Like NotFound with the value -1.

Honestly, this is the best I've seen so far.

And @Oktokolo pointed out a lot of good things why e-mail validation is mostly done wrong. Like totally wrong.

Even filter_var function isn't entirely correct, read the fine print:

https://php.net/manual/en/...

Yes... There might be unicode mail addresses for example. Rare unicorns exist.

So checking for @ is the most plausible, most performant and efficient solution.

Regarding regex stuff: not only are they wrong, but many of them open a very dangerous ReDOS vulnerability as they go haywire depending on length of string.

Even the length of the email can be invalid according to RFC - less than 60 characters before the @ / local part - but still be valid...

Mail is a completely violated and perverted thing.

Don't validate emails with RegEx / complicated logic.

I cannot stress that enough...

@ LensFlare I'm pretty sure @Horus is saying that he thinks the code is fine, questions how/if -1 or an exception would be better. It's obviously returning a user-readable error message, very similar to code I wrote pre HTML5. These days, type = 'email' is all you need client side, usual sanitisation/safeguarding re user input, obvs.

@spongegeoff so, you don’t see a problem in a function returning an integer or a boolean?

@LensFlare Sorry if I'm missing the point entirely, but no, ofc I don't have a problem with functions that return a number or a boolean. If I wanted to return a readable error message for email strings without an '@', the above in the OP would be fine. What am I not seeing?

@spongegeoff the problem with that is that you need to read and rely on the documentation of the strpos function to check for the "not found" case.

It’s better to encode that into the returning type, if it’s supported by the language (Optional<Int>).
So that it’s crystal clear what the possible return values are.

Or, if the language doesn’t support it, at least make it return a value of just one type (Int -1 as a named constant).
So that the caller doesn’t need to type check and cast the values around if he needs both, the found integer and the "not found" value.
It’s also more readable than just "false", which has no inherent meaning in the context of this function. The function is supposed to find a position, represented by an integer, so what does "false" even mean? It’s a question of good API design.

@LensFlare Sorry, I don't agree. The above is pretty much the standard PHP code to test for the presence of a substring, usually a single character, in a string. See it everywhere.

@spongegeoff I think you misunderstand.
I’m not criticizing the code shown here.
I’m criticizing the design of PHP function strpos itself.

@Lensflare @spongegeoff

to be fair... A *lot* of the core PHP functions are... Ancient.

The chance is zero that this will get changed ever... As one would have to rewrite the whole PHP engine probably.

So yeah, it is like it is.

There were many RFCs regarding consolidation of function names etc, they all ended the same: rejected cause the consequences would be too invasive.

@Lensflare Does 'false' not make sense? You ask for the position of a substring or character in a string, it doesn't have a position, so it's false ('not true') that it has a position in the string. We don't test for 'falsey' because the character could be found at position 0, so === false is the way to go.

I wonder how many bugs have been caused by the design choice of coupling a valid result and an error indication in the return value of a function. And making a difference between 0 and false doesn't sound like it would reduce unintended use.

@spongegeoff no. False would make sense for a function that checks if a character is contained in the string.
But since this function finds the position, false makes no sense, because there is no position "false".

Null would make sense, because null represents missing values, so it would indicate that the position is missing (not found).

Anything that can return a meaningful false, should also be able to return a meaningful true. This function only returns false (and integers).

@IntrusionCM yes, of course I know that it won’t change. All I want to say is that this API is crap. And other languages do it better.

@Lensflare Yes. Agree.

Pythons index method throws an ValueError if I'm not mistaken...

I find that the most fitting thing to be honest, as Options don't exist.

Scala has Options and returns -1... Which I always hated as much as returning false. XD

I guess I'm a sucker for exceptions.

@IntrusionCM Oh, rein it in @Lensflare! It's a really straightforward function. Nothing even remotely difficult about how it works or why it works that way. Almost too simple to think about.

Considering the herculian task that is proper email address verification I kinda get it

Task failed successfully