3
\$\begingroup\$

My code fetches quiz data from a JSON file, displays the questions sequentially and records the user's answers. It keeps track of the user's score and at the end display the user's result based on a second JSON file.

const QUIZ_API = 'quiz.json';
const RESULT_API = 'result.json';
const QUIZ_ID = 12;  // Used to identify the quiz in the JSON file.

let jsonQuizData;
let questionNo = 0; // Current question number
let answerSubmitted = false; // Is set to true once the user submits an answer to the current question and still hasn't proceeded to the next one.
let points = 0; // User's current score
let maxPoints = 0; // Max possible score for quiz

/*
* When the document is ready, adds a key listener to enable playing the quiz using the keyboard only, and loads quiz data.
 */
$(document).ready(() => {
    // Allow submitting answer with Enter key
    document.addEventListener('keyup', (event) => {
        if (event.keyCode === 13) // 13 is the Enter key
            $('#next-btn').click();
    });

    $.getJSON( QUIZ_API, { quiz_id: QUIZ_ID})
        .done((resp) => {
            jsonQuizData = resp;
            $('h1').text(resp.title);
            $('h2').html('“' + resp.description + '”');
            updateQuestionDisplay(resp.questions[0]);
        })
        .fail(() => {
            alert('The quiz is not available at the moment! Please try again later.');
        });
});

/*
* Displays the current question and its possible answers
* @param {Object} qObject - Object representing the current question
*/
function updateQuestionDisplay(qObject) {
    /* Question image */
    const imageAlt = `Question ${questionNo + 1} image`;
    $('#question').find('img').attr({
        src: qObject.img,
        alt: imageAlt
    });

    /* Question header (Question number, question title, question points) */
    let maxStr = ''; // Questions with multiple answers will have the word 'max' after the number of points, in order to show that the user may receive fewer points for a partially correct answer.
    if (qObject.question_type === 'mutiplechoice-multiple')
        maxStr = ' max';
    let ptsStr = qObject.points === 1? ' pt': ' pts';
    $('#qTitle').html(qObject.q_id + '. ' + qObject.title + ' <strong>[' + qObject.points + ptsStr + maxStr + ']</strong>');

    /* Question type */
    let inputType;
    switch(qObject.question_type) {
        case 'mutiplechoice-multiple':
            inputType = 'checkbox';
            break;
        default:
            inputType = 'radio';
    }

    /* Answers */
    const inputsContainer = $('#inputs');
    inputsContainer.css('text-align', 'left');

    if (qObject.question_type === 'truefalse') {
        inputsContainer.append(`
        <div class="option">
            <input type="${inputType}" id="true" name="${qObject.q_id}" value="true">
            <label for="true">True</label>
        </div>
        `);
        inputsContainer.append(`
        <div class="option">
            <input type="${inputType}" id="false" name="${qObject.q_id}" value="false">
            <label for="false">False</label>
        </div>
        `);
    }
    else
        qObject.possible_answers.forEach((item) => {
            inputsContainer.append(`
            <div class="option">
                <input type="${inputType}" id="${item.a_id}" name="${qObject.q_id}" value="${item.a_id}">
                <label for="${item.a_id}">${item.caption}</label>
            </div>
            `);
        });
}

/*
* Performs appropriate task on clicking button based on answerSubmitted
 */
function nextButtonClicked() {
    if (answerSubmitted)
        nextQuestion();
    else
        submitAnswer();
}

/*
* Validates and submits the user's answer, changes document and styles accordingly.
 */
function submitAnswer() {
    const inputs = $('input');
    let ans = [];   // ans is an array to accomodate the case of multiple answer questions.
    inputs.each((i, obj) => {
        if (obj.checked)
            ans.push(obj.id);   // obj.id is always a string.
    });
    if (!ans.length)
        alert('No answer selected!');
    else {  // If an answer was selected
        answerSubmitted = true;
        checkAnswer(ans);

        const nextBtn = $('#next-btn');

        inputs.attr('disabled', 'disabled');
        nextBtn.attr('disabled', 'disabled');
        $('#inputs').css('text-align', 'center');

        let newButtonText = jsonQuizData.questions[questionNo + 1]? 'Next question': 'See results!';

        $(this).delay(500).queue(() => {
            nextBtn.text(newButtonText);
            nextBtn.removeAttr('disabled');
            $(this).dequeue();
        });
    }
}

/*
* Performs 2 tasks:
* 1. Add css to indicate the correct answer and highlight the user's chosen answer.
* 2. Check answer and update points.
* @param {string[]} answer - User's submitted answer
 */
