We help IT Professionals succeed at work.

refactor form validation js

max7
max7 asked
on
Greetings,

Check out the following:

<html>
	<head>
		<script type="text/javascript" src="http://ajax.googleapis.com/ajax/libs/jquery/1/jquery.min.js"></script>
	</head>
	<body>
		<form id="theForm" action="process.php">
			<label>Username</label>
			<input type="text" name="username" id="username"/>
			<label>Email</label>
			<input type="text" name="email" id="email"/>
			<label>Password</label>
			<input type="text" name="password" id="password"/>
		</form>
		<input id="submit" type="button" value="submit" />
		<script type="text/javascript">
		$(document).ready(function(){
			Validation ={
				username : $('#username'),
				email : $('#email'),
				password : $('#password'),
				msgChar	:'Each field must have at least 4 characters',
				msgBlank : 'Form fields cannot be left blank',
				msgThanks : 'Thanks for submitting our form!',
				
				validateForm : function(){
					if(Validation.username.val()== ''){
						alert(Validation.msgBlank);
						return false;
					} else if(Validation.username.val()!= '' && Validation.username.val().length <=3){
						alert(Validation.msgChar);
						return false;
					}
					
					if(Validation.email.val()== ''){
						alert(Validation.msgBlank);
						return false;
					}else if (Validation.email.val()!= '' && Validation.email.val().length <=3){
						alert(Validation.msgChar);
						return false;
					}
					
					if(Validation.password.val()== ''){
						alert(Validation.msgBlank);
						return false;
					}else if (Validation.password.val()!= '' && Validation.password.val().length <=3){
						alert(Validation.msgChar);
						return false;
						}
						return true;
					}		 
				};
				
			$('#submit').click(function(){
				if(Validation.validateForm()){
					alert(Validation.msgThanks);
				} else { 
					return false;
				}
			});	
		});
	</script>
	</body>
</html>

Open in new window


Two things: 1) this is just practice I'm doing on my own and 2) the js works as is, but I'm sure it could be re-factored to eliminate all the repeated code areas that are common to each if statement.  I would like see different ways, perhaps better, to coide this.

Thanks.

Comment
Watch Question

Michel PlungjanIT Expert
Top Expert 2009

Commented:
Several issues, the worst is the submit button is outside the form

I will refactor here:http://jsfiddle.net/mplungjan/AcH6z/

and post the result
IT Expert
Top Expert 2009
Commented:
Here
<html>
  <head>
    <script type="text/javascript" src="http://ajax.googleapis.com/ajax/libs/jquery/1/jquery.min.js"></script>
    <script type="text/javascript">
    $(document).ready(function(){
      Validation ={
        username : $('#username'),
        email : $('#email'),
        password : $('#password'),
        msgChar  :'Each field must have at least $1 characters',
        msgBlank : 'Form fields cannot be left blank',
        msgThanks : 'Thanks for submitting our form!',
        isBlank:function(fld) {
          if (fld.val() == "") {
            alert(this.msgBlank);
            fld.focus();
            return true;
          }
          return false;  
        },
        isTooShort: function (fld, len) {
          if (fld.val().length < len) {
            alert(this.msgChar.replace(/\$1/,len));
            fld.focus();
            return true;
          }
          return false;
        },   
        validateForm : function(){
          if(this.isBlank(this.username) || this.isTooShort(this.username,4)){
            return false;
          }
          if(this.isBlank(this.email) || this.isTooShort(this.email,4)){
            return false;
          }
          if(this.isBlank(this.password) || this.isTooShort(this.password,4)){
            return false;
          }
          return true;
        }
      }
      
      
      $('#submit').click(function(){
        if(Validation.validateForm()){
          alert(Validation.msgThanks);
          return true;
        }
        return false;
      });  
    });
  </script>
  </head>
  <body>
    <form id="theForm" action="process.php">
      <label>Username</label>
      <input type="text" name="username" id="username"/>
      <label>Email</label>
      <input type="text" name="email" id="email"/>
      <label>Password</label>
      <input type="text" name="password" id="password"/>
      <input id="submit" type="button" value="submit" />
    </form>

  </body>
