current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 0 down vote favorite

I have a quiz app that gets data from a questions.json file using AJAX, loads them into my script file, and then use Handlebars to compile templates so I don't have to mix HTML in my JavaScript code. I welcome any improvements on my current code.

app.js

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

    // Get all the questions from the JSON file
    var request = $.getJSON("questions.json");

    var Quiz = {
        // Current question index in the array, starting with 0
        currentIndex: 0,

        // Current score, increments when the user chooses the right answer
        currentScore: 0,

        config: {},

        init: function(config) {
            this.config = config;

            // When the request to the questions.json file is complete
            request.done(function(questions) {
                // If they reached the final question of the quiz
                // Calculate their final score
                if (Quiz.currentIndex + 1 > questions.length) {
                    Quiz.renderFinalScore(Quiz.config.finalScoreTemplateEl, questions);
                } else {
                    Quiz.renderTitle(Quiz.config.titleTemplateEl, questions);
                    Quiz.renderChoices(Quiz.config.choicesTemplateEl, questions);   
                }
            });
        },

        renderTitle: function(titleTemplateEl, questions) {
            // Compile the title template
            var template = Handlebars.compile($(titleTemplateEl).html());

            var context = {
                title: questions[Quiz.currentIndex].title
            };

            // Display the question title
            $(Quiz.config.questionTitleEl).html(template(context));
        },

        renderChoices: function(choicesTemplateEl, questions) {
            var template = Handlebars.compile($(choicesTemplateEl).html());

            var context = {
                choices: questions[Quiz.currentIndex].choices
            };

            // Display the question choices
            $(Quiz.config.choicesEl).html(template(context));
        },

        handleQuestion: function(event) {
            // Just so I don't have to type the same thing again
            var questions = event.data.questions;
            var correctAnswer = questions[Quiz.currentIndex].correctAnswer;
            var userInput = $("input[name=choices]:checked").val();

            if (parseInt(userInput, 10) === correctAnswer) {
                Quiz.currentScore += 1;
            }

            // Increment the current index so it can advance to the next question
            // And Re-render everything.
            Quiz.currentIndex += 1;
            Quiz.init(Quiz.config);
        },

        renderFinalScore: function(finalScoreTemplate, questions) {
            var template = Handlebars.compile($(finalScoreTemplate).html());
            var context = {
                totalScore:      Quiz.currentScore,
                questionsLength: questions.length   
            };

            $(Quiz.config.quizEl).html(template(context));
        }
    };


    // Providing a config object just so 
    // when the element names change for some reason 
    // I don't have to change the whole element names
    Quiz.init({
        choicesTemplateEl:      "#choices-template",
        titleTemplateEl:        "#title-template",
        questionTitleEl:        "#question-title",
        choicesEl:              "#choices",
        finalScoreTemplateEl:   "#finalScore-template",
        quizEl:                 "#quiz",
    });

    // Passing the questions as a parameter so it's available to the handleQuestion method
    request.done(function(questions) {
        // When they click on the `Next Question` button
        // Passing a event.data.questions variable so I can use it in the
        // handleQuestion method.
        $("#next-question").on("click", {questions: questions}, Quiz.handleQuestion);
    });
})(jQuery, Handlebars);

index.html

The Handlebars templates are at the bottom of the page, and I did templates for even the simplest things like the question title.

<!DOCTYPE html>
<html>
<head>
    <title>Quiz App</title>
    <link rel="stylesheet" type="text/css" href="style.css">
    <link href="https://fonts.googleapis.com/css?family=Open+Sans" rel="stylesheet">
    <link rel="shortcut icon" href="">
</head>
<body>
    <div id="quiz">
        <div id="question-title"></div>
        <div id="choices"></div>
        <button id="next-question" class="button">Next Question</button>
    </div>

<!-- Scripts -->
<script src="https://code.jquery.com/jquery-3.1.1.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/handlebars.js/4.0.5/handlebars.js"></script>
<script src="app.js"></script>

