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

Validating a form without using a button and changing td class

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

Problem

I need to validate the form inside a table and change the class of a td-element when the value is not set. I don't have a button inside a form and I can't use the jQuery validate script.

I have written this code but is there a way to do this in a simpler way?

I have written many pages with code like this.

```

function richiesti() {

var dati=0;
var cognome=document.clienti.cognome.value;
if ( cognome != '' ){ dati++; } else { document.clienti.cognome.focus(); }
var nome=document.clienti.nome.value;
if ( nome != '' ){ dati++; } else { document.clienti.nome.focus(); }
var codfisc=document.clienti.codfisc.value;
if ( codfisc != '' ){ dati++; } else { document.clienti.codfisc.focus(); }

if ( dati == 3 ){

// Se i tre valori richiesti sono inseriti controllo il codice fiscale

// Definisco un pattern per il confronto
var pattern = /^[a-zA-Z]{6}[0-9]{2}[a-zA-Z][0-9]{2}[a-zA-Z][0-9]{3}[a-zA-Z]$/;

// creo una variabile per richiamare con facilità il nostro campo di input
var CodiceFiscale = document.getElementById("codfisc");

// utilizzo il metodo search per verificare che il valore inserito nel campo
// di input rispetti la stringa di verifica (pattern)
if (CodiceFiscale.value.search(pattern) == -1)
{
// In caso di errore stampo un avviso e pulisco il campo...
alert("Il valore inserito non è un codice fiscale!");
CodiceFiscale.value = "";
CodiceFiscale.focus();
}
else { document.clienti.submit() ; }

}
else { alert('Cognome, Nome e Codice fiscale sono campi obbligatori.');
if ( cognome == '' && nome == '' && codfisc == '' ){
// Cambio la classe del td al valore mancante
document.getElementById('tdcognome').className="tdorange c_white b";
// Imposto la classe degli altri td nel caso sia stata cambiata
document.getElementById('tdcodfisc').className="tdocra c_white b";
document.getElementById('tdnome').className="tdocra c_white b";

Solution

Let's talk about this part:

var cognome=document.clienti.cognome.value;
if ( cognome != '' ){ dati++; } else { document.clienti.cognome.focus(); }


First of all, the indentation is awful. We can either do without braces

if (cognome != '') dati++;
else               document.clienti.cognome.focus();


or preferably, put that conditional onto multiple lines.

if (cognome != ''){
    dati++;
} else {
    document.clienti.cognome.focus();
}


Now, you use both document.clienti.cognome.value and document.clienti.cognome.focus. It would be better to assign document.clienti.cognome to some variable, then:

var cognome = document.clienti.cognome;
if (cognome.value != ''){
    dati++;
} else {
    cognome.focus();
}


This allows us to remove repetition by looping over some values:

var cognome = document.clienti.cognome; 
var nome    = document.clienti.nome;
var codfisc = document.clienti.codfisc;

[cognome, nome, codfisc].forEach(function (field) {
    if (field.value != ''){
        dati++;
    } else {
        field.focus();
    }
});


Later, you test for various combinations of fields being populated and assign classes depending on these combinations. The problem is that the classes do not always depend on all fields, so we could simplify that massive sequence of ifs:

if (cognome.value == '') {
    document.getElementById('tdcognome').className = "tdorange c_white b";
    document.getElementById('tdcodfisc').className = "tdocra c_white b"; 
    document.getElementById('tdnome'   ).className = "tdocra c_white b";
    document.getElementById('cognome').value = "richiesto";
    cognome.focus();
}
else {
    document.getElementById('tdcognome').className="tdocra c_white b";
    if (nome.value == '') {
        document.getElementById('tdcodfisc').className = "tdocra c_white b";
        document.getElementById('tdnome'   ).className = "tdorange c_white b";
        document.getElementById('nome').value = "richiesto";
        nome.focus();
    }
    else {
        if (codfisc.value == '' ) {
            document.getElementById('tdnome'   ).className = "tdocra c_white b";
            document.getElementById('tdcodfisc').className = "tdorange c_white b";
            document.getElementById('codfisc').value = "richiesto";
            codfisc.focus();
        }
    }
}


Oh look, the body is always the same if cognome == ''! Because we now cleaned up the conditions, we can also see that some cases have been missed: what happens if cognome != '' && nome != '' && codfisc != ''? In cases like this it would be helpful to write a comment explaining that no classes will be changed.

There are a few more comments to be made on your style:

-
Avoid non-English comments and variable names. I had difficulty understanding your code because I do not speak Italian. English is spoken by virtually all programmers, so it's a better choice for maintainable code.

-
It seems your indentation has been messed up by copying the code here. However, there are one very bad thing:

if (cond) {
  statement;
  last_statement }


Place the closing curly brace on a line of it's own, this makes it easier to see.

-
You have one variable that's uppercase: CodiceFiscale. Stick to a consistent naming scheme, preferably camelCase.

-
Your code could become much shorter by using jQuery. It's a de-facto standard, and helps abstracting over many compatibility issues.

-
Using == instead of === is usually frowned upon, as == tries to coerce the values to some mathching type. Depending on what you're trying to do, this is actually preferable, but most of the time it's a code smell. The === operator first asserts that both values are of the same type.

Code Snippets

var cognome=document.clienti.cognome.value;
if ( cognome != '' ){ dati++; } else { document.clienti.cognome.focus(); }
if (cognome != '') dati++;
else               document.clienti.cognome.focus();
if (cognome != ''){
    dati++;
} else {
    document.clienti.cognome.focus();
}
var cognome = document.clienti.cognome;
if (cognome.value != ''){
    dati++;
} else {
    cognome.focus();
}
var cognome = document.clienti.cognome; 
var nome    = document.clienti.nome;
var codfisc = document.clienti.codfisc;

[cognome, nome, codfisc].forEach(function (field) {
    if (field.value != ''){
        dati++;
    } else {
        field.focus();
    }
});

Context

StackExchange Code Review Q#40606, answer score: 4

Revisions (0)

No revisions yet.