function checkAnswer(answer) {
    const correctAnswer = jsonQuizData.questions[questionNo].correct_answer;
    let pointsToAward = jsonQuizData.questions[questionNo].points;
    maxPoints += pointsToAward; // Update maxPoints for the current question.

    /* 1. Add css to indicate the correct answer and highlight the user's answer. */
    if (jsonQuizData.questions[questionNo].question_type === 'truefalse') { // For true-false questions it is straightforward
        $('label[for="' + correctAnswer.toString() + '"]').addClass('correct');
        $('label[for="' + (!(correctAnswer)).toString() + '"]').addClass('incorrect');
        $('label[for="' + answer.toString() + '"]').parent().addClass('selected');
    }
    else {  // For questions which are not of true-false type
        jsonQuizData.questions[questionNo].possible_answers.forEach((obj, i) => {
            if (answer.includes(obj.a_id.toString()))
                $('label[for="' + obj.a_id.toString() + '"]').parent().addClass('selected');
            if (typeof correctAnswer === 'object' && correctAnswer != null && correctAnswer.includes(jsonQuizData.questions[questionNo].possible_answers[i].a_id) || correctAnswer === obj.a_id)
                $('label[for="' + obj.a_id.toString() + '"]').addClass('correct');
            else
                $('label[for="' + obj.a_id.toString() + '"]').addClass('incorrect');
        });
    }

    /* 2. Check answer and update points. */
    if (typeof jsonQuizData.questions[questionNo].correct_answer === 'object' && jsonQuizData.questions[questionNo].correct_answer != null) { // The if statement checks if it is a question with multiple answers. This is because 'object' is the type of arrays, and of null. Since correct_answer might be null, we check if it's not null as well.
        const noOfItems = jsonQuizData.questions[questionNo].possible_answers.length;
        let resultBool = new Array(noOfItems).fill(false);  // For each option, resultBool[i] is false if the user's answer is wrong, else true.

        // Setting resultBool
        jsonQuizData.questions[questionNo].possible_answers.forEach((obj, i) => {
            if (correctAnswer.includes(obj.a_id) && answer.includes(obj.a_id.toString()) || !correctAnswer.includes(obj.a_id) && !answer.includes(obj.a_id.toString()))
                resultBool[i] = true;
        });

        resultBool.forEach((obj) => {
           if (!obj)
               pointsToAward = pointsToAward - !!pointsToAward;    // Decrements pointsToAward by 1 if it's greater than 0.
        });

        points += pointsToAward;
    }
    else {  // Questions with a single answer
        if (answer[0] === correctAnswer.toString())
            points += pointsToAward;
    }
}

/*
* If a subsequent question exists, increments questionNo and displays the next question
* with the help of updateQuestionDisplay(). Else, calls displayResults.
 */
function nextQuestion() {
    if  (jsonQuizData.questions[questionNo + 1]) { // If there is a next question
        // Remove current options
        $('.option').each((i, obj) => {
            obj.remove();
        });

        // Replace image with placeholder image so users with slow internet connections do not see the previous level's image while the new level's image is still loading.
        $('.img-container-small').find('img').attr('src', 'img/placeholder-image.png');

        // Set button text
        $('#next-btn').text('Submit');

        // Set answerSubmitted to false
        answerSubmitted = false;

        // Increment question number and display next question
        updateQuestionDisplay(jsonQuizData.questions[++questionNo]);
    }
    else
        displayResults();
}

/*
* Calculates the user's score as a percentage, loads the result data, finds the user's
* result group, displays the user's result.
 */
function displayResults() {
    const pointsPercent = Math.round(points / maxPoints * 100);
    let resultGroupIndex = 0;

    let jsonResultsData;
    $.getJSON( RESULT_API, { quiz_id: QUIZ_ID})
        .done((resp) => {
            jsonResultsData = resp;
            jsonResultsData.results.forEach((obj, i) => {
                if (pointsPercent >= obj.minpoints)
                    resultGroupIndex = i;
            });

            $('main').html(`
            <section>
                <h3>Quiz completed! Points earned: <strong>${points}/${maxPoints} (${pointsPercent}%)</strong></h3>
                <p class="huge-font m-y-sm">${jsonResultsData.results[resultGroupIndex].title}</p>
                <div class="img-container-medium"><img src="${jsonResultsData.results[resultGroupIndex].img}" alt="Result image"></div>
                <p class="p-b-md">${jsonResultsData.results[resultGroupIndex].message}</p>
            </section>
            `);
        })
        .fail(() => {
            alert('Your result is not available at the moment! Please try again later.');
        });
}

The reason I use global variables and all my functions have global scope is because in my HTML file I have this:

<button id="next-btn" onclick="nextButtonClicked()">Submit</button>

Since my HTML button references the function nextButtonClicked(), that function has to have global scope. Since this function eventually ends up calling all the other functions and also needs to use the global variables, I have also given them global scope.

