7
\$\begingroup\$

I'm playing about with JavaScript and wanted to create a simple validation script.

It works ok but is a bit clunky. How could I improve it?

var el = document.getElementById("username");
var el_pwd = document.getElementById("password");
var el2 = document.getElementById("feedback");
var el3 = document.getElementById("ok");
var el4 = document.getElementById("ok2");

function checkUsername() {
var username = el.value;
var password = el_pwd.value;
if((username.length < 5) & (password.length <= 0)) {
el2.className = 'warning'; 
el2.textContent = "Username Not long enough yet..";
el2.style.color = "red";
} else {

  el2.textContent = " ";
 }
}

function checkPassword() {
var username = el.value;
var password = el_pwd.value;
if((username.length >= 5) & (password < 7) ) {

el2.textContent = "Password MUST be 7 or more characters";
el2.style.color = "red";
} else if ((username.length <= 4) & (password.length <= 0))  {

el2.className = 'warning'; 
el2.textContent = "Username Not long enough yet..";
el2.style.color = "red";


 } else {
  el2.textContent = " ";
  }
}

function usernameOK() {
var username = el.value;
if(username.length >= 5) {
el3.style.display="block";
} else {
    el3.style.display = "none";
 }
}

function passwordOK() {
var password = el_pwd.value;

if(password.length >= 7) {
el4.style.display="block";
} else {
    el4.style.display = "none";
 }
}

function feedBack() {
el2.className = 'tip';
el2.textContent = "The username MUST be at least 5 characters";
el2.style.color = "blue";
}




el.addEventListener("focus", feedBack, false);
el.addEventListener("blur", checkUsername, false);
el.addEventListener("blur", usernameOK, false);

el_pwd.addEventListener("focus", checkPassword, false);
el_pwd.addEventListener("blur", passwordOK, false);

var el = document.getElementById("username");
var el_pwd = document.getElementById("password");
var el2 = document.getElementById("feedback");
var el3 = document.getElementById("ok");
var el4 = document.getElementById("ok2");

function checkUsername() {
  var username = el.value;
  var password = el_pwd.value;
  if ((username.length < 5) & (password.length <= 0)) {
    el2.className = 'warning';
    el2.textContent = "Not long enough yet..";
    el2.style.color = "red";
  } else {

    el2.textContent = " ";
  }
}

function checkPassword() {
  var username = el.value;
  var password = el_pwd.value;
  if ((username.length >= 5) & (password < 7)) {

    el2.textContent = "Password MUST be 7 or more characters";
    el2.style.color = "red";
  } else if ((username.length <= 4) & (password.length <= 0)) {

    el2.className = 'warning';
    el2.textContent = "Not long enough yet..";
    el2.style.color = "red";


  } else {
    el2.textContent = " ";
  }
}

function usernameOK() {
  var username = el.value;
  if (username.length >= 5) {
    el3.style.display = "block";
  } else {
    el3.style.display = "none";
  }
}

function passwordOK() {
  var password = el_pwd.value;

  if (password.length >= 7) {
    el4.style.display = "block";
  } else {
    el4.style.display = "none";
  }
}

function feedBack() {
  el2.className = 'tip';
  el2.textContent = "The username MUST be at least 5 characters";
  el2.style.color = "blue";
}




el.addEventListener("focus", feedBack, false);
el.addEventListener("blur", checkUsername, false);
el.addEventListener("blur", usernameOK, false);

el_pwd.addEventListener("focus", checkPassword, false);
el_pwd.addEventListener("blur", passwordOK, false);
@import url(http://fonts.googleapis.com/css?family=Oswald);
 body {
  font-family: 'Oswald', 'Futura', sans-serif;
  margin: 0px;
  padding: 0px;
}
#feedback.warning {
  background-image: url('https://cdn2.iconfinder.com/data/icons/freecns-cumulus/32/519791-101_Warning-128.png');
  background-repeat: no-repeat;
  background-size: 20px 20px;
  padding-left: 30px;
}
#feedback.tip {
  background-image: url('http://bibliomancy.org/images/icons/QuestionMark.png?1347492574');
  background-repeat: no-repeat;
  background-size: 20px 20px;
  padding-left: 30px;
}
#ok {
  position: absolute;
  top: 5px;
  left: 250px;
  background-image: url('http://upload.wikimedia.org/wikipedia/commons/c/cc/Ambox_green_tick.png');
  background-repeat: no-repeat;
  background-size: 20px 20px;
  padding-left: 30px;
  height: 50px;
  width: 50px;
  display: none;
}
#ok2 {
  position: absolute;
  top: 30px;
  left: 250px;
  background-image: url('http://upload.wikimedia.org/wikipedia/commons/c/cc/Ambox_green_tick.png');
  background-repeat: no-repeat;
  background-size: 20px 20px;
  padding-left: 30px;
  height: 50px;
  width: 50px;
  display: none;
}
<form>
  Username:
  <input type="text" id="username">
  <br>Password:
  <input type="password" id="password">
