3
\$\begingroup\$

The app grabs 10 questions from an API, displays them one by one with 4 multiple choice answers. These answers are clickable buttons that alert to whether the answer is correct or not. Finally the last screen shows the amount write and wrong (via wrongCount) and the percentage score as well as a retry that wipes it as restarts. I'm hoping for some good feedback on how bad my new, self taught coding style.


var questionsdata = null;
var wrongCount = 0;
var questionCount = 0;
var answers = [];
var questions = [];
const url = 'https://opentdb.com/api.php?amount=10&type=multiple';

function getData() {
fetch(url)
    .then((resp) => resp.json())
    .then(function(data){
        questionsdata = data.results;
  })
    .catch(function(error){
        alert("Could not get any questions!");
  })
  // add start button creation to load
}

function getQuestion () {
    var correct = []
    var incorrect = []
    for(var i = 0; i < questionsdata.length; i++){
        questions[i] = questionsdata[i].question;
        incorrect[i] = questionsdata[i].incorrect_answers;

    var characters = ['&amp;;', '&quot;', '&#039;', '&rsquo;', '&ldquo;', '&rdquo;', '&eacute;', '&shy;', "&Uuml;", "&Aacute;", '&aacute;'];
    var actual =  ['&', '"', "'", "'", '"', '"', 'é', '-', 'Ü', 'Á', 'á'];

        for (var j = 0; j < incorrect[i].length; j++) {
            for(var h = 0; h < characters.length; h++) {
            incorrect[i][j] =  incorrect[i][j].replace(new RegExp(characters[h], 'g'), actual[h]);
            }
        }

        correct[i] = questionsdata[i].correct_answer;

        for (var j = 0; j < characters.length; j++) {
            correct[i] =  correct[i].replace(new RegExp(characters[j], 'g'), actual[j]);
        }

        answers[i] = [{"answer" : correct[i], "correct" : "correct"}, 
                      {"answer" : incorrect[i][0], "correct" : "incorrect"}, 
                      {"answer" : incorrect[i][1], "correct" : "incorrect"}, 
                      {"answer" : incorrect[i][2], "correct" : "incorrect"}];
        shuffle(answers[i]);

        for(var j = 0; j < characters.length; j++) {
            questions[i] = questions[i].replace(new RegExp(characters[j], 'g'), actual[j]);
        }
    }
}

function shuffle(array) {
    var currentIndex = array.length, temporaryValue, randomIndex;

    // While there remain elements to shuffle
    while (0 !== currentIndex) {

      // Pick a remaining element
      randomIndex = Math.floor(Math.random() * currentIndex);
      currentIndex -= 1;

      // And swap it with the current element.
      temporaryValue = array[currentIndex];
      array[currentIndex] = array[randomIndex];
      array[randomIndex] = temporaryValue;
    }
    return array;
}



function createQuestionElements(question){

if(questionCount < 10){
  var container = document.getElementById("testcontainer");
    container.innerHTML = "";
  var addId = document.createAttribute("id");
    addId.value = "questiontitle";
  var questionTitle = document.createElement("h2");
    questionTitle.setAttributeNode(addId);
    container.appendChild(questionTitle);
    questionTitle.innerText = question;

    createButtons(container);
} else {
    completionScreen();
    }
}

function createButtons(container) {
    var questionId = document.createAttribute("id");
      questionId.value = "answers";
    var buttonDiv = document.createElement("div");
    var buttonContainer = container.appendChild(buttonDiv);
      buttonContainer.setAttributeNode(questionId);

        var count = 0
        for(var i = 0; i < answers.length; i++){
          var buttons = document.createElement("button");
          buttonContainer.appendChild(buttons);
          buttons.innerText = answers[i].answer;
          buttons.id = "answerbutton" + count;
          buttons.classList.add(answers[i].correct);
          buttons.onclick = correctness;
          count++
        }
}

function correctness() {
     if (this.className === "correct"){
        alert("Correct!");
    } else { 
        alert("Incorrect")
        wrongCount++;
  }
  questionCount++
    getQuestion();
    pickQuestion();
}


