2
\$\begingroup\$

I have the following javascript function validateEmail(value). The goal of the function is to validate the users inputted email as they are typing. So if someone puts nfg@*gmail.com the email will be displayed with the * char crossed out. Anytime there is an invalid character the display will show it striked out. This must be updated on every key push. Here's a JSFiddle: https://jsfiddle.net/d4kgejym/1/

This is one of my first code reviews so please comment below if anymore information is needed.

Assumptions about email addresses: (not true in real world)

A valid email is defined as follows: ^[a-z0-9]*[@][a-z]*[.][a-z]{1,3}$

function validateEmail() {
  var value = $('#emailInput').val();

  // TODO unsupported case: nfgallim$@  :(

  // case 5 where has @ and . with chars after .
  // nfgallimore@gmail.com
  if (value.includes('@') &&
    value.substr(value.indexOf('@')).length > 1 &&
    value.includes('.') &&
    value.substr(value.indexOf('.')).length > 1) {

    $('#emailDisplay').html(validateCase5Regex(value));
  }

  // case 4 where has @ with chars after and . with no chars after .
  // nfgallimore@gmail.
  else if (value.includes('@') &&
    value.substr(value.indexOf('@')).length > 1 &&
    value.includes('.') &&
    value[value.length - 1] === '.') {

    $('#emailDisplay').html(validateCase4Regex(value));
  }

  // case 3 where has @ with chars after @
  // nfgallimore@gmail
  else if (value.includes('@') &&
    value.substr(value.indexOf('@')).length > 1) {

    $('#emailDisplay').html(validateCase3Regex(value));
  }

  // case 2 where has only @ with no chars after @
  // nfgallimore@
  else if (value.includes('@') &&
    value[value.length - 1] === '@') {

    $('#emailDisplay').html(validateCase2Regex(value));
  }

  // case 1 with just ^[a-z0-9]*$
  // nfgallimore
  else {
    $('#emailDisplay').html(validateCase1Regex(value));
  }
}

// nfgallimore@gmail.com
function validateCase5Regex(value) {
  var domainTypeRegex = new RegExp('^[a-z]$');

  var stringRegex = new RegExp('^[a-z0-9]*[@][a-z]*[.][a-z]{1,3}$');

  var periodIndex = value.indexOf('.'); // first period

  var firstStr = value.slice(0, periodIndex + 1);
  var substr = value.substr(periodIndex + 1);

  return !value.match(stringRegex) ?
    validateCase4Regex(firstStr) +
    GetHtmlErrorWithMaxLength(substr, domainTypeRegex, 3) : value;
}

// nfgallimore@gmail.
function validateCase4Regex(value) {
  var stringRegex = new RegExp('^[a-z0-9]*[@][a-z]*[.]$');

  // nfgallimore@gmail
  var strWithoutPeriodSymbol = value.slice(0, value.length - 1);

  return !value.match(stringRegex) ?
    validateCase3Regex(strWithoutPeriodSymbol) + '.' : value;
}


// nfgallimore@gmail
function validateCase3Regex(value) {
  var domainRegex = new RegExp('^[a-z]*$');
  var stringRegex = new RegExp('^[a-z0-9]*[@][a-z]*$');

  var atIndex = value.indexOf('@'); // split on first occurrence of @

  var firstStr = value.slice(0, atIndex); // nfgallimore@
  var domainStr = value.substr(atIndex + 1) // gmail

  // apply secondStrRegex to second str
  return !value.match(stringRegex) ?
    validateCase2Regex(firstStr) + GetHtmlError(domainStr, domainRegex) : value;
}

// nfgallimore@
function validateCase2Regex(value) {
  var regex = new RegExp('^[a-z0-9]*[@]$');

  var strWithoutAtSymbol = value.slice(0, value.length); // nfgallimore
  return !value.match(regex) ?
    validateCase1Regex(strWithoutAtSymbol) + '@' : value;
}

