HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Beginner's quiz with jQuery

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withbeginnerquizjquery

Problem

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('');
$('#answerDiv').append('Correct!');
$('#answerDiv').append('Total Score: ' + totalScore + '' );
$('#answerDiv').fadeIn("slow");

if (totalScore
#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;
}

`

Solution

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.

Context

StackExchange Code Review Q#24308, answer score: 5

Revisions (0)

No revisions yet.