function completionScreen(){
  var container = document.getElementById("testcontainer");
    container.innerHTML = "";
  var resultsId = document.createAttribute("id");
    resultsId.value = "results";
  var resultsDiv = document.createElement("div");
  var resultsContainer = container.appendChild(resultsDiv);
    resultsContainer.setAttributeNode(resultsId);    
  var resultHeader =  document.createElement("h2")
    resultsContainer.appendChild(resultHeader)

    if (wrongCount < 4) {
        resultHeader.innerText = "You Passed";
        } else {
        resultHeader.innerText = "You Failed";
        }

var right = 10 - wrongCount;
var wrong = wrongCount;
percent = (right / 10) * 100;

var resultsRight = document.createElement("p");
    resultsContainer.appendChild(resultsRight);
    resultsRight.innerText = "You got " + right + " questions correct!";

var resultsWrong = document.createElement("p");
    resultsContainer.appendChild(resultsWrong);
    resultsWrong.innerText = "you got " + wrong + " questions wrong!";

var resultsPercent = document.createElement("p");
    resultsContainer.appendChild(resultsPercent);
    resultsPercent.innerText = "Your score is " + percent + "%";

var retry = document.createElement("button");
    resultsContainer.appendChild(retry);
    retry.innerText = "Play Again";
    retry.onclick = restart;

  getData();
}

function restart(){
  location.reload();
}

function start() {
    getQuestion();
    pickQuestion();
}

function pickQuestion() {
    question = questions[questionCount]
    answers = answers[questionCount]
    question = questions[questionCount]
    createQuestionElements(question, answers);
}
```
\$\endgroup\$
1
\$\begingroup\$

Some notes on readability

I will focus on method getQuestion. The first thing I noticed is that your indentation is very confusing. You start with an outer loop, and it looks it contains only 2 statements. However, the entire remaining method body is part of that loop. This pop in indentation introduced at var characters should never have been there.

for(var i = 0; i < questionsdata.length; i++){
        questions[i] = questionsdata[i].question;
        incorrect[i] = questionsdata[i].incorrect_answers;

    var characters = ['&amp;;', '&quot;', // .. and so on
    var actual =  ['&', '"', "'", "'", '"', '"', 'é', '-', 'Ü', 'Á', 'á'];
    // .. other code

To increase readaibility, you migth also want to introduce some whitespace here and there:

for (var i = 0; i < questionsdata.length; i++) {

rather than

for(var i = 0; i < questionsdata.length; i++){

But I would use a different syntax than the for (;;) pattern altogether. You have statements deep in the method body that use indexes from both an inner and the outer loop:

correct[i].replace(new RegExp(characters[j], 'g'), actual[j])

I'd have to scroll up to see what j and i represent.

A more readable option is to use the for (. of .) pattern. This would avoid the indexes.

You'd get:

for (const question of questionsdata) {

as opposed to:

for (var i = 0; i < questionsdata.length; i++) {

You'd have to refactor some variables though, no longer to use the indexes:

incorrect[i][j] = // ..

To avoid verbose loops you should check out some built-in methods, they might do the trick for you (forEach, filter, map, reduce, ..). I believe many of your inner loops could be rewritten in this style.

You should also be careful using var, it has a scope broader than the current block, unlike let and const. This could introduce unwanted behavior if you don't take the scope into account.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you so much for the insight! I have heard reducing the number of loops and if statement is code is advisable so moving to .filter,reduce etc is something I've been meaning to work on. \$\endgroup\$ – millies1919 Sep 3 '19 at 1:44
1
\$\begingroup\$

I agree with the advice in dfhwze's answer. Because features like arrow functions are used, for...of loops could be used to simplify the loop syntax. And instead of pushing values into questions and incorrect using i, Array.push() could be used.

I see that getData() is called at the end of completionScreen(), which is called at the end of createQuestionElements() if questionCount is not less than 10, and that function is called at the end of pickQuestion(). I haven't seen this code in action but fear there could be a timing issue - i.e. the results might not be loaded from the API before other code attempts to load data from the questions array.

The second promise callback could be simplified to a one-line arrow function:

.then(data => questionsdata = data.results)

Though be aware that questionsdata would get returned and any subsequent promise would receive that value.

It looks like the for loops with the regexp replacements decode HTML entities. There are various other techniques for this. I noticed the first element of characters has two semi-colons at the end. Should it only have one?

Other ES-6 features could be used, like destructuring assignment for swapping variables without the need for a temporary variable.

Inside the shuffle() function the technique used to decrement currentIndex could be simplified to the decrement operator: --. In fact, that could be moved to the previous line after it is used:

randomIndex = Math.floor(Math.random() * currentIndex);
currentIndex -= 1;

could be simplified to:

randomIndex = Math.floor(Math.random() * currentIndex--);

The conditional for the while could be reduced to simply currentIndex because it is considered falsey

| improve this answer | |
\$\endgroup\$

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.