Clean this JS up

JavaScript
13

Here are a few thoughts – some important, and some less so.

  • Everything should be in an immediately invoked function expression (IIFE). Otherwise, any of your variables and functions might clash with other globals in ways that are hard to diagnose.

  • I would get rid of the email validation. It’s a lot of risk for little benefit. If the regex is imperfect in any way, then you risk rejecting perfectly valid emails. And even if the regex is perfect, you still haven’t verified that totes@whatevs.tv is actually my email. You could probably use the HTML5 type="email" and let the browser do the validation for you, but anything beyond that yields diminishing returns.

  • Try to name your functions as verbs, not nouns. Maybe validatePassword() instead of passwordErrors(). The exception to that rule is a function that returns a boolean. Those can be named like a question: isEmailValid().

  • Your emailErrors and passwordErrors functions seem to have nearly identical code. Paul_Wilkins and also mawburn have good solutions for that.

  • If you tell the user that their password is invalid, you should also tell them why (“must be at least 5 characters”), otherwise the user won’t have any idea how to fix the error.

  • The “submit” listener seems to doubly invoke emailErrors and passwordErrors, first &&'ed then ||'ed. I’m guessing you wanted to make sure they both run while also not submitting if either fails. Let’s try to rewrite that to achieve both goals without invoking the functions twice.

document.getElementById("login").addEventListener("submit", function(e) {
  e.preventDefault();

  var hasEmailErrors = emailErrors();
  var hasPasswordErrors = passwordErrors();

  if(hasEmailErrors || hasPasswordErrors)
  {
    return false;
  }
  else
  {
    return true;
  }
}, true);
3 Likes