However, I am not sure if this is the best way to go about solving this problem. Also, I'm wondering if it would be a good idea to use namespaces and how.

Moreover, the only ES6 features I'm using are arrow functions, let and const. Are there other ES6 features that I can incorporate?

\$\endgroup\$
  • \$\begingroup\$ I found a potential improvement to my own code. The line if (typeof correctAnswer === 'object' && correctAnswer != null && correctAnswer.includes(jsonQuizData.questions[questionNo].possible_answers[i].a_id) || correctAnswer === obj.a_id) can be written as if ((typeof correctAnswer === 'object' && correctAnswer != null && correctAnswer.includes(jsonQuizData.questions[questionNo].possible_answers[i].a_id)) || correctAnswer === obj.a_id) Adding the extra parentheses is considered good practice: softwareengineering.stackexchange.com/a/201187/157676 \$\endgroup\$ – hb20007 Mar 15 '18 at 21:22
  • \$\begingroup\$ why do you make a particular case for questions typed truefalse, when they are just questions typed radio with 2 items, true and false? \$\endgroup\$ – scraaappy Mar 17 '18 at 20:30
  • 1
    \$\begingroup\$ @scraaappy It's a pecualiarity of the JSON file I'm using, it has a different structure for true-false type questions. \$\endgroup\$ – hb20007 Mar 17 '18 at 20:37
  • \$\begingroup\$ More improvements: () in arrow functions like (obj) => {} are unnecessary. obj => {} works fine. Also I use jQuery but I choose to use .forEach() instead of jQuery's $.each(). This is not necessarily an improvement though because jQuery's $.each() kinda sucks because of stackoverflow.com/questions/13095607/… \$\endgroup\$ – hb20007 Apr 2 '18 at 18:59
  • \$\begingroup\$ Even more improvements: if ((typeof correctAnswer === 'object' && correctAnswer != null && correctAnswer.includes(jsonQuizData.questions[questionNo].possible_answers[i].a_id)) || correctAnswer === obj.a_id) can be simplified to: if ((typeof correctAnswer === 'object' && correctAnswer != null && correctAnswer.includes(obj.a_id)) || correctAnswer === obj.a_id) and $('.option').each((i, obj) => { obj.remove(); }); to $('.option').remove(); \$\endgroup\$ – hb20007 Apr 3 '18 at 21:08
2
\$\begingroup\$

Your Questions

However, I am not sure if [having the function nextButtonClicked() in the global scope] is the best way to go about solving this problem.

As was already mentioned in Arnav's answer, it would be wise to set the event handler for that element in Javascript. One technique is to use jQuery's .click() method (which is really just a shortcut for .on('click'))

So at the beginning of the script, where constants and variables are defined, add let nextBtn;, and in the DOM-ready callback, assign that value:

