4
\$\begingroup\$

I've been teaching myself a bit of web design and development over the past month or so, coming from an CG/VFX background with no prior experience so it's been a bit alien to me to say the least.

I found the excellent Javascript is Sexy guide and have been going through it, but today was project time. I must admit I felt kinda stumped. I thought I was getting a good handle on it but then when the hand-holding stops, it gets a bit overwhwelming.

Anyway, I've done the 'quiz' assignment, but I have no idea if this is good or terrible, or on the right track.

http://jsfiddle.net/TeeJayEllis/3q8fs/

var totalScore = 0;
var questionNumber = 0;



var allQuestions = [{
        question: "Who is Prime Minister of the United Kingdom?",
        choices: ["Tony Blair", "Gordon Brown", "Winston Churchill", "David Cameron"],
        correctAnswer: "David Cameron"
    },
    {
        question: "What is the capital city of Spain?",
        choices: ["Barcelona", "London", "Madrid", "Lisbon"],
        correctAnswer: "Madrid"
    },
    {
        question: "How many strings does a guitar have?",
        choices: ["Three", "Four", "Five", "Six"],
        correctAnswer: "Six"
    },
    {
        question: "What year did MTV launch?",
        choices: ["1980", "1992", "1981", "1979"],
        correctAnswer: "1981"
    }
];


var correctGuess = function(i) {
	totalScore += 1;
	questionNumber ++;
	$('#questionDiv').fadeOut("slow");
	$('#mainContent').empty();
	$('#mainContent').append('<div id="answerDiv"></div>');
	$('#answerDiv').append('<h1>Correct!<h1>');
	$('#answerDiv').append('<h2>Total Score: ' + totalScore + '</h2>' );
	$('#answerDiv').fadeIn("slow");
	
	if (totalScore < 4) {
		$('#answerDiv').append('<button id="nextButton">Next Question</button>');
		$('#nextButton').click(function() {question(questionNumber);})
	} else {
		$('#answerDiv').append('<h1>Congratulations, you Win!');
		$('#answerDiv').append('<button id="restartButton">Play Again</button>');
		$('#restartButton').click(function() {
			questionNumber = 0;
			totalScore = 0;
			question(questionNumber);})
	}
};

var incorrectGuess = function(i) {
	totalScore = 0;
	questionNumber = 0;
	$('#questionDiv').fadeOut("slow");
	$('#mainContent').empty();
	$('#mainContent').append('<div id="answerDiv"></div>');
	$('#answerDiv').append('<h1>Wrong!<h1>');
	$('#answerDiv').fadeIn("slow");
	$('#answerDiv').append('<button id="restartButton">Restart</button>');

	$('#restartButton').click(function() {question(questionNumber);})
};


var question = function(i) {
	$('#questionDiv').fadeOut("slow");
	$('#mainContent').empty();
	$('#mainContent').append('<div id="questionDiv"></div>');
	$('#questionDiv').append('<h1>Question ' + (i + 1) + '<h1>');
	$('#questionDiv').append('<h2>' + allQuestions[i].question + '</h2>');
	$('#questionDiv').append('<input type="radio" name="questionChoices" value="' + allQuestions[i].choices[0] + '" checked="yes">' + allQuestions[i].choices[0] + '</input>');
	$('#questionDiv').append('<input type="radio" name="questionChoices" value="' + allQuestions[i].choices[1] + '">' + allQuestions[i].choices[1] + '</input>');
	$('#questionDiv').append('<input type="radio" name="questionChoices" value="' + allQuestions[i].choices[2] + '">' + allQuestions[i].choices[2] + '</input>');
	$('#questionDiv').append('<input type="radio" name="questionChoices" value="' + allQuestions[i].choices[3] + '">' + allQuestions[i].choices[3] + '</input>');
	$('#questionDiv').append('<button id="submitButton">Submit</button>');
	$('#questionDiv').fadeIn("slow");

	$('#submitButton').click(function() {
		if($('input:radio[name=questionChoices]:checked').val() === allQuestions[i].correctAnswer && i < 4) {
			correctGuess();
		} else {
			incorrectGuess();
		}
	})
};

question(questionNumber);
#mainContent {
	margin-top: 50px;
	padding-top: 100px;
	width: 600px;
	height: 300px;
	border: 5px black solid;
	margin-left: auto;
	margin-right: auto;
	text-align: center;
}

button {
	display: block;
	margin-left: auto;
	margin-right: auto;
	margin-top: 10px;
}

#questionDiv {
	display: none;
}

