Deeper abstracting of code into functions

Question

I created a quiz using the module pattern. My code is as follows:

; (function ($) {
"use strict";

var s,
    QuizDefaults = {
        modalOverlay: $(".modal-overlay"),
        modalWrap: $(".js-modal-container"),
        timer: null,             // setInterval var for timer
        totalSeconds: 10,        // Amount of seconds for timer
        count: null,             // Current location of timer
        answerCorrect: null,     // Set to true on correct answer, false incorrect
        score: 0,                // Score counter
        questionCounter: 1,      // store which question user is currently on
        questionText: "quickly click on the correct answer above!", // text below multiple answers
        frameCur: null           // store the current position in quiz
    },
    QuizApp = {
        settings: $.extend({}, QuizDefaults), // copy default settings 

        // Kick it off
        // Kick it off
        init: function () {
            s = $.extend({}, QuizDefaults);
            this.questionsGet();
            this.bindUIActions();
        },

        // User interaction events
        bindUIActions: function () {
            // Open modal trigger
            $(".js-quiz-modal-trigger").on("click", function () {
                QuizApp.toggleModal(s.modalWrap, s.modalOverlay);
                QuizApp.stateInactive();
            });
            // Close modal trigger
            $(".js-quiz-modal-close, .js-quiz-button-finish").on("click", function () {
                QuizApp.timerReset();
                QuizApp.toggleModal(s.modalWrap, s.modalOverlay);
                QuizApp.restartQuiz();
            });
            // Begin quiz and timer trigger
            $(".js-quiz-button-begin").on("click", function () {
                QuizApp.frameNext($(".js-quiz-body-question1"));
                QuizApp.stateActive();
                QuizApp.timerReset();
                QuizApp.timerStart();
            });
            // Help trigger
            $(".js-quiz-help").on("click", function () {
                QuizApp.helpToggle();
            });
            // Selected answer check trigger
            $(".quiz-body").on("click", ".js-quiz-answer a", function () {
                QuizApp.answerCheck($(this));
            });
            // Move to next question trigger
            $(".js-quiz-button-continue").on("click", function () {
                QuizApp.questionNext();
            });
            // Restart quiz trigger
            $(".js-quiz-button-again").on("click", function () {
                QuizApp.restartQuiz();
                QuizApp.stateInactive();
            });
        },

        // Pop up or hide Modal
        toggleModal: function (modal, overlay) {
            modal.toggleClass("is-open");
            this.centerModal();
            if (modal.hasClass("is-open")) {
                $(window).bind("resize", this.centerModal);
                modal.add(overlay).fadeIn(100);
            } else {
                modal.add(overlay).fadeOut(100);
                $(window).unbind("resize");

            }
        },

        // center modal
        centerModal: function () {
            var top, left;

            top = Math.max($(window).height() - s.modalWrap.outerHeight(), 0) / 2;
            left = Math.max($(window).width() - s.modalWrap.outerWidth(), 0) / 2;

            s.modalWrap.css({
                top: top + $(window).scrollTop(),
                left: left + $(window).scrollLeft()
            });
        },

        // Start timer
        timerStart: function () {
            this.timerDisplay();
            var ic = $(".js-quiz-body-incorrect");
            if (s.count === 0) {
                ic.addClass("is-expired");
                this.frameNext(ic);
            } else {
                s.count -= 1;
                s.timer = setTimeout(function () {
                    QuizApp.timerStart();
                }, 1000);
            }
        },

        // Stop timer
        timerPause: function () {
            clearInterval(s.timer);
        },

        // resets countdown
        timerReset: function () {
            this.timerPause();
            s.count = s.totalSeconds;
            this.timerDisplay();
        },

        // prints timer to page
        timerDisplay: function () {
            $(".js-numeric-timer-ticker").text(s.count);
            $(".js-pie-timer div").attr("class", "pie-timer-" + s.count);
        },

        // shows tooltips
        helpToggle: function () {
            var helpTip = $(".tip");
            if ($('.js-quiz-header').hasClass("is-overlay")) {
                $('.js-quiz-header').removeClass("is-overlay");
                this.frameNext(s.frameCur);
                helpTip.fadeOut(100);
                if ($(s.frameCur).is($("[class*=' js-quiz-body-question']")) && s.count > 0) {
                    this.timerStart();
                } else if ($(s.frameCur).is($("[class*=' js-quiz-body-question']"))){
                    var ic = $(".js-quiz-body-incorrect");
                    ic.addClass("is-expired");
                    this.frameNext(ic);
                    this.timerDisplay();
                }
            } else {
                $('.js-quiz-header').delay(100).queue(function (next) {
                    $(this).addClass("is-overlay");
                    next();
                });
                this.frameNext($(".js-quiz-body-default-question"));
                helpTip.delay(100).fadeIn(100);
                this.timerPause();
                this.frameGet();
            }
        },

        // Faded out elements state
        stateInactive : function () {
            $(".js-quiz-header").addClass("is-inactive");
        },

        stateActive : function () {
            $(".js-quiz-header").removeClass("is-inactive");
        },

        // Store the current frame so we can access it from another function
        frameGet: function () {
            s.frameCur = $(".quiz-body > div:visible");
        },

        // get Questions TODO Clean up
        questionsGet: function () {
            $.get('questions.xml', function (d) {
                $('item', d).each(function (index) {
                    var $quiz = $(this),
                        question = $quiz.find('question').text(),
                        answers = '',
                        correctAnswer = $.trim($quiz.find('answer[correct]').text()),
                        html;
                    $quiz.find('answer').each(function (i) {
                        var answer = $.trim($(this).text());
                        answers += '<div class="quiz-answer answer-' + (i + 1) + ' js-quiz-answer"><a href="#">' + answer + '</a></div>';
                    });
                    html = '<div class="quiz-body-question js-quiz-body-question' + (index + 1) + '">';
                    html += '<p> ' + question + '</p>';
                    html += answers;
                    html += '<div class="fine">' + s.questionText + '</div>';
                    html += '<div class="correct-answer hidden">' + correctAnswer + '</div>';
                    html += '</div>';
                    $('.js-quiz-body-correct').before($(html));
                });
            });
        },

        //compare Answers
        answerCheck: function (userAnswer) {
            var u = userAnswer.text(),
                c = userAnswer.parents(".quiz-body-question").find(".correct-answer").text();
            this.timerPause();
            s.answerCorrect = u === c ? true : false;
            this.answerScreen(c);
        },

        // Choose corect or incorrect answer screen
        answerScreen: function (c) {
            if (s.answerCorrect === false) {
                var ic = $(".js-quiz-body-incorrect");
                this.frameNext(ic);
                ic.removeClass("is-expired");
            } else {
                this.frameNext($(".js-quiz-body-correct"));
                this.scoreTally();
            }
            this.answerDisplay(c);
        },

        // Show next question
        questionNext: function () {
            s.questionCounter += 1;
            if ($(".js-quiz-body-question" + s.questionCounter).length > 0) {
                this.frameNext($(".js-quiz-body-question" + s.questionCounter));
                this.stateActive();
                this.timerReset();
                this.timerStart();
            } else {
                this.stateInactive();
                this.scoreFinalUpdate();
                this.frameNext($(".js-quiz-body-final"));
            }
        },

        answerDisplay: function (c) {
            $(".js-display-correct-answer span").text(c);
        },

        // Hide current slide
        frameHide: function () {
            $(".quiz-body > div").stop(true,true).fadeOut(100);
        },

        // Move to next step
        frameNext: function (next) {
            this.frameHide();
            next.delay(100).fadeIn(100);
        },

        // Tally score
        scoreTally: function () {
            s.score += 100 + ((s.count + 1) * 10);
            this.scoreUpdate();
        },

        // Update score display
        scoreUpdate: function () {
            $(".js-quiz-score-numeric").text(s.score);
        },

        scoreFinalUpdate: function () {
            $(".js-final-score-numeric").text(s.score);
        },

        // reset app
        restartQuiz: function () {
            s = $.extend({}, QuizDefaults);
            this.scoreUpdate();
            this.timerReset();
            this.frameNext($(".js-quiz-body-start"));
        }

    };
$(function () {
    QuizApp.init();
});
}(jQuery));

