Multiple-choice quiz

Question

I have been working on improving my javascript skills, mainly "namespacing" and the best practices used by js developers. I would like to know how I can improve this code and any tips that may help me along the way. Below is the specific code I am working with and there is a working fiddler also.

Just to note, I only want to use straight javascript and not jQuery for this.

var quiz = window.quiz || {};
(function(ns) {
var currentIndex = 0;
var parent, back, next;
var selectedChoices = [];
ns.version = '1.0.0';
ns.author = 'Anonymous Hands';
ns.parentId = function(id) { parent = document.getElementById(id); };
ns.nextButtonId = function(id) { next = document.getElementById(id); next.onclick = moveNext; };
ns.backButtonId = function(id) { back = document.getElementById(id); back.onclick = moveBack; };
ns.questions = [];
ns.render = 
 function() {
 renderQuestion(0);
 };
//Pass results and missed questions?
ns.onComplete = function(results){};
function moveNext(){
 saveState();
 if (!atEnd()){
 currentIndex++;
 renderQuestion(currentIndex);
 restoreState();
 return;
 }
 renderComplete();
}
function moveBack(){
 saveState();
 if(!atBeginning()){
 currentIndex--;
 renderQuestion(currentIndex);
 restoreState();
 }
}
function renderQuestion(index){
 clearParent();
 var questionNum = index + 1;
 var node = ns.questions[index];
 var questionNumDiv = makeElement({type: "div", class: "question-number" ,html: questionNum});
 var questionText = makeElement({type: "div", class: "question-text" ,html: node.question});
 var choicesDiv = makeElement({type: "div", class: "choices"});
 var l = node.choices.length;
 for(var i = 0; i < l; i++){
 var choiceRadio = makeElement({
 type: "input", 
 subtype: "radio", 
 name: "choices", 
 id: "choice-" + i
 });
 var choiceLabel = makeElement({type: "label", class: "choice", html: node.choices[i], for: "choice-" + i});
 choicesDiv.appendChild(choiceRadio);
 choicesDiv.appendChild(choiceLabel);
 }
 parent.appendChild(questionNumDiv);
 parent.appendChild(questionText);
 parent.appendChild(choicesDiv);
}
function renderComplete(){
 clearParent();
 var n = getTotalCorrect();
 var d = ns.questions.length;
 var p = Math.round(((n / d) * 100))
 var totalDiv = makeElement({type: "div", id: "score-total", html: "Your score: " + n + "/" + d});
 var percentDiv = makeElement({type: "div", id: "score-percentage", html: p + "%"});
 parent.appendChild(totalDiv);
 parent.appendChild(percentDiv);
 back.style.display = "none";
 next.style.display = "none";
 //onComplete();
}
function saveState(){
 var anwser = selectedChoices[currentIndex];
 var userChoice = getUserChoice();
 if(anwser){
 anwser.choice = userChoice;
 return;
 }
 selectedChoices.push({questionId: currentIndex, choice: userChoice});
}
function restoreState(){
 var anwser = selectedChoices[currentIndex];
 if(anwser && anwser.choice != null){
 var choices = document.getElementsByName("choices");
 choices[anwser.choice].checked = true;
 }
}
function clearParent(){
 parent.innerHTML = "";
}
function atBeginning(){
 return (currentIndex <= 0) ? true : false;
}
function atEnd(){
 return (currentIndex >= ns.questions.length - 1) ? true : false;
}
function getTotalCorrect(){
 var correct = 0;
 var l = ns.questions.length;
 for(var i = 0; i < l; i++){
 if (ns.questions[i].anwser === selectedChoices[i].choice) correct++;
 }
 return correct;
}
function makeElement(p){
 var e = document.createElement(p.type);
 if (p.id) e.id = p.id;
 if (p.class) e.className = p.class;
 if (p.name) e.name = p.name;
 if (p.html) e.innerHTML = p.html;
 if (p.for) e.setAttribute("for", p.for);
 if (e.type) e.type = p.subtype;
 return e;
}
function getUserChoice(){
 var choices = document.getElementsByName("choices");
 var l = choices.length;
 var index = null;
 for(var i = 0; i < l; i++){
 var node = choices[i];
 if (node.checked) index = i;
 }
 return index;
}
})(quiz);

The jsfiddle, as promised.

• Hugo Dozois: Just a little comment on the UX. When you are at the first or last question, you should disable the back/next button. It looks weird that the button has an hover and can be clicked but doesn't do anything. Also, make the labels clickable.
• Justin: @HugoDozois I agree on the buttons in addition next should turn to "Finish" when the user is on the last question. I am really looking for guidance on the structure of the code and ways to improve the overall code.
• Stuart: a small point: return (currentIndex <= 0) ? true : false; can simply be replaced by return currentIndex <= 0;
• SylvainD: It might not seem useful to you but the rules on CR is to post your full code here and not a link to it. (I can't find a link to the explanation if required)
• Winston Ewert: You must post your code in your question, otherwise I'll be forced to close your question.

Answer 1

I tend to use a lot of Java best practices in my JavaScript code. The biggest issue I see is the lack of extensible classes/methods. The methods are procedural and only seem to be broken into new methods because they are repeatable. The class as a whole violates the single responsibility principle because it has to handle the entire widget from modifying input (also bad practice), parsing the input, managing the question state, and appending the view. By following SRP your code will become much easier to read, maintain, and extend. I'm not going to comment on your grammatical issues but you should definitely set up Sonar to scan your code if you haven't already.

So how it could be improved?

I would start be making a new class called Question that could take a single question model and display it on the page. Then create a Questionnaire class that can take a questionnaire model (metadata & list of questions) and handle the state transitions until all questions have been answered. Questionnaire will have to instantiate a new Question object inside a div.

I create the classes this way which allows the class to maintain scope.

function Question(divId, question) {
 // private variables
 var myVar;
 // this is a public method
 this.getAnswer = function() {
 }
 // this is a private method
 function renderChoice() {
 }
 function init() {
 // build the view
 }
 // this method call initializes the class like a constructor
 // so the caller doesn't have to initialize
 init();
}

There are then two ways the state transitions could be handled. POST the answer after every question and store the values at the server or store each question result in the Questionnaire and POST at the end. Don't store the answers in the code because anyone that knows how to access and read the code could cheat. Either way you choose make sure that each class knows as little about each other as possible. Question should know nothing about Questionnaire and Questionnaire should only know that is has a list of questions to loop through and use accessor methods to get the value from.

• Justin: I accepted your answer as you are the only one to actually provide feedback, which I appreciate greatly. Have you ever had any issues with this approach? I ask because since js isn't strongly typed, I don't see a really good way to enforce the use of the Question "class". I rarely see the usage of the new keyword in js these days which seems to hint that it is fluff and makes the user have to type more to accomplish the same thing.
• Brian Blain: I agree that new is not used much. I would attribute that to most frameworks becoming more like utilities that can register types for DOM parsing and attaching events. The "classes" remove functions from the global namespace so there is no collision/overwriting and allow a MVP pattern. Data models are still in JSON. Usually I don't use more than one presenter class because reusable components of a page can become a JSP tag or custom widget. The classes are defined in a .js while the variable is defined in the .html script tag.