#answerDiv {
	display: none;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script>
<div id="mainContent"></div>

\$\endgroup\$
  • \$\begingroup\$ There is something one should understand when programming in client-side javascript: the DOM is slow. Awfully slow. Very very slow. You have to use the DOM the least possible times. 2 times is too much. And I'm serious, the horrible performance of the DOM is noticeable. \$\endgroup\$ – Florian Margaine Mar 24 '13 at 14:57
1
\$\begingroup\$

I've worked on you're code yo provide you with an actual code example. I've re-done most of the code to show you and I've explained it all with comments. Here's the link to the Fiddle and I'll be happy to answer any questions you might have.

Here's the code:

var totalScore = 0, //You only need to say 'var' once. If you want to declare more, just use a ',' between each one.
    questionNumber = 0,
    allQuestions = [{
        question: "Who is Prime Minister of the United Kingdom?",
        choices: ["Tony Blair", "Gordon Brown", "Winston Churchill", "David Cameron"],
        correctAnswer: "David Cameron"
    },
    {
        question: "What is the capital city of Spain?",
        choices: ["Barcelona", "London", "Madrid", "Lisbon"],
        correctAnswer: "Madrid"
    },
    {
        question: "How many strings does a guitar have?",
        choices: ["Three", "Four", "Five", "Six"],
        correctAnswer: "Six"
    },
    {
        question: "What year did MTV launch?",
        choices: ["1980", "1992", "1981", "1979"],
        correctAnswer: "1981"
    }
];

//As a rule of thumb, if you use a selection more than once you should cahce it. Like this:
var mainContent = $('#mainContent'); //Now we replace all the $('#mainContent') with a simple 'mainContent' reference.

function correctGuess (i) { //Name your functions the easy way.
    totalScore ++; //+=1 is the same thing as ++
    questionNumber ++;

    var updatePage = ['<div id="answerDiv">' +
        '<h1>Correct!<h1>' +
        '<h2>Total Score: ' + totalScore + '</h2></div>'
    ], // This is how you concatenate. We join all the items we want to put in first, then call a single append.
    whereToPut = updatePage[0].length -6; //We want to put it at the end of the text, but before the </div>

    if(totalScore < 4){
        var whatToPut = '<button id="nextButton">Next Question</button>';


        //Here we place the nextQuestion variable intro the updatePage variable at the end just before the closing div
        updatePage = [updatePage.slice(0, whereToPut), whatToPut, updatePage.slice(whereToPut)].join('');

        $('#mainContent').html(updatePage); //You don't need to empty out the div every time, just replace what is there using .html().
        //Notice how we use it only once.

        $('#nextButton').on('click', function() { //Using this .on() method, you save a function call because .click() calls the .on(), so you might as well jump straight there.
                question(questionNumber); //Don't do one liner functions
        });
    } else {
        //Same thing as before
        var whatToPut = '<h1>Congratulations, you Win!</h1><button id="restartButton">Play Again</button>';

        updatePage = [updatePage.slice(0, whereToPut), whatToPut, updatePage.slice(whereToPut)].join('');

        $('#mainContent').html(updatePage);

        $('#restartButton').on('click', function() {
            questionNumber = 0;
            totalScore = 0;
            question(questionNumber);
        });
    }

    $('#answerDiv').fadeIn("slow");
};

function incorrectGuess(i) {
    //Same as before, concatenate and append a single time.
    totalScore = 0;
    questionNumber = 0;

    $('#questionDiv').fadeOut("slow");

    var updatePage = ['<div id="answerDiv"></div>' +
        '<h1>Wrong!<h1>' +
        '<button id="restartButton">Restart</button>'
        ];

    $('#mainContent').html(updatePage);
    $('#answerDiv').fadeIn("slow");

    $('#restartButton').on('click', function() {
        question(questionNumber);
    });
};

function question(i) {
    $('#questionDiv').fadeOut("slow");
    mainContent.html('<div id="questionDiv">' +
        '<h1>Question ' + (i + 1) + '<h1>' +
        '<h2>' + allQuestions[i].question + '</h2>' +
        '<input type="radio" name="questionChoices" value="' + allQuestions[i].choices[0] + '" checked="yes">' + allQuestions[i].choices[0] + '</input>' +
        '<input type="radio" name="questionChoices" value="' + allQuestions[i].choices[1] + '">' + allQuestions[i].choices[1] + '</input>' +
        '<input type="radio" name="questionChoices" value="' + allQuestions[i].choices[2] + '">' + allQuestions[i].choices[2] + '</input>' +
        '<input type="radio" name="questionChoices" value="' + allQuestions[i].choices[3] + '">' + allQuestions[i].choices[3] + '</input>' +
        '<button id="submitButton">Submit</button>' +
        '</div>'
    );
    $('#questionDiv').fadeIn("slow");

    $('#submitButton').on('click', function() {
        if($('input:radio[name=questionChoices]:checked').val() === allQuestions[i].correctAnswer && i < 4) {
            correctGuess();
        } else {
            incorrectGuess();
        }
    });
};

