3
\$\begingroup\$

Is this a "good" method of form validation?

<!-- Originalwerte -->
			ori_edit_spitzname = $('#edit_spitzname').val();
			ori_edit_gebtag = $('#edit_gebtag').val();
			ori_edit_handy = $('#edit_handy').val();
			ori_edit_herkunft = $('#edit_herkunft').val();
		
			<!-- FORM PRÜFEN -->
			form_error = false;
			form_changed = false;
	
						// Spitzname 
						$('input[name=edit_spitzname]').change(function() {  
								if ($('#edit_spitzname').val().length > 0 && $('#edit_spitzname').val().length < 2) {
								alert("Spitzname muss leer, oder minimal 2 Buchstaben lang sein!");	
								$('#edit_spitzname').css({'color': 'red', 'border-color' : 'red'});
								$('#edit_spitzname').focus();
								form_error = true;
								} else {
								$('#edit_spitzname').css({'color': 'green', 'border-color' : 'green'});
								form_error = false;
								}
							is_changed_general();		
						 });
						 
						 // Geburtstag
						$('input[name=edit_gebtag]').change(function() {  
						  if (!validatebirthday($('#edit_gebtag').val(), ".")) {
								alert("Falsches Geburtstagsformat!\nDD.MM.YYYY");	
								$('#edit_gebtag').css({'color': 'red', 'border-color' : 'red'});
								$('#edit_gebtag').focus();
								form_error = true;
						} else {
								$('#edit_gebtag').css({'color': 'green', 'border-color' : 'green'});
								form_error = false;	}
							is_changed_general();		
						 });
						 
						  // Handynummer
						 $('input[name=edit_handy]').change(function() {  
						 alert($('#edit_handy').val().substr(0, 2));
						  if ($('#edit_handy').val().substr(0, 2)!="43") {
								alert("Handynummer MUSS mit 43 beginnen!\n43660123456");	
								$('#edit_handy').css({'color': 'red', 'border-color' : 'red'});
								$('#edit_handy').focus();
								form_error = true;
						} else {
								$('#edit_handy').css({'color': 'green', 'border-color' : 'green'});
								form_error = false;	}
							is_changed_general();		
						 });

						// Herkunft 
						$('input[name=edit_herkunft]').change(function() {  
								if ($('#edit_herkunft').val().length<4) {
								alert("Ein Herkunftsort muss angegeben werden! (Min 3 Buchstaben!)");	
								$('#edit_herkunft').css({'color': 'red', 'border-color' : 'red'});
								$('#edit_herkunft').focus();
								form_error = true;
								} else {
								$('#edit_herkunft').css({'color': 'green', 'border-color' : 'green'});
								form_error = false;
								}
							is_changed_general();		
						 });
						
						// Speichern-Button nur anzeigen wenn änderung und kein Fehler!
						function is_changed_general() {
						if (!form_error) {
							
							var general_changed = false; 
							
						if (ori_edit_spitzname!=$('#edit_spitzname').val()) { general_changed = true;  } 
							else { $('#edit_spitzname').css({'color': 'black', 'border-color' : '#404040'}); }
						if (ori_edit_gebtag!=$('#edit_gebtag').val()) { general_changed = true;  } 
							else { $('#edit_gebtag').css({'color': 'black', 'border-color' : '#404040'}); }
						if (ori_edit_handy!=$('#edit_handy').val()) { general_changed = true;  } 
							else { $('#edit_handy').css({'color': 'black', 'border-color' : '#404040'}); }
						if (ori_edit_herkunft!=$('#edit_herkunft').val()) { general_changed = true;  } 
							else { $('#edit_herkunft').css({'color': 'black', 'border-color' : '#404040'}); }
							
							if (general_changed) {
							// EINBLENDEN
							$('#gen_sendbutton').css('display', 'block');
							} else {
							// AUSBLENDEN
							$('#gen_sendbutton').css('display', 'none');
							}
						
						} else {
							// Ausblenden
							$('#gen_sendbutton').css('display', 'none');

						}
							general_changed = false;
						}
	
				// Wenn kein Fehler, Formular absenden
				function send_edit_general() {
						if (form_error==true) { alert("Bitte alle Eingaben prüfen!"); } else {
						alert("Daten würden jetzt gespeichert werden, alle Eingaben OK !");	
						}
				}
.memb_col {
	width:550px;
}

.memb_col_0 {
	width:550px;
	float:left;
	font-size:1.2em;
	font-style:italic;
}

.memb_col_0_1 {
	width:550px;
	float:left;
	padding-top: 10px;
}

.memb_col_1 {
	width:140px;
	float:left;
	padding-top: 6px;
	font-weight:bold;

}

.memb_col_2 {
	width:410px;
	float:left;
	padding-top: 6px;
}
<script src="https://ajax.googleapis.com/ajax/libs/jqueryui/1.11.4/jquery-ui.min.js"></script>
<link href="https://ajax.googleapis.com/ajax/libs/jqueryui/1.11.4/themes/smoothness/jquery-ui.css" rel="stylesheet"/>
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script>