My main question is how do I know how deep to abstract functionality into individual functions? Two that I struggled with abstracting further but that really needed it were the functions helpToggle and questionsGet. Also wondering how much abstraction is too much? Lastly, any pointers on ways to optimize this style of coding are appreciated, I'm new to the module pattern and have been writing mediocre code for a while, really trying to get better, so anything helps. Thank you!

EDIT: Per request, I've added the relevant html below, in case it makes my question clearer. I attempted to create a jsfiddle, but failed when I tried to use the XML import.

    <div class="modal-overlay js-modal-overlay">
    </div>
    <div class="modal-container js-modal-container">
        <div class="modal-wrapper">
            <div class="modal">
                <div class="close">
                    <a href="#" class="js-quiz-modal-close"><span>X</span> Close</a>
                </div>
                <div class="quiz">
                    <div class="quiz-header js-quiz-header">
                        <div class="quiz-logo">
                            <img src="img/quiz-logo.png" />
                        </div>
                        <div class="quiz-options">
                            <div class="quiz-score">
                                <span class="quiz-score-text">Score:</span>
                                <span class="quiz-score-numeric js-quiz-score-numeric">0</span>
                            </div>
                            <div class="quiz-help">
                                <a href="#" class="js-quiz-help"></a>
                            </div>
                            <div class="quiz-tooltips">
                                <div class="tip tip-resume">
                                    <p>Click to 
                                    resume game</p>
                                </div>
                                <div class="tip tip-score">
                                    <p>The quicker you answer correctly the more points you receive</p>
                                </div>
                                <div class="tip tip-timer">
                                    <p>10 seconds 
                                    to answer</p>
                                </div>
                                <div class="tip tip-answer">
                                    <p>Click on the answer to make your selection</p>
                                </div>
                            </div>
                        </div>
                        <div class="quiz-timer">
                            <div class="numeric-timer js-numeric-timer">
                                <span class="numeric-timer-ticker js-numeric-timer-ticker">10</span><span class="numeric-timer-default">10</span>
                            </div>
                            <div class="pie-timer js-pie-timer"><div></div></div>
                        </div>
                    </div>
                    <div class="quiz-body">
                        <div class="quiz-body-start js-quiz-body-start">
                            <p>Think you are <br />the ULTIMATE FAN?</p>
                            <a href="#" class="quiz-button js-quiz-button-begin">Test Your Wits <span>&#x25b6;</span></a>
                        </div>
                        <div class="quiz-body-correct quiz-result js-quiz-body-correct">
                            <p>Nicely Done!</p>
                            <div class="quiz-answer is-selected js-display-correct-answer">
                                <span>Professional Wrestler</span>
                            </div>
                            <a href="#" class="quiz-button quiz-button-continue js-quiz-button-continue">Continue <span>&#x25b6;</span></a>
                        </div>
                        <div class="quiz-body-incorrect quiz-result js-quiz-body-incorrect">
                            <p>Oops, <span class="missed-it">missed it</span><span class="times-up">Time's up</span>.</p>
                            <span class="missed-answer fine">Correct Answer:</span>
                            <div class="quiz-answer is-selected  js-display-correct-answer">
                                <span>Professional Wrestler</span>
                            </div>
                            <a href="#" class="quiz-button quiz-button-continue js-quiz-button-continue">Continue <span>&#x25b6;</span></a>
                        </div>
                        <div class="quiz-body-final js-quiz-body-final">
                            <p>You Rock!</p>
                            <div class="final-score">
                                <span class="final-score-text">Final Score:</span>
                                <span class="final-score-numeric js-final-score-numeric">123</span>
                            </div>
                            <a href="#" class="quiz-button quiz-button-finish  js-quiz-button-finish">Finish <span>&#x25b6;</span></a>
                            <a href="#" class="quiz-button quiz-button-again  js-quiz-button-again">Play Again <span>&#x25b6;</span></a>
                        </div>
                        <div class="quiz-body-default-question js-quiz-body-default-question">
                            <p>Sample quiz question goes here multiple lines?</p>
                            <div class="quiz-answer quiz-answer1">
                                <span class="answer-demo">Answer</span>
                            </div>
                            <div class="quiz-answer quiz-answer2">
                                <span class="answer-demo">Answer</span>
                            </div>
                            <div class="quiz-answer quiz-answer3">
                                <span class="answer-demo">Answer</span>
                            </div>
                            <div class="quiz-answer quiz-answer4">
                                <span class="answer-demo">Answer</span>
                            </div>
                        </div>
                    </div>
                </div><!-- .quiz -->
            </div>
        </div>
    </div><!-- .modal-container -->