question(questionNumber);
\$\endgroup\$
5
\$\begingroup\$

Overall, it's ok.

Several points though:

  • The DOM is slow. Really. Calling it ten times when it's possible to do it only once is awful.
  • To follow this, cache your selectors. $ is a function returning an object. And you're calling it every time you use $(arg).
  • Be consistent. Why do you use these two forms? totalScore += 1; questionNumber ++;. They're doing exactly the same, yet you use two different ways to do the same thing.
  • Why don't you declare your functions with the simple form? function correctGuess()... Your form doesn't change much in your case, but hoisting could hurt you later.
  • Don't do one-liners. $('#nextButton').click(function() {question(questionNumber);}) is impossible to read.
  • Why are you having an i argument to correctGuess and incorrectGuess? You're not using it at all.
\$\endgroup\$
  • \$\begingroup\$ Thanks, Florian. I didn't even know using the DOM so much was frowned upon! Complete beginner! So, what is a more appropriate alternative? I was just going by the assignment from the guide I was using which suggests generating a dynamic quiz with either JQuery or JavaScript. \$\endgroup\$ – Tom Ellis Mar 24 '13 at 20:12
  • \$\begingroup\$ @TomEllis Well, in your example, it's better to create a single string that you concatenate. This way, you have only one interaction with the DOM (when you'll html(str)). \$\endgroup\$ – Florian Margaine Mar 24 '13 at 20:22
  • \$\begingroup\$ Some reading on the .append() method you might want to check out: learningjquery.com/2009/03/… \$\endgroup\$ – Jonny Sooter Mar 24 '13 at 20:34
  • \$\begingroup\$ @TomEllis the DOM is not frowned upon. It's essential! But it's also well known that it's very slow, and must thus be handled specifically. \$\endgroup\$ – Florian Margaine Mar 25 '13 at 12:01
  • \$\begingroup\$ Ok thanks guys. I still don't really get it, I'm not even sure how to use the concatenation as you suggested. I think I'll hit the books a bit more! \$\endgroup\$ – Tom Ellis Mar 25 '13 at 14:24
1
\$\begingroup\$

One thing you should always consider is "namespacing" your code (basically putting all of your code into one object), or "hiding" it in an an anonymous function. This helps keeping the global namespace clean, which becomes relevant when you have other people's JavaScript in your page.

Regarding the DOM mentioned by the others: You should anyway keep the "separation of concerns" in mind and instead of building the HTML in your script (which is slow), put as much of the HTML into the, well, HTML as possible. Example:

<div id="questionDiv">
  <h1>Question <span></span><h1>
  <h2></h2>
  <input type="radio" value="" checked id="answer1"><label for="answer1"></label>
  <input type="radio" value="" id="answer2"><label for="answer2"></label>
  <input type="radio" value="" id="answer3"><label for="answer3"></label>
  <input type="radio" value="" id="answer4"><label for="answer4"></label>
  <button type="button">Submit</button>
</div>

And in the JavaScript:

var questionDiv = $('#questionDiv');
questionDiv.find('h1 > span').text(i + 1);
questionDiv.find('h2').text(allQuestions[i].question);
questionDiv.find('input[type=radio]').each(function(index) {
  var choice = allQuestions[i].choices[index];
  $(this).val(choice).next('label').text(choice);
});

You can do simularly with the right and wrong answer divs.

Also if you do this keep in mind you'll only need to assign the event handlers once and not for each question.

BTW, you are generating invalid HTML. There is not such thing as a closing </input> tag. Use a label as I did.

Also checked="yes" is wrong too. It's either checked="checked" or just checked.

Finally don't forget type="button" on the button, or it will be a real submit button that submits to the server.

\$\endgroup\$
  • \$\begingroup\$ In xhtml you are expected to close input tags, e.g. <input type="radio" value="" id="answer2" />. I think html 5 is flexible, you can self close or not - you are right about never using </input> though! \$\endgroup\$ – RobH Mar 26 '13 at 20:18

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.