</html>

Open in new window

Author

Commented:
wow.  Night and day, I'd say.  I had thought about using arguments but couldn't think of a way to incorporate it.  And the submit button thing is embarrassing, I just wasn't thinking.

some questions:


1) I notice that with your code, if I put the required 4 characters in a field, I receive the alert msgBlank and then the next field which is empty is highlighted and selected for entered information.  I realize focus() is doing some of that but how it is it that the next empty field is selected?

2) for each function isBlank and isTooShort,  there is a return true and then a return false.  Could you please explain what is going on here i.e. why first return true and then return false?

3) Regarding this line here:

alert(this.msgChar.replace(/\$1/,len));

Open in new window


Why use the replace?  Is that to clean unwanted characters out of whatever is entered in the fields?  If so, is that just standard "good practice" when validating a form?
Michel PlungjanIT Expert
Top Expert 2009

Commented:
1) I focus whatever FIRST field is in error - the return false in each of the validateForm tests makes sure the validation stops at the first error

2) in JavaScript, code after a return is never executed so

isTooShort: function (fld, len) {
  if (fld.val().length < len) {
    alert(this.msgChar.replace(/\$1/,len));
    fld.focus();
    return true; // the field was blank, get out of here and tell the calling function that the field was blank
  }
  return false; // implicit ELSE here
}

3) I decided you might want to ask for 5 characters in another field, so whatever you pass in is used both to test the length AND to construct the message by replacing whereever you put $1 with whatever you fill in.

We could take that one step further and have generic "Please fill in the $1 field" and replace that with fld.name for example

Author

Commented:
>>>We could take that one step further and have generic "Please fill in the $1 field" and replace that with fld.name for example

If it isn't too much to ask, how would that look?
Michel PlungjanIT Expert
Top Expert 2009

Commented:
       msgChar  :'The $1 field must have at least $2 characters',
        msgBlank : 'The $1 field cannot be left blank',


       isBlank:function(fld) {
          if (fld.val() == "") {
            alert(this.msgBlank.replace(/\$1/,fld.name));
            fld.focus();
            return true;
          }
          return false;  
        },
        isTooShort: function (fld, len) {
          if (fld.val().length < len) {
            alert(this.msgChar.replace(/\$1/,fld.name).replace(/\$2/,len));
            fld.focus();
            return true;
          }
          return false;
        },  

Author

Commented:
Regarding this code:

$('#submit').click(function(){
        if(Validation.validateForm()){
          alert(Validation.msgThanks);
          return true;
        }
        return false;
      });  

Open in new window


I'm not clear why there is a return true and then, return false.  If the form passes validation, I would assume we would want to stop the script from executing since everything is ok.

Btw, I want you to know that I realize you've answered my initial question and deserve the points to be awarded.  I'm hoping you don't mind that I dig deeper on these since I really want to understand it better.
Michel PlungjanIT Expert
Top Expert 2009

Commented:
No problem

return true will submit the form, return false will stop the submission
you need BOTH statements

$('#submit').click(function(){
        if(Validation.validateForm()){
          alert(Validation.msgThanks); // the validation passed
          return true; // allow the submission
        } // implicit else
        return false; // disallow submission
      });  
Michel PlungjanIT Expert
Top Expert 2009

Commented:
ACTUALLY I would prefer


$('#theForm').submit(function(e){ // pass the event
  if(Validation.validateForm()){
    alert(Validation.msgThanks); // the validation passed
    return true; // allow the submission
  } // implicit else
  e.preventDefault(); // stop the default event
  return false; // disallow submission (for old time's sake)
});  

Author

Commented:
Sorry for letting this question languish; I've gotten sucked into a project but I still have perhaps a couple of more questions forthcoming before i close it out?
Michel PlungjanIT Expert
Top Expert 2009