<div class="memb_col clearfix">
                    <div class="memb_col_0">Persönliches:<br></div>

                    <div class="memb_col_1">Vorname</div>
                    <div class="memb_col_2">Christoph</div>
                    
                    <div class="memb_col_1">Nachname</div>
                    <div class="memb_col_2">Eder</div>
 
                    <div class="memb_col_1">Spitzname</div>
                    <div class="memb_col_2"><input type="text" name="edit_spitzname" id="edit_spitzname" value="" class="ui-widget ui-widget-content ui-corner-all"></div>
                    
                    <div class="memb_col_1">Geburtstag</div>
                    <div class="memb_col_2"><input type="text" name="edit_gebtag" id="edit_gebtag" value="04.05.1990" class="ui-widget ui-widget-content ui-corner-all hasDatepicker"></div>
                    
                    <div class="memb_col_1">Handynummer</div>
                    <div class="memb_col_2"><input type="text" name="edit_handy" id="edit_handy" value="4366123456789" class="ui-widget ui-widget-content ui-corner-all"></div>
                    
                    <div class="memb_col_1">Telegram-ID</div>
                    <div class="memb_col_2">123456</div>
                    
                    <div class="memb_col_1">Wohnort</div>
                    <div class="memb_col_2"><input type="text" name="edit_herkunft" id="edit_herkunft" value="Example City" class="ui-widget ui-widget-content ui-corner-all"></div>
                    
                                     
                    <div class="memb_col_0"><br><input type="button" style="display:none;" id="gen_sendbutton" onclick="send_edit_general()" value="Änderungen speichern #dummy" class="ui-widget ui-widget-content ui-corner-all"></div>
				</div>

\$\endgroup\$
  • \$\begingroup\$ Cross-posted from Stack Overflow (where the question was closed). \$\endgroup\$ – 200_success Jul 12 '15 at 8:20
  • \$\begingroup\$ Yes, cross posted BECAUSE it was closed! \$\endgroup\$ – C.E. Jul 13 '15 at 22:00
  • \$\begingroup\$ No need to be upset. There's nothing wrong with cross-posting, as long as you declare it properly to avoid wasting other people's time. \$\endgroup\$ – 200_success Jul 13 '15 at 22:01
5
\$\begingroup\$

Formatting

First of all with the excessive whitespace at the beginning of lines, and the indenting that seems to follow no logic, this is extremely difficult to read. For example, this would be so much better:

$('input[name=edit_spitzname]').change(function() {  
    if ($('#edit_spitzname').val().length > 0 && $('#edit_spitzname').val().length < 2) {
        alert("Spitzname muss leer, oder minimal 2 Buchstaben lang sein!");   
        $('#edit_spitzname').css({'color': 'red', 'border-color' : 'red'});
        $('#edit_spitzname').focus();
        form_error = true;
    } else {
        $('#edit_spitzname').css({'color': 'green', 'border-color' : 'green'});
        form_error = false;
    }
    is_changed_general();     
});

Another awful formatting is here:

    if (ori_edit_spitzname!=$('#edit_spitzname').val()) { general_changed = true;  } 
        else { $('#edit_spitzname').css({'color': 'black', 'border-color' : '#404040'}); }
    if (ori_edit_gebtag!=$('#edit_gebtag').val()) { general_changed = true;  } 
        else { $('#edit_gebtag').css({'color': 'black', 'border-color' : '#404040'}); }

It would be so much easier to read like this:

if (ori_edit_spitzname != $('#edit_spitzname').val()) {
    general_changed = true;
} else {
    $('#edit_spitzname').css({'color': 'black', 'border-color' : '#404040'});
}
if (ori_edit_gebtag != $('#edit_gebtag').val()) {
    general_changed = true;  
} else {
    $('#edit_gebtag').css({'color': 'black', 'border-color' : '#404040'});
}

Simplify

This condition can be simplified:

if ($('#edit_spitzname').val().length > 0 && $('#edit_spitzname').val().length < 2) {

Because length greater than 0 and less than 2 is simply length that is exactly 1:

if ($('#edit_spitzname').val().length == 1) {

Avoid repeated dom lookups

Dom lookups like $('#edit_spitzname') are expensive, and it's good to cache their result and reuse. So instead of this:

if ($('#edit_spitzname').val().length > 0 && $('#edit_spitzname').val().length < 2) {
    alert("Spitzname muss leer, oder minimal 2 Buchstaben lang sein!");   
    $('#edit_spitzname').css({'color': 'red', 'border-color' : 'red'});
    $('#edit_spitzname').focus();

This is recommended:

var edit_spitzname = $('#edit_spitzname');
if (edit_spitzname.val().length > 0 && edit_spitzname.val().length < 2) {
    alert("Spitzname muss leer, oder minimal 2 Buchstaben lang sein!"); 
    edit_spitzname.css({'color': 'red', 'border-color' : 'red'});
    edit_spitzname.focus();

Use boolean expressions directly

Instead of:

if (form_error==true) {

You can use boolean expressions directly, and simply write:

if (form_error) {

Use style sheets

Instead of this:

    $('#edit_spitzname').css({'color': 'black', 'border-color' : '#404040'});
    // ...
    $('#edit_gebtag').css({'color': 'black', 'border-color' : '#404040'});

It would be better to use a CSS class that applies these styles.

\$\endgroup\$
2
\$\begingroup\$

There is a lot of repetitive code. For each field, the jQuery selector, the test, and the error message vary. However, the code to trigger the check, change the colors, set the focus, and note the form state are just cut-and-paste.

If the form has multiple errors, fixing any one field clears the universal form_error flag.

For these and other reasons, I recommend that you take the suggestion on your Stack Overflow question, and use a jQuery validation library instead of reinventing the wheel.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer! I found the "multiple-error"-problem short after posting. Fixed it with explicit error_vars for each field (fixed amount of fields in this form). Repetitive Code was the reason, why i ask on stackoverflow for better solutions. I'll try to use the validation libary for my next form. \$\endgroup\$ – C.E. Jul 12 '15 at 9:10

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.