2
\$\begingroup\$

I am using jQuery to implement a basic quiz functionality. How can I improve this code snippet even more? Is there any better way of doing it?

I am using jQuery 1.6.4.

/*jshint -W065 */
$(function () {
    var jQuiz = {
        answers: {
            q1: 'd',
            q2: 'd',
            q3: 'a',
            q4: 'c',
            q5: 'a'
        },
        questionLenght: 5,
        checkAnswers: function () {
            var arr = this.answers;
            var ans = this.userAnswers;
            var resultArr = [];
            for (var p in ans) {
                var x = parseInt(p) + 1;
                var key = 'q' + x;
                var flag = false;
                if (ans[p] == 'q' + x + '-' + arr[key]) {
                    flag = true;
                } else {
                    flag = false;
                }
                resultArr.push(flag);
            }
            return resultArr;
        },
        init: function () {
            $('.btnNext').click(function () {
                if ($('input[type=radio]:checked:visible').length === 0) {

                    return false;
                }
                $(this).parents('.questionContainer').fadeOut(500, function () {
                    $(this).next().fadeIn(500);
                });
                var el = $('#progress');
                el.width(el.width() + 120 + 'px');
            });
            $('.btnPrev').click(function () {
                $(this).parents('.questionContainer').fadeOut(500, function () {
                    $(this).prev().fadeIn(500);
                });
                var el = $('#progress');
                el.width(el.width() - 120 + 'px');
            });
            $('.btnShowResult').click(function () {
                var arr = $('input[type=radio]:checked');
                var ans = jQuiz.userAnswers = [];
                for (var i = 0, ii = arr.length; i < ii; i++) {
                    ans.push(arr[i].getAttribute('id'));
                }
                $('#progress').width(300);
                $('#progressKeeper').hide();
                var results = jQuiz.checkAnswers();
                var resultSet = '';
                var trueCount = 0;
                for (var i = 0, ii = results.length; i < ii; i++) {
                    if (results[i] === true) trueCount++;
                    resultSet += '<div> Question ' + (i + 1) + ' is ' + results[i] + '</div>';
                }
                resultSet += '<div class="totalScore">Your total score is ' + trueCount * 20 + ' / 100</div>';
                $('#resultKeeper').html(resultSet).show();
            });

        }
    };
    jQuiz.init();
});
\$\endgroup\$
  • \$\begingroup\$ Why are you binding two different functions to $('.btnShowResult').click \$\endgroup\$ – Tim Seguine Feb 5 '14 at 10:42
  • \$\begingroup\$ You answered with "what" you are binding. I asked "why". Since they are using the same selector, you could chain the method calls, but you could just as easily put them in the same click handler. If you are worried about readability, factor some code out into well named methods. \$\endgroup\$ – Tim Seguine Feb 5 '14 at 11:12
  • \$\begingroup\$ As a starting point, jslint.com put a comment /*global $ */ at the top of your code, and follow the advice it gives you. \$\endgroup\$ – Tim Seguine Feb 5 '14 at 11:19
  • 1
    \$\begingroup\$ Is your quiz trolling me? Some of the answers are incorrect. \$\endgroup\$ – Tim Seguine Feb 5 '14 at 11:35
  • \$\begingroup\$ And yes of course I could chain the method calls, my mistake. I have updated the code. And yes I have updated the answers too.. :) \$\endgroup\$ – newTag Feb 5 '14 at 11:41
2
\$\begingroup\$

Some suggestions

  • If you ignore the q your questions object is a list of sequential integers so might as well be an array. This will simplify the logic later on.

    answers: [
        'd',
        'd',
        'a',
        'c',
        'a'
    ]
    
  • If you use an array as suggested above, then we can simplify this block quite a lot (also, use strict comparators!):

    var resultArr = [];
            for (var p in ans) {
                var x = parseInt(p) + 1;
                var key = 'q' + x;
                var flag = false;
                if (ans[p] == 'q' + x + '-' + arr[key]) {
                    flag = true;
                } else {
                    flag = false;
                }
                resultArr.push(flag);
            }
    
        var results = [];
        for ( var i = 0; = < ans.length; i++ ) {
            var flag = (ans[p] === arr[i]);
            result.push(flag);
        }
    
  • This is daft unless you want to make certain it's not something other than a boolean;

    if (results[i] === true) {}
    
    if (results[i]) {}
    
  • It's much nicer to build HTML using jQuery than with Strings. And your for loop seems to have an unnecessary variable.

     var resultSet = $('<div>');
     var trueCount = 0;
     for (var i = 0; i < results.length; i++) {
        if (results[i] === true) trueCount++;
        resultSet.text('Question ' + (i + 1) + ' is ' + results[i] + ');
     }
    
  • If you're declaring multiple variables you can separate them with a comma.

    var results = jQuiz.checkAnswers(),
      resultSet = $('<div>'),
      trueCount = 0;
    
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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