Commented:
No problems

Author

Commented:
regarding this code:

 msgChar  :'The $1 field must have at least $2 characters',
        msgBlank : 'The $1 field cannot be left blank',


       isBlank:function(fld) {
          if (fld.val() == "") {
            alert(this.msgBlank.replace(/\$1/,fld.name));
            fld.focus();
            return true;
          }
          return false;  
        },
        isTooShort: function (fld, len) {
          if (fld.val().length < len) {
            alert(this.msgChar.replace(/\$1/,fld.name).replace(/\$2/,len));
            fld.focus();
            return true;
          }
          return false;
        },  

Open in new window


fld.name is coming back undefined.  I augmented the code in the following manner to troubleshoot:

validateForm : function(){
          if(this.isBlank(this.username) || this.isTooShort(this.username,4)){
          	alert(this.username.toSource());
            return false;
          }
          if(this.isBlank(this.email) || this.isTooShort(this.email,4)){
          	alert(this.email.toSource());
            return false;
          }
          if(this.isBlank(this.password) || this.isTooShort(this.password,4)){
          	alert(this.password.toSource());
            return false;
          }
          return true;

Open in new window


and along with the message "The undefined field must have at least 4 characters", I then get "({length:1, 0:({}), context:({}), selector:"#username"})"

So that's two alerts in total but I'm not clear on why fld.name would be undefined.
Michel PlungjanIT Expert
Top Expert 2009

Commented:
fld.attr("name") would likely work better than fld.name. Apologies for regressing to normal JS syntax

Can you revert to my code and try that instead?

Author

Commented:
Yes that worked.  What threw me off is that I believe in certain instances, I've been able to use normal js syntax with jQuery and it has worked ... or at least, I think that is the case.

Author

Commented:
regarding this code:

$('#theForm').submit(function(e){
        if(Validation.validateForm()){
          alert(Validation.msgThanks);
          return true;
        }
         e.preventDefault();
        return false;
      });  

Open in new window


unless I've made an error on my end, it's not working.  I click the submit button and none of the js runs at all.
Michel PlungjanIT Expert
Top Expert 2009

Commented:
Jquery allows normal access via "this" instead of $(this) I'll look when I get to a computer

Author

Commented:
Ok, the type was wrong on the submit input.  Once corrected, it submitted the form just fine.

Testing a couple of other things now ...

Author

Commented:
back to this code you rewrote:

$('#theForm').submit(function(e){
        if(Validation.validateForm()){
          alert(Validation.msgThanks);
          return true;
        }
         e.preventDefault();
        return false;
      });

Open in new window


As I said, it works but then I reverted back to my original code:

$('#theForm').submit(function(){
        if(Validation.validateForm()){
          alert(Validation.msgThanks);
       }
        return false;
      });  

Open in new window


and this appears to produce the same end result as your code.  

My question is: what is the advantage of using your code over mine?  Why pass the event and then prevent the default event (rather than using return false)?  My experience with this coding (passing the event, preventing default event) is limited.
Michel PlungjanIT Expert
Top Expert 2009

Commented:
Just me wearing suspenders and a belt. Returning false should be enough

Author

Commented:
HA! alrighty then :)

but my question is: why would someone want to use function(e) and then e.preventDefault(); instead of simply return false; ?
Michel PlungjanIT Expert
Top Expert 2009

Commented:
Because that seems to be the jQuery cross browser way to do it. I'll investigate it for you tomorrow
Bed time here
Michel PlungjanIT Expert
Top Expert 2009

Commented:

Author

Commented:
wow, that looks awesome.  Thanks so much.  I guess I shall cease my interrogation of you on this question :)

Author

Commented:
Thank you so much for your generous help, I learn so much from you.
Michel PlungjanIT Expert
Top Expert 2009

Commented:
You are welcome..

I had to read that article too since I was not 100% sure on the issue so you helped me too :)