2
\$\begingroup\$

I'd like to have a generic way for validating user provided email with a bias toward ease of use. Does the following do so in a clear way for implementations that have the URL interface available? The sample input below is only intended to exercise the range of expected input and demonstrate it works. The UI is not in scope and varies separately--the solution is only intended to provide input validation with usable feedback for implementations.

Any feedback welcome on the style, approach, etc.

function validEmail(input=''){
    const emailPatternInput = /^[^@]{1,64}@[^@]{4,253}$/, emailPatternUrl = /^[^@]{1,64}@[a-z][a-z0-9\.-]{3,252}$/i;
    let email, url, valid = false, error, same = false;
    try{
        email = input.trim();
        // handles punycode, etc using browser's own maintained implementation
        url = new URL('http://'+email);
        let urlderived = `${url.username}@${url.hostname}`;
        same = urlderived === email;
        valid = emailPatternInput.test( email );
        if(!valid) throw new Error('invalid email pattern on input:' + email);
        valid = emailPatternUrl.test( urlderived );
        if(!valid) throw new Error('invalid email pattern on url:' + urlderived);
    }catch(err){
        error = err;
    };
    return {email, url, same, valid, error};
}

[
 'user+this@はじめよう.みんな'
, 'stuff@things.eu'
, 'stuff@things'
, 'user+that@host.com'
, 'Jean+François@anydomain.museum','هيا@יאללה'
, '试@例子.测试.مثال.آزمایشی'
, 'not@@really'
, 'no'
].forEach(email=>console.log(validEmail(email), email));
\$\endgroup\$
4
0
\$\begingroup\$

I would avoid the habit of declaring multiple variables in the same line. In most cases, they're hard to read especially when they get long. The only one exception I'd do this would be if everything is short enough to read, like your let line. But that one has other issues as well: why the two falses? and why initialize with false when they end up as other types (string for email and url, and an Error instance for error)?

A general rule for try-catch is to only use it for unexpected errors, like for calls to other functions. Never use it as a control structure. In your case, you're using thrown errors to bail out of the block. Throwing an error inside your own try-catch is an anti-pattern. It's better to use a conditional instead.

Then theres the inconsistent use of syntax, like the use of template literals here but not there, or the one-time assignments to a let which can just be a const, etc.

Now, doing away with the try-catch, the only thing that's complex is the relationship between error and valid. Everything else is just a value resulting from an expression, which in the following example... is just a series of const.

In your code, you compute validity twice and determine which error to use based on which check failed. This probably led to the strange complexity in your version of the code, you had to bail out with your error before it did the next check. But you can flip it around. You can use the check to determine the error. Then define valid based on the presence or absence of that error.

function validEmail(input = '') {
  const inputPattern = /^[^@]{1,64}@[^@]{4,253}$/
  const emailPattern = /^[^@]{1,64}@[a-z][a-z0-9\.-]{3,252}$/i
  const email = input.trim()
  
  // handles punycode, etc using browser's own maintained implementation
  const url = new URL(`http://${email}`)
  const urlDerived = `${url.username}@${url.hostname}`

  // Same-ness is the same regardless of errors. Bumping up.
  const same = urlDerived === email;
  
  // Test for incorrectness, null if ok.
  const error = !inputPattern.test(email) ? new Error(`invalid email pattern on input: ${email}`)
    : !emailPattern.test(urlDerived) ? new Error(`invalid email pattern on url: ${urlDerived}`)
    : null
  
  // The presence/absence of the error determines validity.
  const valid = !Boolean(error)
  
  return { email, url, same, valid, error }
}

[
  'user+this@はじめよう.みんな',
  'stuff@things.eu',
  'stuff@things',
  'user+that@host.com',
  'Jean+François@anydomain.museum',
  'هيا@יאללה',
  '试@例子.测试.مثال.آزمایشی',
  'not@@really',
  'no'
].forEach(email => console.log(validEmail(email), email));

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Perhaps it would help your argument to share that using const over let can help avoid bugs with accidental reassignment \$\endgroup\$ Dec 4 '20 at 8:12
  • \$\begingroup\$ Thanks Joseph, this is useful feedback. The try-catch is initially used to trap unexpected user input errors. I can short-circuit by throwing errors where their input doesn't match the url and the URL interface will throw errors as well depending on input and sequence, using the same general type for all errors in a UI. Valid and same are different comparisons to expose for implementations for use in messaging to the user UI--that separates the concern while removing the need to do the same operation (message the UI and discard the result is the expected use). \$\endgroup\$
    – jimmont
    Dec 4 '20 at 9:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.