I was building a QUIZ module for one website. It is a simple TRUE/FALSE statements where you first choose the answer then correct answer is getting displayed and then you can move forward to next question.

For better understanding, please find working code snippet here: http://jsfiddle.net/ylokesh/Qaf8w/50/

I need mentor guidance in order to make my existing code shorter & optimized. Please guide me.

Here is the jQuery code for this QUIZ module.

var flag = $('.quiz ul'),
    currentPosition = 0,
    currentQuestion = 0,
    slideWidth = 200,
    option = $('.flag'),
    tryCTA = $('.CTAtry'),
    nextQuestion = $('.next'),
    answer = $('span.answer')
    questionWrapper = $('.quiz ul'),
    totalQuestions = $('.quiz ul li').length,
    answers = ["True","False","False"];

//Set UL width    
questionWrapper.css('width',totalQuestions*slideWidth)

  option.on('click',function(e) {
      e.preventDefault();
      var nextCTA = $(this).parent().next(nextQuestion).length,
          optionWrapper = $(this).parent();
      if(nextCTA) {
          optionWrapper.next(nextQuestion).show();
      } else {
          tryCTA.show();
      }
      optionWrapper.parent().find(answer).html(answers[currentQuestion]).show();
      optionWrapper.hide();
  });

  nextQuestion.on('click',function(e){
      e.preventDefault();
      currentPosition = currentPosition+1;
      $(this).parent().parent().animate({
          'marginLeft' : slideWidth*(-currentPosition)
      });
      currentQuestion++;
  });

  tryCTA.on('click',function(e) {
      e.preventDefault();
      resetQuiz(e);
      $(this).hide();
  })

  function resetQuiz(e) {
      $('.quiz ul').animate({
           opacity: 0
      },0)
      .animate({
           opacity: 1            
       },500)

      $('span.answer').html(" ");
      $('.quiz ul').css('margin-left','0');
      $('.options').show();    
      $('.next').hide();
      currentPosition = 0;
      currentQuestion = 0;
  }

HTML:

<div class="quiz">
<ul>
    <li>
        <span class="answer"></span>
        <p>Writing Object Oriented JavaScript is not a FUN.</p>
        <div class="options">
            <a href="" class="flag">True</a>
            <span>or</span>
            <a href="" class="flag">False</a>
        </div>                        
        <a href="" class="next">Next</a>
    </li>
    <li>
        <span class="answer"></span>
        <p>Let's give it a try! No harm in learning anything NEW.</p>
        <div class="options">
            <a href="" class="flag">True</a>
            <span>or</span>
            <a href="" class="flag">False</a>
        </div>
        <a href="" class="next">Next</a>
    </li>
    <li>
        <span class="answer"></span>
        <p>Some code running !</p>
        <div class="options">
            <a href="" class="flag">True</a>
            <span>or</span>
            <a href="" class="flag">False</a>
        </div>
    </li>
</ul>
<a href="" class="CTAtry">Try Again!</a>
</div>

UPDATED CODE : as per Pgraham suggestions. Please suggest other optimization steps which you think can be incorporated in this code to make it crisp.

(function ($, undefined) {
    var currentPosition = 0,
        currentQuestion = 0,
        slideWidth = 200
        optionsWrapper = $('.options'),
        tryCTA = $('.CTAtry'),
        nextQuestion = $('.next'),
        answer = $('span.answer')
        questionWrapper = $('.quiz ul'),
        totalQuestions = $('.quiz ul li').length,
        answers = ["False","False","False","True"];

    questionWrapper.width(totalQuestions*slideWidth)


    $('.flag').on('click',function(e) {
        e.preventDefault();
        var nextCTA = $(this).parent().next(nextQuestion).length,
            optionWrapper = $(this).parent(),
            selectedAnswer = $(this).html(),
            actualAnswer = answers[currentQuestion],
            displayOptionText = "";

        if(nextCTA) {
            optionWrapper.next(nextQuestion).show();
        } else {
            tryCTA.show();
        }

        displayOptionText = (selectedAnswer == actualAnswer) ? "Correct" : "Incorrect";
        optionWrapper.hide().parent().find(answer).html(displayOptionText).show();
    });

    nextQuestion.on('click',function(e){
        e.preventDefault();
        currentPosition = currentPosition+1;
        $(this).parent().parent().animate({
            'marginLeft' : slideWidth*(-currentPosition)
        });
        currentQuestion++;
    });

    tryCTA.on('click',function(e) {
        e.preventDefault();
        resetQuiz(e);
        $(this).hide();
    })

    function resetQuiz(e) {
        questionWrapper.animate({ opacity: 0 },0)
                       .animate({ opacity: 1 },500)

        answer.html(" ");
        questionWrapper.css('margin-left','0');
        optionsWrapper.show();    
        nextQuestion .hide();
        currentPosition = 0;
        currentQuestion = 0;
   }

} (jQuery));
share|improve this question

1 Answer

up vote 2 down vote accepted

Overall this is already pretty well organized and compact. There isn't a lot of duplication or obvious abstractions that can be made. It would be helpful to see the HTML to see if there are any changes that would make the Javascript easier to write.

A couple of suggestions:

  1. Wrap the entire code block in an IIFE to avoid cluttering global namespace

    (function ($, undefined) {
        // Your code here ...
    } (jQuery));
    
  2. Use .width() instead of .css('width', ...)

  3. Some of your variables are only being referenced once and can be inlined:

    $('.flag').on('click, ...); // etc.
    
share|improve this answer
About 4, he cannot merge them because the .show() is applied to the "answer" inside the "optionWrapper", and the hide is applied to the "optionWrapper". But yes, the .show() feels redundant. – ANeves Jun 1 '12 at 8:29
@pgraham thanks a lot for your feedback. I will definitely incorporate your points and post the updated code by tomorrow. Only thing where i need some guidance is "Why are we passing 'undefined' with $"?? any specific reason ?? – Lokesh Jun 1 '12 at 10:16
1  
passing undefined and then not providing a value for it is a way of protecting against undefined = 'A defined value'; or similar somewhere in previous code. It is not strictly necessary but ensures that undefined is actually undefined within that block of code. – pgraham Jun 2 '12 at 14:09

Your Answer

 
or
required, but never shown
discard

By posting your answer, you agree to the privacy policy and terms of service.

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