</form>

<div id="feedback"></div>
<div id="ok"></div>
<div id="ok2"></div>

\$\endgroup\$
  • \$\begingroup\$ Thanks RubberDuck, I didn't know I needed to add all the code :) \$\endgroup\$ – Addioioi Sep 26 '14 at 12:48
  • \$\begingroup\$ You didn't need to add all of the code @Addioioi, we just have the ability to create "jsfiddles" right here on the site natively now. blog.stackoverflow.com/2014/09/… \$\endgroup\$ – RubberDuck Sep 26 '14 at 14:09
  • 1
    \$\begingroup\$ awesome! I'll remember to do that next time! \$\endgroup\$ – Addioioi Sep 26 '14 at 14:18
  • \$\begingroup\$ You are using a bitwise & where a logical && would be expected. \$\endgroup\$ – Johnny Mopp Sep 26 '14 at 18:47
11
\$\begingroup\$

Validation gone wrong

Every piece of code you write should solve a problem. Every good piece of code you write should solve a problem so small it has nearly no dependencies. So often, you'll end up writing a lot of little pieces of code in order to create beautiful software.

Take for instance a Calculator. Instead of cramming everything in one huge function. There will probably be tons of little methods that do as much as possible without knowing to much. A 'sum' method will have 2 arguments passed in, and return the sum. It couldn't care less where they come from.

Validation the right way

What you actually needed was some code that uses a validator to validate your usernames and passwords.

But what is a validator?

At it's core, a valdiator needs 2 things. It needs an input, and it needs a strategy to validate that input. It might even have a chain of strategies.

A Validator

First of, let's define our validator:

function createValidator(input, strategy) {
    var input = input,
        strategy = strategy;

    return {
        passes : function() {
            return strategy(input);
        }
    }
}

Now we can create our validators, we can start creating our strategies. For instance a LengthValidator strategy:

function createLengthValidator(min, max) {
    var minLength = min,
        maxLength = max || Infinity;

    return function(data) {
        return data.length <= max && data.length >= min;
    }
}

To use it we would do:

var usernameValidator = createValidator(
    document.getElementById("username").value,
    createLengthValidator(5)
);
var passwordValidator = createValidator(
    document.getElementById("password").value,
    createLengthValidator(7)
);

And then our check* functions:

function checkUsername() {
    if ( !usernameValidator.passes() ) {
        var feedback = document.getElementById("feedback");
        feedback.className = 'warning'; 
        feedback.textContent = "Username Not long enough yet..";
        feedback.style.color = "red";
    }   
}

function checkPassword() {
    if ( !passwordValidator.passes() ) {
        var feedback = document.getElementById("feedback");
        feedback.className = 'warning'; 
        feedback.textContent = "Password MUST be 7 or more characters";
        feedback.style.color = "red";
    }
}

Handle only the bare minimum

Your function checkUsername relies on the password (hunk?) and your checkPassword also checks the userName (hunk?). But they handle different criteria.

Your functions add a class, text and CSS. Woah, that's a lot. Let's refactor that using events.

Defining how it rolls

Defining the problem is always the hardest step. But somehow, a lot of people tend to skip this step. Don't.

What we want is the following: We have an input-field. Every time the input loses focus (blur event) the input should be evaluated. If the given input is not correct, show an error message.

Our problem is defined in 3 sentences. Each sentence defines a smaller problem. The first part is easy:

<input type="text" name="user_name" id="username" />

