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>
1 Answer 1
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());
};
-
1\$\begingroup\$ 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
Quizclass, you have semi-colons in the prototype property list, you referencethis.configandthis.questionswithout declaring them as anything other than local variables, etc. \$\endgroup\$mhodges– mhodges2016年10月25日 16:37:20 +00:00Commented Oct 25, 2016 at 16:37 -
1\$\begingroup\$ @mhodges There is no need to "return" anything from main class (function) enclosure. The
newkeyword 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. \$\endgroup\$Mike Brant– Mike Brant2016年10月25日 18:59:03 +00:00Commented Oct 25, 2016 at 18:59 -
\$\begingroup\$ 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.prototypethen you are correct. +1'd \$\endgroup\$mhodges– mhodges2016年10月25日 19:31:13 +00:00Commented Oct 25, 2016 at 19:31
request.donefunctions, abovecurrentIndexandcurrentScore, and aboveHandlebars.compile. Other ones can be reworded or simplified, but they're not tragic. \$\endgroup\$Handlebars.compilemethods,currentIndexandcurrentScore. Thanks for pointing those out. \$\endgroup\$