patternjavascriptMinor
Validating a form without using a button and changing td class
Viewed 0 times
withoutvalidatingusingbuttonandformclasschanging
Problem
I need to validate the form inside a table and change the class of a
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";
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:
First of all, the indentation is awful. We can either do without braces
or preferably, put that conditional onto multiple lines.
Now, you use both
This allows us to remove repetition by looping over some values:
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
Oh look, the body is always the same if
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:
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:
-
Your code could become much shorter by using jQuery. It's a de-facto standard, and helps abstracting over many compatibility issues.
-
Using
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.