What do you think about my questionnaire?

Question

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.

<? session_start(); ?>
<html>
<head>
    <script src="http://ajax.googleapis.com/ajax/libs/jquery/1.10.2/jquery.min.js"></script>
    <!--<script type="text/javascript" src="quiz.js" /> -->

</head>
<body>

    <div class="domande">
    <p>Sei ...?
</p>
        <div class="option" value="yes" name="domanda0"><button>YES</button></div>
        <div class="option" value="no" name="domanda0"><button>NO</button></div>
    </div>

    <div class="domande">
    <p>Sai ...?</p>
        <div class="option" value="yes" name="domanda1"><button>YES</button></div>
        <div class="option" value="no" name="domanda1"><button>NO</button></div>
    </div>
    <div class="domande">
    <p>Ricerchi...?</p>
        <div class="option" value="yes" name="domanda1_1"><button>YES</button></div>
        <div class="option" value="no" name="domanda1_1"><button>NO</button></div>
    </div>
    <div class="domande">
       <p>Finita la spesa, tieni sempre gli scontrini?</p>
        <div class="option" value="yes" name="domanda1_2"><button>YES</button></div>
        <div class="option" value="no" name="domanda1_2"><button>NO</button></div>
    </div>

    <!-- domanda 2 -->
    <div class="domande">
    <p>Hai già pensato ..?</p>
        <div class="option" value="yes" name="domanda2"><button>YES</button></div>
        <div class="option" value="no" name="domanda2"><button>NO</button></div>
    </div>
    <div class="domande">
    <p>Cerchi di...?</p>
        <div class="option" value="yes" name="domanda2_1"><button>YES</button></div>
        <div class="option" value="no" name="domanda2_1"><button>NO</button></div>
    </div>
    <div class="domande">
    <p>La tua ...?</p>
        <div class="option" value="yes" name="domanda2_2"><button>YES</button></div>
        <div class="option" value="no" name="domanda2_2"><button>NO</button></div>
    </div>


<script>
        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;
                    }
                }
                if((domandaOn == 2 || domandaOn == 5) && countRisposte <3){
                    domandaOn = domandaOn+1;
                }

                if((domandaOn == 3 || domandaOn == 6) && countRisposte > 2){
                    console.log(domandaOn);
                    var resFinale = "";
                    console.log("risultato into 6: " +risultato);
                    switch (risultato)
                    {
                    case "000":
                        resFinale = "result1";
                      break;
                    case "001":
                        resFinale = "result2";
                      break;
                    case "010":
                        resFinale = "result3";
                      break;
                    case "011":
                        resFinale = "result4";
                      break;
                    case "100":
                        resFinale = "result5";
                      break;
                    case "101":
                        resFinale = "result6";
                      break;
                    case "110":
                        resFinale = "result7";
                      break;
                    case "111":
                        resFinale = "result8";
                      break;
                    }
                    console.log(resFinale);
                    alert("risultato : "+resFinale);
                }else{
                    $($domande.get(domandaOn)).fadeIn();                    
                }
            });

        });
</script>
</body>


</html>

Uh, well english variable names would be a good start. You won't see me (german) writing code like gericht.beilage(new Sauerkraut()); gericht.beilage(new Knödel()); if (!achtung) alert("Es gibt " + gericht.bezeichnung);. In other words, I have no idea what your code is supposed to do. — amon, Commented Oct 8, 2013 at 17:02
I'm Italian but I never use Italian names for variables. It makes harder to share code. Next time either ask help on an Italian forum, or write in English. I'd also suggest to avoid Italian in the strings. There are tools such as gettext et similia to deal with i18n and l10n. — Bakuriu, Commented Oct 9, 2013 at 6:08

200_success · Accepted Answer · 2013-10-09 07:32:07Z

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?

<?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
    }
?>

Also, always use PHP long tags (<?php ... ?>); short tags (<? ... ?>) are officially discouraged in the PHP manual.

Note that I added the id attribute to the outer <div>. I'll use that later in the solution below.

JavaScript inclusion

If you omit the http: from the URL, like this:

<script src="//ajax.googleapis.com/ajax/libs/jquery/1.10.2/jquery.min.js"></script>

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).fadeIn();
        }
    });

});

I've fixed your unreachable-questions bug in the way I think you intended, so it does not behave like the original. Here's a jsFiddle of my solution.

I would go one step further and rename domanda0, domanda1, etc. to something more descriptive, but I have refrained from doing so in this solution because it would be hard to compare it against the original code.

Summary

The code is complex, and needs to be simplified. Instead of hard-coding all the logic, you want to generalize, such that the application is data-driven. That way, the questionnaire is largely defined by the data structures, and the code is elegant.