Can you add the html to go with this? Even better a fiddle also. — abuzittin gillifirca, Dec 6, 2013 at 12:03
Two things I need to know. First, are you at all interested in security, e.g. is there any consequence or motivation for someone to cheat on your quiz? Second, when you ask about abstraction, what sort of changes are you interested in protecting against, e.g. do you need this JS to survive a total HTML transplant? — sqykly, Dec 7, 2013 at 12:28
@sqykly, no security necessary, it's a children's quiz with no reward besides a score :) it does not need to survive a transplant, I was just looking for general guidance and best practices. — Ivan Durst, Dec 10, 2013 at 1:43

James Khoury · Accepted Answer · 2013-12-06 03:31:17Z

5

Slight issue in your JavaScript. I can't find s declared. I assume it should have been settings.
String concatenation inside a loop is not a good idea for performance. I'd also try to keep the addition to the DOM till after the loop.
you can treat some values as truthy/falsey instead of === if you know you have set them.
```
if(s.answerCorrect === false)
```
is more confusing than writing
```
if(!s.answerCorrect)
```
Similarly I'd change
```
s.answerCorrect = u === c ? true : false;
```
to
```
s.answerCorrect = u === c;
```
I'd say your level of abstraction is bordering on too much. for example:
```
this.centerModal();
```
is used in only one place and is only a couple of lines of code. This wouldn't be enough to make me create another function for it.

