patternjavascriptModerate
Human anatomy quiz
Viewed 0 times
humananatomyquiz
Problem
My code works fine. I just want to have your insight and opinions about the way I did it. For example, is it easy to maintain? Does it have good architecture and design?
Quizz
var quiz = [{
"question": "What is function of respiratory system?",
"choices": ["help u gain nutrients", "to filter and eliminate wastes", "to bring the oxygens into the body and expel carbon dioxide"],
"answer": "to bring the oxygens into the body and expel carbon dioxide"
}, {
"question": "What body system is responsible for protections of body tissues and enabling human body movements",
"choices": ["Skeleton System", "Digestive system", "Circular system"],
"answer": "Skeleton System"
}, {
"question": "How many body systems are there in total",
"choices": ["4", "5", "6"],
"answer": "6"
}, {
"question": "What system is responsible for bringing energy to the body for cell growth and repair?",
"choices": ["Digestive", "Respiratory", "Circulatory"],
"answer": "Digestive"
}];
var currentQuestion = 0,
score = 0;
function loadquestions(){
var choices = quiz[currentQuestion].choices,
choicesHtml = "";
for (var i = 0; i " + choices[i] + "
";
}
document.getElementById('questions').textContent = "Q" + (currentQuestion + 1) + ". " +quiz[currentQuestion].question;
// load the choices
document.getElementById('choices').innerHTML = choicesHtml;
}
function checkanswers(){
var userpick='';
var pick=document.getElementsByName('quiz'+currentQuestion);
alert(pick.length);
for (var i = 0; i
Submit
Solution
-
I'm not a fan at all of the mixed spacing and indentation. I'm hopeful (but doubtful) that this is due to something like mixed spaces and tabs. I can't easily skim your code, because you have distinct levels of indentation matching up unfortunately well.
-
I'm usually a fan of wrapping code in an IIFE, but it's not a huge deal for a small app. You might want to look into it anyway.
-
The kind of variable declaration you use is potentially dangerous. If you try to add something in the middle and add a semicolon instead of a comma, you'll be introducing a new global. In this case, it's not that relevant since youre using globals anyway, but you might want to think about it.
-
Convention dictates function names be in camelCase. It should be
-
On that note,
-
In
-
Personally, I'm not a fan of replacing the Check Answers and Next Question buttons with each other. Instead, it might be better to have both buttons, hiding one at a time. Then you can bind the events once instead of setting an onclick every time, which is somewhat inelegant.
-
You have a rather significant amount of vertical whitespace that serves no purpose, particularly in your HTML.
I'm not a fan at all of the mixed spacing and indentation. I'm hopeful (but doubtful) that this is due to something like mixed spaces and tabs. I can't easily skim your code, because you have distinct levels of indentation matching up unfortunately well.
-
I'm usually a fan of wrapping code in an IIFE, but it's not a huge deal for a small app. You might want to look into it anyway.
-
The kind of variable declaration you use is potentially dangerous. If you try to add something in the middle and add a semicolon instead of a comma, you'll be introducing a new global. In this case, it's not that relevant since youre using globals anyway, but you might want to think about it.
var x = 0, // dangerous! if I add "z;" after this, y becomes a global
y = 0;
// prefer:
var x = 0, y = 0;
// or:
var x = 0;
var y = 0;-
Convention dictates function names be in camelCase. It should be
loadQuestions().-
On that note,
loadQuestions() is a bad name; it implies to me that you're loading all the questions, when in fact you're only putting the current question onto the page.-
In
loadQuestions(), you're generating an ` followed by a . For improved useability, consider setting a for attribute on the , or wrapping the inside the . (This will let the user click the text to select the option, rather than just the radio button.)
-
You're using jQuery. Rather than looping through the radio buttons to find which is checked, you can get this easily with something like $("[name=quiz" + currentQuestion + "]:checked").val(). Furthermore, rather than document.getElementById("answers"), you can use $("#answers") and the like — the performance difference here should not be a big concern.
-
You're duplicating your $("#check").replaceWith ... code — you have it in both an if and its corresponding else`. Just move it out of the conditional blocks.-
Personally, I'm not a fan of replacing the Check Answers and Next Question buttons with each other. Instead, it might be better to have both buttons, hiding one at a time. Then you can bind the events once instead of setting an onclick every time, which is somewhat inelegant.
-
You have a rather significant amount of vertical whitespace that serves no purpose, particularly in your HTML.
Code Snippets
var x = 0, // dangerous! if I add "z;" after this, y becomes a global
y = 0;
// prefer:
var x = 0, y = 0;
// or:
var x = 0;
var y = 0;Context
StackExchange Code Review Q#68531, answer score: 13
Revisions (0)
No revisions yet.