<!-- Templates  -->
<script id="choices-template" type="text/x-handlebars-template">
        {{#each choices}}
        <div class="choice">
            <input type="radio" name="choices" id="choice{{@index}}" value="{{@index}}">
            <label for="choice{{@index}}">{{ @this }}</label>
        </div>
        {{/each}}
</script>
<script  id="title-template" type="text/x-handlebars-template">
    {{ title }}
</script>

<script  id="finalScore-template" type="text/x-handlebars-template">
    <h2 id="finalScore">You've scored {{ totalScore }} out of {{ questionsLength }}!</h2>
</script>

</body>
</html>

Live Demo

share|improve this question
    
For what it's worth, 5 lines into this code, I could tell that you were likely young or a student without professional dev experience by your use of superfluous comments. Some examples are the comments: above both request.done functions, above currentIndex and currentScore, and above Handlebars.compile. Other ones can be reworded or simplified, but they're not tragic. – mhodges Oct 25 '16 at 16:51
    
I made obvious comments so it's not confusing at the first sight, I wrote those comments 4 in the morning so when I wake up I could read it without using my brain, and yeah I'm young, I don't have professional dev experience, I'm trying to learn Javascript by creating a Quiz application, I was pretty sure that those were obvious comments. – Akar Oct 25 '16 at 16:55
    
That's understandable, I 100% agree with putting comments in like that during development if you are still learning or need reminders on what things do, but from a code-review perspective, those should be removed for production/professional purposes. – mhodges Oct 25 '16 at 17:03
    
I just copy pasted what code I had during the development, I didn't remove the comments to look more professional, I removed the most obvious ones like the ones you mentioned, above Handlebars.compile methods, currentIndex and currentScore. Thanks for pointing those out. – Akar Oct 25 '16 at 17:09
up vote 3 down vote

Some thoughts

I like your approach with regards to separating display from logic. Keep thinking this way and you will end up with more re-usable and maintainable code.

Since you are, in essence, building out a Quiz class, I would think you should consider implementing your various properties and methods in more "class-like" manner (not necessarily ES6 type classes, but something ES5 compatible like I have shown below).

This would allow for higher level of re-use for your Quiz class (i.e. multiple quizzes on same page) and allow for optimized performance of methods (because they exist on prototype rather than on the instance itself).

// define instance properties and methods:
function Quiz(callerConfig) {
    // public instance properties
    this.currentIndex = 0;
    this.currentScore = 0;
    this.config = {};

    // cache jQuery selection references on prototype
    this.$quizEl = null;
    ...

    // store quiz question/answer information on object
    // so you don't have to continually pass question information around
    this.questions = [];

    // constructor
    function init() {
        // finalize instance configuration
        this.config = $.extend(Quiz.defaultConfig, callerConfig);
        var config = this.config;

        // do other set-up steps like pulling in your JSON file
        // make sure to handle success AND failure case for getJSON
        // set question information on object in `done()` callback
        $.getJSON.(config.jsonUri)
            .done( ... )
            .fail( ... )
            .always( ... );

        // execute jQuery references and cache on object.
        this.$quizEl = $(config.quizEl);
        ...
    }

    // call constructor
    init(); 
}

// Define class methods on prototype
// Any method logic that does not need top be defined for a specific instance
// should be here.
// I think most of your current methods would fall in this category.
Quiz.prototype = {
    renderTitle: function(...),
    renderChoices: function(...),
    ...
}

// Define static class variables.
// A good example would be default configuration
Quiz.defaultConfig = {
    ...
};


// usage example
// assumes Quiz class has already been included
(function($, Handlebars) {
    // instantiate quiz with default config
    var quiz1 = new Quiz(); 

    // instantiate quiz with alternate URI for JSON file and some different selectors
    var quiz2 = new Quiz({
        'jsonUri': 'someOtherFile.json',
        'quizEl': '#quiz2',
        ...
    });
})(jQuery, Handlebars);

var request = $.getJSON("questions.json");

Should URI for question file be configurable rather than hard-coded?

I don't see great need to store the jqxhr reference in request here. It seems odd to have callbacks against the request jqxhr object split across different areas of your code. If all that is happening with this request is that you retrieve the JSON for question configuration, why not just execute and store the questions on the object and then not have to maintain reference to request and pass questions around between methods? You could even unset and make that memory available to garbage collection immediately after callbacks are processed.

If you take the approach noted above and store element refences on object as well as question information on object, all of your public class methods could change signature to not require parameters to be passed.

For example:

renderTitle: function(titleTemplateEl, questions) {

Can become:

renderTitle: function () {

If you take suggestion of moving this to more "class-like" structure, you would need to replace references in your methods from Quiz (the class) to this (the instance).

I would consider separating the logic on grading or evaluating the question from the logic to navigate between questions. Right now both occur in handleQuestion() method. Perhaps handleQuestion() should just call a method like nextQuestion() which hold the logic for next question.

This also gives you the ability to allow the user to navigate amongst questions (i.e. previous, next) separately from the grading operation, and perhaps do things down the line like:

  • randomize question order (something that should not be tied to grading)
  • grade all questions only once all questions are completed.
var userInput = $("input[name=choices]:checked").val();

I would consider building and storing reference to choice options related to a specific question as part of the renderChoices. That way in this method you don't need to do any jQuery selection to determine the universe of options related to this particular question. I also think that you might want to consider the fact that you could have multiple items selected if you ever move beyond just using radio buttons, which should probably be a property of the question (i.e. allow multiple answers), not something hard-coded into the template.

So you might end up with something like:

var answerValues = [];
this.questions[this.currentIndex].$answers
    .filter(':checked').each(function() {
        answerValues.push($(this).val());
    };
share|improve this answer
1  
Your idea with restructuring the OP's class is on the right track, but please spend some more time on the code you have provided. You're not returning anything from the Quiz class, you have semi-colons in the prototype property list, you reference this.config and this.questions without declaring them as anything other than local variables, etc. – mhodges Oct 25 '16 at 16:37
1  
@mhodges There is no need to "return" anything from main class (function) enclosure. The new keyword will result in an instance of that "class" being assigned to whatever variable it is being assigned to. You are correct on other issues, which I have corrected in example. In throwing this example together I conflated "public" instance variables (this.whatever) vs. "private" instance variables (var whatever). I have simplified everything to use public instance variables for sake of simplicity. – Mike Brant Oct 25 '16 at 18:59
    
Yes, the only reason I mentioned not returning anything was because you did not make any of your variables/functions public. If you are going to use this.* and .prototype then you are correct. +1'd – mhodges Oct 25 '16 at 19:31

Your Answer

 
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.