$(document).ready(() => {
    //move this out of submitAnswer()
    nextBtn = $('#next-btn');

As the comment implies, that assignment can be removed from the function submitAnswer(). Then the HTML for the button can be simplified to not have that onclick attribute, as many believe this leads to good separation for the business logic to be out of the markup.

 <button id="next-btn">Submit</button>

Actually, that function .ready() is deprecated as of version 3.0. See the last paragraph below for more details on that topic.

Moreover, the only ES6 features I'm using are arrow functions,let and const. Are there other ES6 features that I can incorporate?

The code already uses template literals. Most of the functions don't appear to currently have more than one parameter but if any did, there could be a place to use default parameters. Also, you could perhaps utilize destructuring assignment somewhere - maybe for the assignment of resultsBool?

If you really wanted, you could perhaps utilize the class structure to move your code to being slightly more object-oriented, but be aware that it is "primarily syntactical sugar over JavaScript's existing prototype-based inheritance. The class syntax does not introduce a new object-oriented inheritance model to JavaScript."1.

Other feedback

switch statement

let inputType;
switch(qObject.question_type) {
    case 'mutiplechoice-multiple':
        inputType = 'checkbox';
        break;
    default:
        inputType = 'radio';
}

If there were more cases in the switch statement, then it might make sense to have that but because there are really only two cases, one could argue that it should be simplified to a simple if-else statement:

let inputType;
if (qObject.question_type == 'mutiplechoice-multiple') {
    inputType = 'checkbox';
}
else {
    inputType = 'radio';
}

While that is only one less line, there is no need to worry about adding break statements (some might argue it should have been added in the default case). It could also be simplified by moving the assignment from the else block to the initial declaration:

let inputType = 'radio';
if (qObject.question_type == 'mutiplechoice-multiple') {
    inputType = 'checkbox';
}

let vs const

There is a variable created for newButtonText

let newButtonText = jsonQuizData.questions[questionNo + 1]? 'Next question': 'See results!';

However it apepars to never be re-assigned. It is wise to default to making this a constant with const until a valid reason to reassign that value arises.

Using jQuery and then document.addEventListener()

If jQuery is loaded on the page, then why use document.addEventListener(), as in:

document.addEventListener('keyup', (event) => {

When instead it could be simplified using jQuery's .on() method?

$(document).on('keyup', (event) => { ... })

Edit

Another more jQuery like way to avoid global variables, making use of IIFE's and $(document).ready(function() {})

Sure you could do that, though bear in mind that as of jQuery 3 "only $(handler) is recommended" and $(document).ready() will still work but is deprecated.2

\$\endgroup\$
3
+50
\$\begingroup\$

The reason I use global variables and all my functions have global scope is because in my HTML file I have this:

<button id="next-btn" onclick="nextButtonClicked()">Submit</button>

Since my HTML button references nextButtonClicked(), the function has to have global scope

In modern javascript, polluting the global namespace is generally considered a bad idea as there is already so much in there. In order to move everything outside of the global namespace, I would wrap everything in a Anonymous IIFE such as the following:

(function() {
    const QUIZ_API = 'quiz.json';
    const RESULT_API = 'result.json';
    const QUIZ_ID = 12;  // Used to identify the quiz in the JSON file.
    // ... Rest of code ...
}());

You could even use an arrow function here.

Now to address your specific problem: Adding an event listener. To solve this issue, I would add the following to your $(document).ready(() =>:

$(document).ready(() => {
    document.getElementById("next-btn").addEventListener("click", nextButtonClicked);
});

Using the jQuery click listener is also an option. The above will do the same thing as the onclick and one advantage to this is that the JS code now doesn't pollute your HTML. (More information)

Also, I'm wondering if it would be a good idea to use namespaces and how.

TBH, in such a small program, namespacing wouldn't really add much benefit. If you were planning on using a module system of some sort to separate the different parts of your code or were planning on increasing the program's size, then you would multiple files with namespaces to create modularity. There are actually many examples online of this, such as the revealing module pattern.

A Minor Style Issue

else
    qObject.possible_answers.forEach((item) => {

I notice that sometimes you omit the braces with conditional branches when there is only one statement inside. While this is allowed, this is generally not considered good practice because you never know when you may unknowingly add an extra line. It also hurts readability (Especially in the case above), since it is now hard to tell when the block starts/ends.

EDIT:

Another more jQuery like way to avoid global variables, making use of IIFE's and $(document).ready(function() {}):

(function($) {
    $(document).ready(function() {
        // Code
    });
})(jQuery);

This approach removes any namespacing conflicts, since now the $ only refers to jQuery inside the IIFE.

\$\endgroup\$
  • 2
    \$\begingroup\$ @hb20007 Readability is hurt when you are inconsistent with the usage of braces or not. In Python, only indentation is used so the pattern is obvious there. For your second question, coming from a language such as (C/C++) where global variables are frowned upon (Not in the JS global sense), I would pass them around. For such a small program, it doesn't really matter, however. I don't see how the linked answer relates... \$\endgroup\$ – Arnav Borborah Mar 15 '18 at 10:37
  • 1
    \$\begingroup\$ @hb20007 By JS Global Sense, I mean that the variables are not polluting the global namespace, they are just global for your code. The reason I believe variables that all your functions can access from a single standpoint are bad is because it makes the state of your program unpredictable, as you don't know when one of those functions modifies that "global" variable. That's why it's better to pass them around as your inputs and outputs are clearly defined. \$\endgroup\$ – Arnav Borborah Mar 15 '18 at 10:44
  • 2
    \$\begingroup\$ @hb20007 Since you want to avoid polluting the global namespace, you want to avoid putting functions there as well. That's why even if you have just functions and no global variables, then you would still use an IIFE. \$\endgroup\$ – Arnav Borborah Mar 15 '18 at 17:03
  • 1
    \$\begingroup\$ @hb20007 It is (Other than the possibilities of global functions, unless you put them in $(document).ready(function(){}), but as I have shown in my new edit to the answer, I would wrap even that inside an IIFE ;-) The code I have shown is common with jQuery code that 1. Prevents global pollution and 2. Guarantees that the $ you are using inside that IIFE is always jQuery. \$\endgroup\$ – Arnav Borborah Mar 17 '18 at 22:08
  • 1
    \$\begingroup\$ @hb20007 Another note. It is simply a matter of preference where you should put your functions (Inside the $(document).ready(() => {}) or not). Just as long as everything is inside the IIFE that I have shown in the code above, then you avoid naming conflicts. My personal preference is to avoid putting functions inside each other (I just don't like it with the exceptions of IIFEs), but it is an extremely common pattern to do so whenever the need comes up. \$\endgroup\$ – Arnav Borborah Mar 17 '18 at 22:11

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.