Clean this JS up

RyanReese · April 18, 2015, 2:27am

http://www.codefundamentals.com/cadeui/login

How can I clean up those event listeners? I seem to be repeating the same crap and I don’t like how it currently is.

Off Topic:

Any Design/UX comments always welcome so far

mawburn · April 18, 2015, 2:40am

Put anything that’s repeated, into functions or variables. Keep it DRY (Don’t Repeat Yourself)

function emailError() {
    var errElm = document.getElementsByClassName("email-error")[0];

    errElm.innerHTML="Error: Invalid username. Please try again.";
    errElm.classList.remove("hide");
    emailInput.classList.add("error");
    emailInput.classList.remove("success");
    errors=true;
}

Same thing for success and password error.

RyanReese · April 18, 2015, 2:55am

Cleaned up. Could another review be given?

mawburn · April 18, 2015, 2:58am

It’s not on live.

RyanReese · April 18, 2015, 3:00am

You sure you aren’t just seeing a cached version?

mawburn · April 18, 2015, 3:01am

Tried 2 different browsers.

RyanReese · April 18, 2015, 3:03am

Weird - I don’t have a development environment; doing everything live. Here is the copy/pasted code though.

var submitButton, emailInput, passwordInput, validateEmail, showHide, errors;
validateEmail = function(email) {var e = (email+'@').split('@'); if (e[0].length > 255 || e[1].length > 63) return false; return !email.search(/^[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*(?:(?:[^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff]+(?![^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff])|"[^\\\x80-\xff\n\015"]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015"]*)*")[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*(?:\.[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*(?:[^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff]+(?![^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff])|"[^\\\x80-\xff\n\015"]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015"]*)*")[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*)*@[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*(?:[^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff]+(?![^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff])|\[(?:[^\\\x80-\xff\n\015\[\]]|\\[^\x80-\xff])*\])[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*(?:\.[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*(?:[^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff]+(?![^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff])|\[(?:[^\\\x80-\xff\n\015\[\]]|\\[^\x80-\xff])*\])[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*)*|(?:[^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff]+(?![^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff])|"[^\\\x80-\xff\n\015"]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015"]*)*")[^()<>@,;:".\\\[\]\x80-\xff\000-\010\012-\037]*(?:(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)|"[^\\\x80-\xff\n\015"]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015"]*)*")[^()<>@,;:".\\\[\]\x80-\xff\000-\010\012-\037]*)*<[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*(?:@[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*(?:[^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff]+(?![^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff])|\[(?:[^\\\x80-\xff\n\015\[\]]|\\[^\x80-\xff])*\])[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*(?:\.[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*(?:[^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff]+(?![^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff])|\[(?:[^\\\x80-\xff\n\015\[\]]|\\[^\x80-\xff])*\])[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*)*(?:,[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*@[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*(?:[^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff]+(?![^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff])|\[(?:[^\\\x80-\xff\n\015\[\]]|\\[^\x80-\xff])*\])[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*(?:\.[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*(?:[^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff]+(?![^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff])|\[(?:[^\\\x80-\xff\n\015\[\]]|\\[^\x80-\xff])*\])[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*)*)*:[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*)?(?:[^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff]+(?![^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff])|"[^\\\x80-\xff\n\015"]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015"]*)*")[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*(?:\.[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*(?:[^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff]+(?![^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff])|"[^\\\x80-\xff\n\015"]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015"]*)*")[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*)*@[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*(?:[^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff]+(?![^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff])|\[(?:[^\\\x80-\xff\n\015\[\]]|\\[^\x80-\xff])*\])[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*(?:\.[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*(?:[^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff]+(?![^(\040)<>@,;:".\\\[\]\000-\037\x80-\xff])|\[(?:[^\\\x80-\xff\n\015\[\]]|\\[^\x80-\xff])*\])[\040\t]*(?:\([^\\\x80-\xff\n\015()]*(?:(?:\\[^\x80-\xff]|\([^\\\x80-\xff\n\015()]*(?:\\[^\x80-\xff][^\\\x80-\xff\n\015()]*)*\))[^\\\x80-\xff\n\015()]*)*\)[\040\t]*)*)*>)$/);}

submitButton=document.getElementById("submit");
emailInput=document.getElementById("email");
passwordInput=document.getElementById("password");
showHide=document.getElementsByClassName("showHide")[0];

function emailErrors()
{
  if(!validateEmail(email.value))
  {
    document.getElementsByClassName("email-error")[0].innerHTML="Error: Invalid username. Please try again.";
    document.getElementsByClassName("email-error")[0].classList.remove("hide");
    emailInput.classList.add("error");
    emailInput.classList.remove("success");
    return true;
  }
  else
  {
    emailInput.classList.add("success");
    emailInput.classList.remove("error");
    document.getElementsByClassName("email-error")[0].innerHTML="";
    document.getElementsByClassName("email-error")[0].classList.add("hide");
    return false;
  }
}
function passwordErrors()
{
  if(passwordInput.value.length===0)
  {
    document.getElementsByClassName("password-error")[0].innerHTML="Error: Invalid password. Please try again.";
    document.getElementsByClassName("password-error")[0].classList.remove("hide");
    passwordInput.classList.add("error");
    passwordInput.classList.remove("success");
    return true;
  }
  else
  {
    passwordInput.classList.add("success");
    passwordInput.classList.remove("error");
    document.getElementsByClassName("password-error")[0].innerHTML="";
    document.getElementsByClassName("password-error")[0].classList.add("hide");
    return false;
  }
}
showHide.addEventListener("click", function() {
  if(showHide.classList.contains("showPW"))
  {
    showHide.classList.remove("showPW");
    showHide.classList.add("hidePW");
    passwordInput.type="text";
    showHide.innerHTML="Hide";
  }
  else
  {
    showHide.classList.remove("hidePW");
    showHide.classList.add("showPW");
    passwordInput.type="password";
    showHide.innerHTML="Show";
  }
});
document.getElementById("login").addEventListener("submit", function(e) {
  errors=false;
  e.preventDefault();
  if(emailErrors() || passwordErrors())
    return false;
  else
    return true;
});
emailInput.addEventListener("blur", emailErrors);
passwordInput.addEventListener("blur", passwordErrors);

Paul_Wilkins · April 18, 2015, 3:08am

You could start by moving common code out to a separate function, such as for when an error occurs:

function fieldError(field, type, message) {
    document.getElementsByClassName(type)[0].innerHTML = "Error: " + message + " Please try again.";
    document.getElementsByClassName(type)[0].classList.remove("hide");
}

So that you can then use it from different related areas:

if(!validateEmail(email.value))
{
    fieldError(emailInput, "email-error", "Invalid username.");
    ...
}

Then you can look for other types of duplication and deal with them.

You may also want to browse through this code review of form validation where a large post was made detailing how improvements are made using a variety of different helpful beneficial techniques.

mawburn · April 18, 2015, 3:19am

Ah I see the problem. You didn’t do what I meant …because I didn’t explain it right. I don’t work with the DOM that much with vanilla, so the code is a little hard to read.

I meant this:

function emailErrors() {
    if(something) {
       return postError('email-error');
    } else {
       return postSuccess();
    }
}

function postError(className) {
    var errElm = document.getElementsByClassName(className)[0];

    errElm.innerHTML="Error: Invalid username. Please try again.";
    errElm.classList.remove("hide");
    emailInput.classList.add("error");
    emailInput.classList.remove("success");
    errors=true;
}

Then you would do the same sort of thing to the success.

Any reason for not using jQuery? Have you considered Zepto.js? It would really clean up a lot of that clutter. Dev Speed/Ease > A few extra kb the size of your blank-profile.png

$('.someclass').addClass('success').removeClass('error');

or even

$('.someclass').toggleClass('success').toggleClass('error');

RyanReese · April 18, 2015, 3:49am

Just practice really.

I’m going to do this in jQuery in the morning though. For this CadeUI project, it’s just be absolutely silly not to do it otherwise.

Could you take a look adn see why my form isn’t submitting though? Also what logic could I use to validate both fields before submission? Right now, asy ou can see, it’s a little poor logic.

This is more informational and if not answered by the time I wake up, I’ll just scrap it for jQuery.

mawburn · April 18, 2015, 4:01am

I’m off the computer for tonight, but I will tomorrow.

Jeff_Mott · April 18, 2015, 6:41am

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);

Jeff_Mott · April 18, 2015, 6:46am

Also, make a dev environment and use version control.

RyanReese · April 18, 2015, 1:34pm

@felgall says it’s pefect . I will be using type=“email” but that’s a pain to test on so it’s type="text for now.

As far as that last point you made, I had to do && and || because if the username has errors on submit, it automatically would stop and not validate the password field. Thanks for the help.

RyanReese · April 18, 2015, 2:20pm

http://jqueryvalidation.org/documentation/

http://www.codefundamentals.com/cadeui/login-test#

How can I make that validation use my pre-made <span>s in the source?

mawburn · April 18, 2015, 2:30pm

I’d follow everything @Jeff_Mott said. That’s a pretty good deep analysis of the structure and he’s right on every point.

As far as domains go, there are a ton of TLDs now. All encompassing rgexes are a thing of the past while I was replying here last night. I was working on registering [my last name].ninja and plan on switching to [my first name]@[my last name].ninja as my email. (namechep has a $3 sale for .ninja right now)

RyanReese · April 18, 2015, 2:35pm

I’m using jquery validation now. One final thought
http://www.codefundamentals.com/cadeui/login-test

Do you guys mind the jump for error messages/success?

If so, then how can I modify that jquery validation plugin to use existing elements on the page?

felgall · April 18, 2015, 9:16pm

No I didn’t. I said I based it on the one built into PERL for validating email addresses there. So it is exactly as accurate in detecting whether email addresses are formatted correctly or not as the person who originally wrote it was able to get it and exactly as accurate in validating email addresses as PERL is (assuming no copy/paste errors).

The same applies to if you use an email filter in PHP or type=“email” in HTML - in each case you are relying on how the ability of the person who wrote the corresponding code in PHP or the browser to correctly follow the actual standard for what is and isn’t valid in an email address.

Using one that is built in eliminates the possibility of you making a typo and you can always complain if you come across a valid email address that someone else’s code rejects as invalid.

The current recommended way to test email addresses in the browser is to use type=“email”

As Jeff pointed out, any email validation done in the browser can only ever test if what is entered is in a valid format (for example “# ??!”@[127.0.0.1]) to actually validate that an email address is correct requires sending an email to that address and getting a reply.

system · July 19, 2015, 4:23am

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.