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

What do you think about my questionnaire?

Submitted by: @import:stackexchange-codereview··
0
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;
}

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, 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.