Thanks so much. The main problem i've is to structure the logic before start programming. I coded the thing you read above without structuring the flow before. Today i made some changes before reading your answer but your code is of course better. I appreciate what you did for me. Thanks a lot! I hope to do better next time :) — Stefano Cerelli, Commented Oct 9, 2013 at 9:28

Community · Accepted Answer · 2017-05-23 12:41:06Z

You should get into the habit of using English variable names and identifiers. English is the de facto standard language of programming and in most environments there will be people from different nationalities working with the code, and everyone must understand what the names mean. (There's always someone who looks at the code who doesn't understand Italian, if no-one else then people you ask help from on Stack Overflow...)

var risposte = new Object();

You should use {} instead of new Object().

var totDomande = $('.domande').size();

.size() is deprecated, you should use .length instead.

$($domande.get(domandaOn)).fadeIn();

It would be cleaner to use $domande.eq( domandaOn ).fadeIn() which does the same thing, although you can just reduce that to $domande.eq( 0 ).fadeIn() or $domande.first().fadeIn() since the value of domandaOn is guaranteed to be 0 at that point.

countRisposte = countRisposte+1;

Incrementing by one is usually done with countRisposte++;

var risposta = ($(this).attr('value'));
var domanda = ($(this).attr('name'));

The outer parentheses are unnecessary here.

if(risposta == "yes"){
    risultato += "1";
}
if(risposta == "no"){
    risultato += "0";
}

I see you're building a kind of binary string from the answers; this kinda works when you have very few questions like the three here, but it's not very flexible or maintainable. You might want to consider storing the answers in an object, along with the questionnaire logic, but going into details might be beyond the scope of this answer (perhaps someone else can come up with something).

It's also a good habit to use strict comparison (===) when the type of the variable is known, e.g. if(risposta === "yes"){.

if((domandaOn == 4 || domandaOn == 1) && countRisposte >1){

You could shorten that to if( domandaOn % 3 === 1 && countRisposte > 1 ) although it might sacrifice legibility (the modulo operator evaluates to the remainder after dividing by 3).

I'm not sure why the countRisposte checks are necessary since there doesn't seem to be other than one path through the questionnaire, and the binary-string-mechanism would break if there are more than three answers.

switch (risultato)
{
case "000":
    resFinale = "result1";
  break;
case "001":
    ...

Instead of this big switch you could save the results in an object:

var results = {
    "000": "result1",
    "001": "result2",
    "010": "result3",
        // ...and so on
};

resFinale = results[ risultato ];

(I'm assuming there will be some actual text there instead of just "resultX".)

While the use of English in identifiers is often a practical choice, you should remember that the vast majority of people on Earth do not understand English, or understand it very little. There are large communities of programmers that use their own language. Besides, using any other language but English makes it more obvious which words are reserved words or predefined or library names as opposite to identifiers chosen by the programmer. — Jukka K. Korpela, Commented Oct 8, 2013 at 17:20
I'm French-speaking and it KILLS me when I see identifiers in French in someone else's code I have to maintain. Having them in English blends with the language and makes a coherent whole, as opposed to a soup of mixed-up languages. I maintain a code base with mixed English, French and Spanish and it's a nightmare. +1 for keeping the code sane and readable, and for an excellent code review that doesn't deserve a downvote just for the first couple words. — Mathieu Guindon, Commented Oct 8, 2013 at 19:00
Once upon a time, I used to mix Portuguese, Spanish and English on my code. Surely enough: "um, what was that var name again? Was it a Z or a S or a double S?". . . Sidenote, people who don't speak English doesn't use Code Review - @JukkaK.Korpela — brasofilo, Commented Oct 8, 2013 at 23:09

Jukka K. Korpela · Accepted Answer · 2013-10-08 17:27:16Z

3

By looking at the code, it is not obvious at all what the code is supposed to do (and I do understand Italian sufficiently to undertand the texts and names). You should explain the purpose and logic in prose, or in comments, or both. Primarily, the HTML markup should make it more apparent what is going on, using headings (with appropriate markup), descriptive labels, and other structural elements.

answered Oct 8, 2013 at 17:27

Jukka K. Korpela

2381 silver badge4 bronze badges

Add a comment |

Stack Exchange Network

What do you think about my questionnaire?

3 Answers 3

User Experience

Language

PHP

JavaScript inclusion

Calculation of `risultato` and `resFinale`

Question Dependencies

Usage of variables

Solution

Summary

Your Answer

Not the answer you're looking for? Browse other questions tagged
javascript
jquery
html
or ask your own question.

Hot Network Questions

What do you think about my questionnaire?

3 Answers 3

User Experience

Language

PHP

JavaScript inclusion

Calculation of risultato and resFinale

Question Dependencies

Usage of variables

Solution

Summary

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged javascriptjqueryhtml or ask your own question.

Related

Hot Network Questions

Calculation of `risultato` and `resFinale`

Not the answer you're looking for? Browse other questions tagged
javascript
jquery
html
or ask your own question.