The second part, a little harder. But still doable since we already have our validator:

var $username = document.getElementById('username');

$username.addEventListener('blur', function() {
    var usernameValidator = createValidator(
        $username.value,
        createLengthValidator(5)
    );
    if ( usernameValidator.passes() ) {
        //create a validatorPassed event
        var event = new CustomEvent('validatorPassed');
        $username.dispatchEvent(event);
    } else {
        //create a validatorFailed event
        var event = new CustomEvent('validatorFailed');
        $username.dispatchEvent(event);
    }
});

Wow, so much code. But why?

Here is why:

$username.addEventListener('validatorPassed', function() {
    var feedback = document.getElementById("feedback");
    feedback.textContent = "";
});

$username.addEventListener('validatorFailed', function() {
    var feedback = document.getElementById("feedback");
    feedback.className = 'warning'; 
    feedback.textContent = "Username Not long enough yet..";
    feedback.style.color = "red";
});

See how we have successful decoupled our code? Our validator now knows nothing about our html. It simply validates input. We then have 2 eventListeners that listen to the Validator and do html-editing accordingly.

Disclaimer: I wrote this code inside the text-editor and is written as an example. I don't expect you to go all the way as I have. But it gives you the idea ;) always keep track of that one rule:

First make it work, then make it fast and then make it nice.

\$\endgroup\$
  • \$\begingroup\$ Wow thanks so much for a detailed answer and description, there is quite a bit in their that I haven't come accross yet, but I will see if I can work it out! thanks again! \$\endgroup\$ – Addioioi Sep 26 '14 at 18:14
  • \$\begingroup\$ @Addioioi I went full out to give you an example of what can be done by following patterns ;) You ofcourse don't have to use them. In small applications it often doesn't help. But once stuff gets biger. You will feel the need for patterns \$\endgroup\$ – Pinoniq Sep 26 '14 at 18:56
  • \$\begingroup\$ Do I just copy it exactly as you have written it? I'm trying to see it working and I can't! \$\endgroup\$ – Addioioi Sep 26 '14 at 19:03
  • \$\begingroup\$ Nah, It could be my code doesn't work. I wrote it into the text editor here. I will look at it this weekend \$\endgroup\$ – Pinoniq Sep 26 '14 at 20:21
4
\$\begingroup\$

Firstly, rename your variables to something more readable. It's entirely unintuitive what var e12 is supposed to be - try names like username_input or username_element. e12 could just be called feedback or warning_message

Your indentation makes the code kind of hard to follow visually; look up a style guide to see the correct javascript indentation, or follow the basics of:

function whatever(){
  console.log("This is in one block (function) so it is indented once.")
  if (bool) {
    console.log("This is in two blocks (function, if) so it is indented twice.")
  }
}

The line if((username.length >= 5) & (password < 7) ) { looks like it should probably have password.length.

You can use the ternary operator to change the body of usernameOK to:

var username = el.value;
el3.style.display = (username.length >= 5) ? "block" : "none"

You can do something similar for the body of passwordOK.

\$\endgroup\$
  • \$\begingroup\$ I might come back to expand on this in a bit but I'm at work and busy. This will help you clean up your code a bit, and with the variables renamed it will be easier to see if there are any formal improvements to make. \$\endgroup\$ – Devon Parsons Sep 26 '14 at 12:58
  • \$\begingroup\$ no,no thank you very much for the input. I will have to look up ternary operators as I see that ? all the time and never seem to understand it! \$\endgroup\$ – Addioioi Sep 26 '14 at 13:04
  • 1
    \$\begingroup\$ The variable name is very misleading since it's not e12 it's el2 for element2 (I guess). \$\endgroup\$ – Marc-Andre Sep 26 '14 at 13:17
  • 2
    \$\begingroup\$ Yeah exactly right, I've been reading a book and thats the style they do it in, so I adopted it, but I have now changed the variable names to more appropriate ones such as username_input, password_tick etc \$\endgroup\$ – Addioioi Sep 26 '14 at 13:29
  • 1
    \$\begingroup\$ @Addioioi if they use that style in the book. The rest of the book probably won't be worth much \$\endgroup\$ – Pinoniq Sep 26 '14 at 13:50

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.