5
\$\begingroup\$

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.

\$\endgroup\$

migrated from stackoverflow.com Jun 26 '13 at 15:24

This question came from our site for professional and enthusiast programmers.

  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Hugo Dozois Jun 26 '13 at 16:47
  • \$\begingroup\$ @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. \$\endgroup\$ – Justin Jun 26 '13 at 17:49
  • 1
    \$\begingroup\$ a small point: return (currentIndex <= 0) ? true : false; can simply be replaced by return currentIndex <= 0; \$\endgroup\$ – Stuart Jun 26 '13 at 18:55
  • 2
    \$\begingroup\$ 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) \$\endgroup\$ – SylvainD Jun 26 '13 at 21:38
  • \$\begingroup\$ You must post your code in your question, otherwise I'll be forced to close your question. \$\endgroup\$ – Winston Ewert Jun 27 '13 at 2:01
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Justin Jul 9 '13 at 14:29
  • \$\begingroup\$ 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. \$\endgroup\$ – Brian Blain Jul 9 '13 at 19:17

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.