HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

JavaScript form validation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
javascriptformvalidation

Problem

Is this a "good" method of form validation?




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_error = false;
form_changed = false;

// Spitzname
$('input[name=edit_spitzname]').change(function() {
if ($('#edit_spitzname').val().length > 0 && $('#edit_spitzname').val().length
.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;
}


Persönliches:

Vorname
Christoph

Nachname
Eder

Spitzname


Geburtstag


Handynummer


Telegram-ID
123456

Wohnort





`

Solution

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.

Code Snippets

$('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();     
});
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_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 ($('#edit_spitzname').val().length > 0 && $('#edit_spitzname').val().length < 2) {
if ($('#edit_spitzname').val().length == 1) {

Context

StackExchange Code Review Q#96646, answer score: 5

Revisions (0)

No revisions yet.