1
\$\begingroup\$

I'm a teacher and I built a one-page web app to display random question sets for quizzes and tests. The script I wrote selects one question from a set (array) of questions.

The script works fine, but I wonder if it follows best practices or if it could be made more efficient. I want to share my app with other teachers, but I don't want to spread bad practices.

Here's a fiddle of the app itself.

Clicking anywhere on the page brings up a spinner. Clicking again brings up five new random questions. There is more JS in the app, but I'm mainly concerned about the JS related to the question randomization.

// change questions here -- in quotes, comma separated
var qset1 = new Array("Set 1 Question 1","Set 1 Question 2","Set 1 Question 3","Set 1 Question 4","Set 1 Question 5");

var qset2 = new Array("Set 2 Question 1","Set 2 Question 2","Set 2 Question 3","Set 2 Question 4","Set 2 Question 5");

var qset3 = new Array("Set 3 Question 1","Set 3 Question 2","Set 3 Question 3","Set 3 Question 4","Set 3 Question 5");

var qset4 = new Array("Set 4 Question 1","Set 4 Question 2","Set 4 Question 4","Set 4 Question 4","Set 4 Question 5");

var qset5 = new Array("Set 5 Question 1","Set 5 Question 2","Set 5 Question 5","Set 5 Question 4","Set 5 Question 5");



function setUP() {
var set1=Math.floor(Math.random()*qset1.length);
var set2=Math.floor(Math.random()*qset2.length);
var set3=Math.floor(Math.random()*qset3.length);
var set4=Math.floor(Math.random()*qset4.length);
var set5=Math.floor(Math.random()*qset5.length);


document.getElementById('set_1').innerHTML = qset1[set1];
document.getElementById('set_2').innerHTML = qset2[set2];
document.getElementById('set_3').innerHTML = qset3[set3];
document.getElementById('set_4').innerHTML = qset4[set4];
document.getElementById('set_5').innerHTML = qset5[set5];

}
\$\endgroup\$
  • \$\begingroup\$ How do you communicate to the server? \$\endgroup\$ – destoryer Aug 13 '16 at 12:24
  • \$\begingroup\$ And would you like to exclude questions that were already shown? \$\endgroup\$ – destoryer Aug 13 '16 at 12:24
3
\$\begingroup\$

I assume your sets won't actually look like that, and that was just for the sake of this example. Right now you're doing the same operation on a bunch of virtually identical things - this is exactly the use case for a for-loop.

First we need to put all of our questions into an array of arrays, and then we can loop over them and do whatever we want. I made a few other changes as well - by using the nth-child() selector I don't have to have the ids - you can switch between the two methods by changing which lines are commented out. You can run this code snippet and see how it works down below.

function populateQuestions() {
  var questionSets = [
    ["Set 1 Question 1", "Set 1 Question 2", "Set 1 Question 3", "Set 1 Question 4", "Set 1 Question 5"],
    ["Set 2 Question 1", "Set 2 Question 2", "Set 2 Question 3", "Set 2 Question 4", "Set 2 Question 5"],
    ["Set 3 Question 1", "Set 3 Question 2", "Set 3 Question 3", "Set 3 Question 4", "Set 3 Question 5"],
    ["Set 4 Question 1", "Set 4 Question 2", "Set 4 Question 4", "Set 4 Question 4", "Set 4 Question 5"],
    ["Set 5 Question 1", "Set 5 Question 2", "Set 5 Question 5", "Set 5 Question 4", "Set 5 Question 5"]
  ];

  for (var setIndex = 0; setIndex < questionSets.length; ++setIndex) {
    var questionSet = questionSets[setIndex];
    var questionIndex = Math.floor(Math.random() * questionSet.length);
    var question = questionSet[questionIndex];
    var selector = '#questions li:nth-child(' + (setIndex + 1).toString() + ')';
    //var setId = 'set_' + (setIndex + 1).toString();
    document.querySelector(selector).innerHTML = question;
    //document.getElementById(setId).innerHTML = question;
  }
}
<ul id="questions">
  <li id="set_1">First question</li>
  <li id="set_2">Second question</li>
  <li id="set_3">Third question</li>
  <li id="set_4">Fourth question</li>
  <li id="set_5">Fifth question</li>
</ul>
<button type=button onclick="populateQuestions()">Populate the questions!</button>

I do have one potential concern - right now you're still presenting the sets in the same order, but you might want to randomize what order the sets appear in. I'll leave that as an exercise to the reader, but it isn't much more complicated than this. You might also want to not present questions that have already been seen before, which is a little tricky. You either want to remove them from the sets once they've been used, or have an object to keep track of ones that have already been used.

Lastly, it might be worthwhile to write a function like getQuestions that looks like this

function getQuestions(callback) {
    // do something to get the questions
    var questions = ...;
    // once the questions are ready, use the callback
    callback(questions);
}

function populateQuestions(questionSets) {
    // same code, minus the creation of questionSets
}

getQuestions(populateQuestions);

We use the callback so this can become an asynchronous method - if you ever wanted to get this data via AJAX, or a web-service, or database, or wherever, it becomes much easier to integrate that into your workflow.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the detailed reply Dannnno! Both methods work great! Very helpful for learning about an array of arrays and for-loops. A few things: <I assume your sets won't actually look like that> That's right. <you might want to randomize what order the sets appear in> I don't need that for this app, but it's good to have in mind. <it might be worthwhile to write a function like getQuestions> This might be very useful in the future. I might build an app where teachers can enter questions via a form instead of having to edit code themselves. Thanks a lot! \$\endgroup\$ – Nic47 Aug 14 '16 at 3:31
1
\$\begingroup\$