// nfgallimore
function validateCase1Regex(value) {
  var regex = new RegExp('^[a-z0-9]*$');

  return !value.match(regex) ?
    GetHtmlError(value, regex) : value;
}

// Helper function that returns html.
// Chars that fail the regex get striken out
function GetHtmlError(value, regex) {
  var letters = '';

  for (var i = 0, len = value.length; i < len; i++) {
    if (value[i].match(regex)) {
      letters += value[i];
    } else {
      letters += '<span class="strike">' + value[i] + '</span>';
    }
  }
  return letters;
}

// Helper function that returns html.
// Chars that fail the regex or exceed length get striken out
function GetHtmlErrorWithMaxLength(value, regex, length) {
    var letters = '';

    for (var i = 0, len = value.length; i < len; i++) {
        if (value[i].match(regex) && i < length) {
            letters += value[i];
        }
        else {
            letters += '<span class="strike">' + value[i] + '</span>';
        }
    }
    return letters;
}
\$\endgroup\$
  • \$\begingroup\$ I added the function. I think that should be all of them. I was extracting it from the entire web app to make it is simple as possible. \$\endgroup\$ – Nick Gallimore Sep 27 '18 at 20:46
  • 1
    \$\begingroup\$ Your regex is much too restrictive. \$\endgroup\$ – 200_success Sep 27 '18 at 20:51
  • \$\begingroup\$ ^[a-z0-9]*[@][a-z]*[.][a-z]{1,3}$ Is an underlying assumption of the code. That is something that I cannot alter, as that part is not up to me. I am merely implementing the best approach to enforce that as the user is typing. Already an email address such as nfgallimore@gmail.com.br would fail. Nothing I can do though. \$\endgroup\$ – Nick Gallimore Sep 27 '18 at 20:54
  • \$\begingroup\$ My recommendation would be to set the <input> elements type to email and let the browser worry about the validation. developer.mozilla.org/en-US/docs/Web/HTML/Element/… \$\endgroup\$ – Marc Rohloff Sep 27 '18 at 21:50
1
\$\begingroup\$

Initial Comments

@MarkRohloff's comment is good:

My recommendation would be to set the <input> elements type to email and let the browser worry about the validation.

Furthermore, while you stated "A valid email is defined as follows: ^[a-z0-9]*[@][a-z]*[.][a-z]{1,3}$", I am reminded of this SO answer for a regex for validating email addresses, which also reminds me of the great answer about parsing HTML with regular expressions.

Code Review

Let's look at the function valideEmail(). This function has a lot of coupling - with the view (i.e. input elements, output elements). If something needs to be updated in the HTML, then this function may need to be updated as well. A less-coupled approach might be to accept a single value and return a boolean value. That way the controlling code can pass the value (e.g. from the HTML view, or test code could pass test values) and then use the result accordingly (e.g. display a message in the view, or for test code it might compare the result with expected results).

Also, String.prototype.includes() (used in value.includes('.') in some of those conditional statements) appears to be a feature of EcmaScript-2015 A.K.A. es-6. Thus you could use other es-6 features, like String.prototype.endsWith() instead of value[value.length - 1], as well as arrow functions.

Additionally, DOM lookups (e.g. $('#emailInput'), $('#emailDisplay')) are not cheap, so it is wise to store those in a variable (and use const since you are using es-6 code), and utilize the variables whenever needed later.

\$\endgroup\$
1
\$\begingroup\$

Comments

  1. Don't use new RegExp if you can just use the literal syntax. new RegExp('^[a-z]*$') is equivalent to /^[a-z]*$/

  2. Instead of using ''.match(/^$/) to check if a string matches a regular expression, use /^$/.test('') to more clearly communicate your intent.

  3. Watch out for HTML characters! You got lucky in this case that since < is an invalid character, it will always be translated into <span class="strike"><</span>, however this isn't really something to rely on. Building HTML from strings is dangerous and should be avoided if at all possible.

  4. While this system helps the user recognize where they have included invalid characters, it doesn't tell them what they are missing. It would be helpful to indicate that they are missing part of the required string.

