patternjavascriptModerate
What do you think about my questionnaire?
Viewed 0 times
questionnaireyouwhataboutthink
Problem
This time I'm not here to ask for help with my code, but to ask you to judge my code. Where can I improve? What should I do better? Where did I do things wrong?
This is a quiz that I had to do as an assignment today. I'm not a professional. I'm still studying, and at the moment, I'm doing an internship.
```
-->
Sei ...?
YES
NO
Sai ...?
YES
NO
Ricerchi...?
YES
NO
Finita la spesa, tieni sempre gli scontrini?
YES
NO
Hai già pensato ..?
YES
NO
Cerchi di...?
YES
NO
La tua ...?
YES
NO
var risposte = new Object();
var $domande = $('.domande');
$domande.hide();
var totDomande = $('.domande').size();
var domandaOn = 0;
var risultato = "";
var countRisposte = 0;
$($domande.get(domandaOn)).fadeIn();
console.log(domandaOn);
$('.option').click(function(){
countRisposte = countRisposte+1;
console.log(countRisposte);
var risposta = ($(this).attr('value'));
var domanda = ($(this).attr('name'));
if(risposta == "yes"){
risultato += "1";
}
if(risposta == "no"){
risultato += "0";
}
$($domande.get(domandaOn)).fadeOut(function(){
if(domandaOn == 0){
if(risposta == "yes"){
domandaOn = 4;
}else if(risposta == "no"){
domandaOn = 1;
}
}
if((domandaOn == 4 || domandaOn == 1) && countRisposte >1){
if(risposta == "yes"){
domandaOn = domandaOn+2;
}else{
domandaOn = domandaOn+1;
}
This is a quiz that I had to do as an assignment today. I'm not a professional. I'm still studying, and at the moment, I'm doing an internship.
```
-->
Sei ...?
YES
NO
Sai ...?
YES
NO
Ricerchi...?
YES
NO
Finita la spesa, tieni sempre gli scontrini?
YES
NO
Hai già pensato ..?
YES
NO
Cerchi di...?
YES
NO
La tua ...?
YES
NO
var risposte = new Object();
var $domande = $('.domande');
$domande.hide();
var totDomande = $('.domande').size();
var domandaOn = 0;
var risultato = "";
var countRisposte = 0;
$($domande.get(domandaOn)).fadeIn();
console.log(domandaOn);
$('.option').click(function(){
countRisposte = countRisposte+1;
console.log(countRisposte);
var risposta = ($(this).attr('value'));
var domanda = ($(this).attr('name'));
if(risposta == "yes"){
risultato += "1";
}
if(risposta == "no"){
risultato += "0";
}
$($domande.get(domandaOn)).fadeOut(function(){
if(domandaOn == 0){
if(risposta == "yes"){
domandaOn = 4;
}else if(risposta == "no"){
domandaOn = 1;
}
}
if((domandaOn == 4 || domandaOn == 1) && countRisposte >1){
if(risposta == "yes"){
domandaOn = domandaOn+2;
}else{
domandaOn = domandaOn+1;
}
Solution
For your convenience, here's a jsFiddle of your original code.
User Experience
The user cannot easily amend an accidentally incorrect response, because the question fades away as soon as one clicks "YES" or "NO". Usually, such user-unfriendliness is not desirable.
Language
I'm personally fine with you coding in Italian. However, I find this flavour of inglesiano disturbing. For example,
PHP
You called
Also, always use PHP long tags (`
// Answers to each question add these values to resFinale
var valore = {
'domanda0' : { no: 0, yes: 4 },
'domanda1' : { no: 0, yes: 2 },
'domanda1_1' : { no: 0, yes: 1 },
'domanda1_2' : { no: 0, yes: 1 },
'domanda2' : { no: 0, yes: 2 },
'domanda2_1' : { no: 0, yes: 1 },
'domanda2_2' : { no: 0, yes: 1 },
};
// The next question to present after each response
var FINE = null;
var domandeSeguenti = {
'domanda0' : { no: 'domanda1', yes: 'domanda2' },
'domanda1' : { no: 'domanda1_1', yes: 'domanda1_2' },
'domanda1_1' : { no: FINE, yes: FINE },
'domanda1_2' : { no: FINE, yes: FINE },
'domanda2' : { no: 'domanda2_1', yes: 'domanda2_2' },
'domanda2_1' : { no: FINE, yes: FINE },
'domanda2_2' : { no: FINE, yes: FINE },
};
// Show just the first question
$('.domande').hide();
$('#domanda0').fadeIn();
var risultato = 1;
$('.option').click(function(){
var risposta = $(this).attr('value');
var domanda = $(this).attr('name');
risultato += valore[domanda][risposta];
$('#' + domanda).fadeOut(function(){
var domandaSeguente = domandeSeguenti[domanda][risposta];
if (domandaSeguente == FINE) {
var resFinale = 'result' + risultato;
alert("risultato : " + resFinale);
} else {
$('#' + domandaSeguente).
User Experience
The user cannot easily amend an accidentally incorrect response, because the question fades away as soon as one clicks "YES" or "NO". Usually, such user-unfriendliness is not desirable.
Language
I'm personally fine with you coding in Italian. However, I find this flavour of inglesiano disturbing. For example,
countRisposte, domandaOn, resFinale are all weird mixtures of languages. Another programmer maintaining code like that would be likely to make a spelling mistake because of such odd words.PHP
You called
session_start(), so it looks like you are using PHP. In that case, why not also use PHP to render your questions with less redundancy? "Sei ...?",
'domanda1' => "Sai ...?",
'domanda1_1' => "Ricerchi...?",
'domanda1_2' => "Finita la spesa, tieni sempre gli scontrini?",
'domanda2' => "Hai già pensato ..?",
'domanda2_1' => "Cerchi di...?",
'domanda2_2' => "La tua ...?",
);
foreach ($domande as $nome => $domanda) {
?>
">
" value="yes">YES
" value="no">NO
Also, always use PHP long tags (`
); short tags () are officially discouraged in the PHP manual.
Note that I added the id attribute to the outer . I'll use that later in the solution below.
JavaScript inclusion
If you omit the http: from the URL, like this:
then your page will work equally well whether it is served using HTTP or HTTPS.
Calculation of risultato and resFinale
The way you assemble risultato
if (risposta == "yes") { risultato += "1"; }
if (risposta == "no") { risultato += "0"; }
… seems rather fragile to me, because you would only arrive at the correct result if those statements are executed in the expected sequence.
You use a long switch block to convert risultato to resFinale. It might be clearer and shorter as:
// Map risultato to resFinale.
// risultato = "000" -> resFinale = "result1"
// risultato = "001" -> resFinale = "result2"
// (etc.)
// risultato = "111" -> resFinale = "result8"
if (/^[01][01][01]$/.test(risultato)) {
resFinale = 'result' + (1 + parseInt(risultato, 2));
}
… but see the solution below for an even better approach.
Question Dependencies
The code related to domandaOn is very hard to follow. The magic values you use for domandaOn don't correspond to the names of the questions, but to their index in the DOM. The whole mechanism is fragile. (Have fun modifying the code if you ever need to insert or remove questions!)
What is particularly bad is that your if-blocks are not mutually exclusive. For example, if domandaOn == 4 and risposta == 'no' (i.e., you answer "NO" to "Hai già pensato ..?", also known as "domanda2"), you first assign domandaOn = domandaOn+1, which cascades to another assignment domandaOn = domandaOn+1 because domandaOn == 5 and countRisposte == 2. I'm pretty sure it's a bug, because your "Ricerchi...?" and "Cerchi di...?" questions are unreachable. In any case, it's nasty code that should be rethought rather than fixed.
Usage of variables
You never use risposte and totDomande.
The outer parentheses are superfluous:
var risposta = ($(this).attr('value'));
var domanda = ($(this).attr('name'));
Interestingly, while you use risposta, you never use domanda. The code could be a lot more robust and understandable if you used domanda.
Solution
Putting all these ideas together and more, I would write the JavaScript code this way:
``// Answers to each question add these values to resFinale
var valore = {
'domanda0' : { no: 0, yes: 4 },
'domanda1' : { no: 0, yes: 2 },
'domanda1_1' : { no: 0, yes: 1 },
'domanda1_2' : { no: 0, yes: 1 },
'domanda2' : { no: 0, yes: 2 },
'domanda2_1' : { no: 0, yes: 1 },
'domanda2_2' : { no: 0, yes: 1 },
};
// The next question to present after each response
var FINE = null;
var domandeSeguenti = {
'domanda0' : { no: 'domanda1', yes: 'domanda2' },
'domanda1' : { no: 'domanda1_1', yes: 'domanda1_2' },
'domanda1_1' : { no: FINE, yes: FINE },
'domanda1_2' : { no: FINE, yes: FINE },
'domanda2' : { no: 'domanda2_1', yes: 'domanda2_2' },
'domanda2_1' : { no: FINE, yes: FINE },
'domanda2_2' : { no: FINE, yes: FINE },
};
// Show just the first question
$('.domande').hide();
$('#domanda0').fadeIn();
var risultato = 1;
$('.option').click(function(){
var risposta = $(this).attr('value');
var domanda = $(this).attr('name');
risultato += valore[domanda][risposta];
$('#' + domanda).fadeOut(function(){
var domandaSeguente = domandeSeguenti[domanda][risposta];
if (domandaSeguente == FINE) {
var resFinale = 'result' + risultato;
alert("risultato : " + resFinale);
} else {
$('#' + domandaSeguente).
Code Snippets
<?php
$domande = array(
'domanda0' => "Sei ...?",
'domanda1' => "Sai ...?",
'domanda1_1' => "Ricerchi...?",
'domanda1_2' => "Finita la spesa, tieni sempre gli scontrini?",
'domanda2' => "Hai già pensato ..?",
'domanda2_1' => "Cerchi di...?",
'domanda2_2' => "La tua ...?",
);
foreach ($domande as $nome => $domanda) {
?>
<div class="domande" id="<?php echo $nome ?>">
<p><?php echo htmlspecialchars($domanda); ?></p>
<div class="option" name="<?php echo $nome; ?>" value="yes"><button>YES</button></div>
<div class="option" name="<?php echo $nome; ?>" value="no"><button>NO</button></div>
</div>
<?php
}
?><script src="//ajax.googleapis.com/ajax/libs/jquery/1.10.2/jquery.min.js"></script>if (risposta == "yes") { risultato += "1"; }
if (risposta == "no") { risultato += "0"; }// Map risultato to resFinale.
// risultato = "000" -> resFinale = "result1"
// risultato = "001" -> resFinale = "result2"
// (etc.)
// risultato = "111" -> resFinale = "result8"
if (/^[01][01][01]$/.test(risultato)) {
resFinale = 'result' + (1 + parseInt(risultato, 2));
}var risposta = ($(this).attr('value'));
var domanda = ($(this).attr('name'));Context
StackExchange Code Review Q#32425, answer score: 18
Revisions (0)
No revisions yet.