Just like the other reviewer has mentioned, having an array of arrays is more preferable. Alternative to what @Dannnno suggested

  • I created a button with an onclick function that calls a function I created called displayQuestions e.g
    [qset1, qset2,qset3,qset4,qset5 ].forEach(displayQuestions);
    Note: The button populates the div with questions. Using Foreach in javascript is similar to using Foreach construct in other languages. It might be helpful to look at the construct for Foreach in javascript Array.prototype.forEach().
  • I added a shuffle method to randomise the arrays. Note this shuffle is aFisher-Yates Shuffle implementation but I used ES6 standard to swap the elements in a given array. You read more about this inHow can I shuffle an array in JavaScript? .
  • When you can transverse the parent node, then use a for..loop to change the innerHtml. For instance,
    var outerDiv = document.getElementById("sets");
    outerDiv.querySelector("#set_" + i).innerHTML= "blabla";

rather than

document.getElementById('set_1').innerHTML = qset1[set1];
document.getElementById('set_2').innerHTML = qset2[set2];
document.getElementById('set_3').innerHTML = qset3[set3];
document.getElementById('set_4').innerHTML = qset4[set4];
document.getElementById('set_5').innerHTML = qset5[set5];
  • Your implementation of randomisation allows some questions to be repeated frequently as each question have equal probability. To avoid this, each time an element/item/question is being displayed you have to update the array so its length is n-1. I have done this below. So you get unique questions on each button click. I'm not sure of what you want to do if all the questions have been displayed hence If you notice trying to get more questions when they've all been displayed or in technical terms when index>5 the div gets updated with undefined meaning the index has no assigned value. P.S I would suggest making the button blurry by assigning true to the disabled property of button element or displaying end of test or whatever you want to call it.

#sets div {
  font-family: Arial, Helvetica, sans-serif;
  font-size: 7vh;
  margin-top: 6vh;
  border: 1px solid gray;
  padding: 1vh;
  width: 100%;
  background-color: #ececff;
}
<body>
  <div id="sets">
    <div id="set_1">???</div>
    <div id="set_2">???</div>
    <div id="set_3">???</div>
    <div id="set_4">???</div>
    <div id="set_5">???</div>
  </div>

  <button id="myBtn" onclick="[qset1, qset2,qset3,qset4,qset5 ].forEach(displayQuestions);">Try it</button>


  <script>
    // qset3,qset4,qset5
    var qset1 = new Array("Set 1 Question 1", "Set 1 Question 2", "Set 1 Question 3", "Set 1 Question 4", "Set 1 Question 5");

    var qset2 = new Array("Set 2 Question 1", "Set 2 Question 2", "Set 2 Question 3", "Set 2 Question 4", "Set 2 Question 5");

    var qset3 = new Array("Set 3 Question 1", "Set 3 Question 2", "Set 3 Question 3", "Set 3 Question 4", "Set 3 Question 5");

    var qset4 = new Array("Set 4 Question 1", "Set 4 Question 2", "Set 4 Question 3", "Set 4 Question 4", "Set 4 Question 5");

    var qset5 = new Array("Set 5 Question 1", "Set 5 Question 2", "Set 5 Question 5", "Set 5 Question 4", "Set 5 Question 3");

    function shuffle(a) {
      var j, x, i;
      for (i = a.length; i; i--) {
        j = Math.floor(Math.random() * i);
        [a[i - 1], a[j]] = [a[j], a[i - 1]];
      }
      return a;
    }
    var outerDiv = document.getElementById("sets");

    function displayQuestions(element, index, array1) {

      var shuffleElements = shuffle(element);
      var i = index;
      i++;
      var randomIndex = Math.floor(Math.random() * element.length);
      outerDiv.querySelector("#set_" + i).innerHTML = shuffleElements[randomIndex];
      element.splice(randomIndex, 1);
    }
    
  </script>
</body>

I hope this helps. Cheers.

\$\endgroup\$
  • \$\begingroup\$ Thanks for this – the shuffle function is really helpful for learning! For the app in question, I don't need to eliminate questions (as every student should be prepared for any question). Nonetheless, now I can build other quiz apps using this technique – once I understand how it works. Thanks! \$\endgroup\$ – Nic47 Aug 14 '16 at 3:47
  • \$\begingroup\$ @Nic47 I'm glad you found it beneficial. Eliminating questions doesn't mean it never gets displayed what it does is if the first div displays "Set 1 Question 1","Set 2 Question 1","Set 3 Question 4", "Set 1 Question 5". It would be reasonable not to display "Set 1 Question 1","Set 2 Question 2","Set 3 Question 4", "Set 1 Question 3" because the question "Set 1 Question 1" has been displayed earlier except it's a bonus question. This happens when you randomise with replacement.I hope you understand. \$\endgroup\$ – Siobhan Aug 14 '16 at 5:04
  • \$\begingroup\$ Yes, "eliminate" in my reply was an imprecise word choice. I ran your snippet, and I see what it does. I understand your point about "when index>5." This snippet will definitely come in handy for other quiz apps. Thanks again! \$\endgroup\$ – Nic47 Aug 14 '16 at 10:50

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.