Invalid Emails

You state that the assumptions about emails are already baked in, and cannot be changed, but it really should be. Why? /^[a-z0-9]*[@][a-z]*[.][a-z]{1,3}$/.test('@.co') is true. According to this regex, @.co is a valid email. Chances are, this will cause issues someday. It is likely that whoever wrote the regex really meant ^[a-z0-9]+[@][a-z]+[.][a-z]{1,3}$, or more succinctly, ^[a-z0-9]+@[a-z]+\.[a-z]{1,3}$. I'd recommend bringing this up to whoever can make this change, even if modifying the system more than this isn't feasible.

Alternative Approach

I'd like to make a note on your approach to validating the email itself. At its core, you assign a pattern, consume the string until you find a signal character, and then change patterns until you hit another signal character (or length). Recognizing this makes it possible to significantly simplify your logic, and leave room for expansion in the future. Here's one way to implement this scanner.

// @ts-check

/**
 * Parses a string into an array of objects that can be combined to produce
 * a helpful display for the user to see where their input is invalid.
 * A matcher must mark its signal value as valid.
 * @param value {string} the string to parse.
 * @param matchers {{ valid: (c: string, i: number) => boolean, done: (c: string, i: number) => boolean}[]} the matchers that reveal if a character is valid.
 * @returns {{ text: string, valid: boolean }[]}
 */
function validateString(value, matchers) {
  const result = []

  let matcherIndex = 0
  let matcherLength = 0
  let outputLength = 0

  const append = (text, valid) => {
    outputLength += text.length
    if (!result.length || result[result.length - 1].valid !== valid) {
      result.push({ text, valid })
    } else {
      result[result.length - 1].text += text
    }
  }

  for (const char of value) {
    if (matchers.length <= matcherIndex) {
      break
    }
    const matcher = matchers[matcherIndex]

    append(char, matcher.valid(char, matcherLength))

    if (matcher.done(char, matcherLength++)) {
      matcherIndex++
      matcherLength = 0
    }
  }

  // If we ran off the end of the matchers, the rest of the string is invalid
  if (outputLength < value.length) {
    append(value.substring(outputLength), false)
  }

  return result
}

const emailMatchers = [
  { valid: c => /[a-z0-9@]/.test(c), done: c => c == '@' }, // ^[a-z0-9]*@
  { valid: c => /[a-z.]/.test(c), done: c => c === '.' }, // [a-z]*\.
  { valid: c => /[a-z]/.test(c), done: (_, i) => i === 2 } // [a-z]{1,3}
]

const emails = [
  'test@test.com',
  'test@*test.com',
  'test@*test.comasdf',
  'test***#@*#*test.com',
  '***aaa@bbb.com',
  '@example.com',
  '@asd.d**',
  'test$@'
]

const ul = document.querySelector('ul')
for (const email of emails) {
  const li = ul.appendChild(document.createElement('li'))
  for (const { text, valid } of validateString(email, emailMatchers)) {
    const el = document.createElement(valid ? 'span' : 'del')
    li.appendChild(el).textContent = text
  }
}

const resultEl = document.querySelector('div')

document.querySelector('input').addEventListener('input', ({ target }) => {
  resultEl.innerHTML = ''
  for (const { text, valid } of validateString(target.value, emailMatchers)) {
    const el = document.createElement(valid ? 'span' : 'del')
    resultEl.appendChild(el).textContent = text
  }
})
del {
  color: red;
}
<input placeholder="example@example.com">

<div></div>

<ul></ul>

\$\endgroup\$
  • \$\begingroup\$ I'd be hesitant to recommend users select an element by tag name - sure your example happens to only have one div and one ul element so it works here but it would be wise to get people in the habit of selecting an element by id or elements by class name (otherwise use a more complex selector)... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Oct 1 '18 at 20:37
  • \$\begingroup\$ If we are criticising the acceptance criteria of the email then I'd like to point out that admin@com is a valid email address \$\endgroup\$ – JGNI Oct 11 '18 at 10:07

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.