On the otherhand I would move constants into variables/settings like:
```
var CLASS_PREFIX = ".js-quiz";
var INCORRECT_BODY_CSS_CLASS = CLASS_PREFIX + "-body-incorrect";
```
only for classes and text you might want to change or repeat.

In general I would say this is a clean and readable attempt.

edited Dec 6, 2013 at 3:31

answered Dec 5, 2013 at 23:32

James Khoury

3,0951 gold badge24 silver badges51 bronze badges

\$\begingroup\$ Thank you for the in-depth analysis sir! Great info here. S is declared at the very top: ; (function ($) { "use strict"; var s, QuizDefaults \$\endgroup\$
– Ivan Durst
Dec 5, 2013 at 23:46
\$\begingroup\$ So in general, it's better to not abstract functionality into a separate function if it's only going to be used in one other function? \$\endgroup\$
– Ivan Durst
Dec 5, 2013 at 23:50
\$\begingroup\$ @IvanDurst - it may be language dependant, but sometimes (like in Java), having a function call inside a loop even if that function is only called from in the loop, can sometimes in fact help performance. For non-looped, called-from-one-place, small functions, I would say, manually 'inline' the function. \$\endgroup\$
– rolfl
Dec 6, 2013 at 0:46
\$\begingroup\$ @rolfl, great info, thank you. James, I will likely be accepting this answer soon. \$\endgroup\$
– Ivan Durst
Dec 6, 2013 at 1:04

Add a comment |

abuzittin gillifirca · Accepted Answer · 2013-12-07 11:51:08Z

Your application does not regard Separation of Concerns. Which makes it unmaintainable and untestable.

For example:

You store your application state in DOM. If you are not experienced enough to know that it's just bad, consider this: string js-quiz-body-question found 6 times throughout the code. Every piece of code should be as DRY as possible.

Look for example at function questionsGet.

It does retrieves an XML document.
Walks its DOM.
Builds up HTML snippet.
Inserts the HTML snippet to current document.

A function's name should tell what it does. Just extracting the last 3 of the above into a named function, say renderQuestionsFromXmlDocument you could easily add XML inline for testing.

renderQuestionsFromString : function() {
    var doc = $.parseXML("<quiz><item><question>question?</question><answer>ans1</answer><answer correct='correct'>ans2</answer></item></quiz>");
    renderQuestionsFromXmlDocument(doc);
}

You should not do much in init at all. Don't look for things for example. init should only set fields that differ from their *sensible default*s.

Stack Exchange Network

Deeper abstracting of code into functions

2 Answers 2

Your Answer

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

Hot Network Questions

Deeper abstracting of code into functions

2 Answers 2

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions

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