1
\$\begingroup\$

Particularly on the CSS page, in the #question_list id, you'll see I've put a transparent border. This is obviously pointless, but without that border, my footer moves up and down when the proceeding questions come in, due to their size. Any suggestions on what I can do to remove that border and have my footer stay put as it is? I didn't want to use fixed positioning.

I'm just looking for ways to optimize the code. Perhaps there's a better way to organize the HTML? I'm still relatively new to front-end web dev.

var number = 0;
var numCorrect = 0;
var totalQuestions = 15

$(function() {
	loadQuestion();
});

function loadQuestion() {
	$("#question" + number).remove();
	$("span").hide();
	if (number == totalQuestions) {
		showScore();
	}
	
	number++;
	$("#question" + number).fadeIn({queue: false, duration: 1500});
	if (number % 2 == 0) {
		$("#question" + number).css("float", "right");
		$("#question" + number).css("margin-top", "75px");
		$("#question" + number).animate({marginRight:'31%'}, 1000);
	} else {
		$("#question" + number).animate({marginLeft:'29%'}, 1000);
	}
	$("#question" + number).css("position", "relative");
}

function retake() {
	location.reload();
}
	
function showScore() {
	$("#review").show();
	$("#review").html("You got " + numCorrect + " out of 15 correct. <br/>Your score is " 
		+ Math.round((numCorrect / totalQuestions) * 100) + "% <br/><button onclick='retake();'>Retake Test</button>");
}
	
function displayCorrect() {
	$("span").text("Correct!");
	$("span").css("color", "#006600");
}

function displayWrong() {
	$("span").text("Wrong!");
	$("span").css("color", "#990000");
}

function checkAnswers() {
	$("span").show();
	var answerKey = {1 : "true", 2 : "true", 3 : "false", 4 : "d", 
		5 : "b", 6 : "b", 7 : "true", 8 : "false", 9 : "true", 10 : "true",
		11 : "true", 12 : "false", 13 : "true", 14 : "true", 15 : "true"};

	if ($("input:radio[value=" + answerKey[number] + "]:checked").val()) {
		numCorrect++;
		displayCorrect();
	} else {
		displayWrong();
	}
	$("#answers" + number).remove();
	$("#continue" + number).css("display", "block");	
}
@font-face {
	font-family:'Safety Font';
	src: url(Oswald-Medium.ttf);
}

h2 {
	text-align:center;
}

body {
	font-family: 'Safety Font';
	background-color:#7C7C53;
	color: #333300;
	font-size:1.5em;
}

.correct, .wrong {
	display:none;
}

#question_list {
	height:600px;
	border:transparent solid 1px;
}

#question1, 
#question2, 
#question3, 
#question4, 
#question5, 
#question6, 
#question7, 
#question8, 
#question9,
#question10,
#question11,
#question12,
#question13,
#question14,
#question15 {
	width:40%;
	margin-top:100px;
	display:none;
}

#continue1,
#continue2,
#continue3,
#continue4,
#continue5,
#continue6,
#continue7,
#continue8,
#continue9,
#continue10,
#continue11,
#continue12,
#continue13,
#continue14,
#continue15 {
	display:none;
	margin-top:50px;
}

#review {
	text-align:center;
	margin-top:100px;
	display:none;
}

input[type=radio] {
	height:1.3em;
	width:1.3em;
}

button {
	font-family:'Safety Font';
	font-size:0.7em;
	margin-top:20px;
	height:30px;
	width:150px;
	color:#333300;
}

#footer {
	position:relative;
	bottom:0px;
	height:70px;
	width:100%;
	font-size:0.7em;
	text-align:center;
}

a {
	text-decoration:none;
}

a:visited, a:link {
	color:#333300;
	font-style:italic;
}
    <!DOCTYPE html>

<html lang="en">
	<head>	
		<meta charset="UTF-8"/>
		<title>California Firearm Safety Certificate - Practice Test</title>
		<link rel="stylesheet" type="text/css" href="firearm_safety.css"/>
		<script src="http://code.jquery.com/jquery-1.10.2.min.js"></script>
		<script type="text/javascript" src="firearm_safety.js"></script>	
	</head>
	
	<body>
		<h2>California Firearm Safety Certificate - Practice Test</h2>

			<div id="question_list">
				<div id="question1">
					<p>A safe practice when handling a gun is to rest your finger on the outside 
					of the trigger guard or along the side of the gun until you are ready to shoot. 
					(page 9) </p>
					
					<div id="answers1">
						<input type="radio" name="question1" value="true">True <br/>
						<input type="radio" name="question1" value="false">False <br/><br/>
						<button onclick="checkAnswers();">Check Answer</button>
					</div>
					<span></span>
					<button id="continue1" onclick="loadQuestion();">Continue</button>
				</div>
				
			
				<div id="question2">
					<p>To "know your target, its surroundings and beyond," you must consider that 
					if the bullet misses or completely passes through the target, it could strike 
					a person or object. (page 10)</p>
					
					<div id="answers2">
						<input type="radio" name="question2" value="true">True <br/>
						<input type="radio" name="question2" value="false">False <br/><br/>
						<button onclick="checkAnswers();">Check Answer</button>
					</div>
					
					<span></span>
					<button id="continue2" onclick="loadQuestion();">Continue</button>
				</div>
			
				<div id="question3">
					<p>Drinking alcohol while handling firearms is safe if your blood alcohol level remains
					below the legal limit. (page 11)</p>
					
					<div id="answers3">
						<input type="radio" name="question3" value="true">True <br/>
						<input type="radio" name="question3" value="false">False <br/><br/>
						<button onclick="checkAnswers();">Check Answer</button>
					</div>
					
					<span></span>
					<button id="continue3" onclick="loadQuestion();">Continue</button>
				</div>
			
				<div id="question4">
					<p>Which of the following safety points should you remember when handling a gun? (page 11)</p>
					<div id="answers4">
						<input type="radio" name="question4" value="a">Never shoot a gun in celebration. <br/>
						<input type="radio" name="question4" value="b">Do not fire at water, flat or hard surfaces. <br/>
						<input type="radio" name="question4" value="c">Wear ear and eye protection when shooting a gun. <br/>
						<input type="radio" name="question4" value="d">All of the above. <br/><br/>
						<button onclick="checkAnswers();">Check Answer</button>
					</div>
					
					<span></span>
					<button id="continue4" onclick="loadQuestion();">Continue</button>
				</div>
		
				<div id="question5">
					<p>Safety Rule Number Two is keep the gun pointed: (page 9)</p>
					<div id="answers5">
						<input type="radio" name="question5" value="a">To the north.<br/>
						<input type="radio" name="question5" value="b">In the safest possible direction. <br/>
						<input type="radio" name="question5" value="c">Up.<br/>
						<input type="radio" name="question5" value="d">Down.<br/><br/>
						<button onclick="checkAnswers();">Check Answer</button>
					</div>

					<span></span>
					<button id="continue5" onclick="loadQuestion();">Continue</button>
				</div>
			
				<div id="question6">
					<p>Safety Rule Number Five is know how to properly: (page 10)</p>
					<div id="answers6">
						<input type="radio" name="question6" value="a">Clear a malfunction. <br/>
						<input type="radio" name="question6" value="b">Operate your gun.<br/>
						<input type="radio" name="question6" value="c">Load your gun.<br/>
						<input type="radio" name="question6" value="d">Clean your gun.<br/><br/>
						<button onclick="checkAnswers();">Check Answer</button>
					</div>
					
					<span></span>
					<button id="continue6" onclick="loadQuestion();">Continue</button>
				</div>

				<div id="question7">
					<p>When not using a gun, use a firearms safety device and store the gun
					in a locked container. (page 13)</p>
					<div id="answers7">
						<input type="radio" name="question7" value="true">True <br/>
						<input type="radio" name="question7" value="false">False <br/><br/>
						<button onclick="checkAnswers();">Check Answer</button>
					</div>
					
					<span></span>
					<button id="continue7" onclick="loadQuestion();">Continue</button>
				</div>
			
				<div id="question8">
					<p>Toddlers lack the strength to pull the trigger of a firearm. (page 8)</p>
					<div id="answers8">
						<input type="radio" name="question8" value="true">True <br/>
						<input type="radio" name="question8" value="false">False <br/><br/>
						<button onclick="checkAnswers();">Check Answer</button>
					</div>
					
					<span></span>
					<button id="continue8" onclick="loadQuestion();">Continue</button>
				</div>

				<div id="question9">
					<p>You may face misdemeanor or felony charges if you keep a loaded firearm 
					where a child obtains and improperly uses it. (page 8)</p>
					<div id="answers9">
						<input type="radio" name="question9" value="true">True <br/>
						<input type="radio" name="question9" value="false">False <br/><br/>
						<button onclick="checkAnswers();">Check Answer</button>
					</div>
					
					<span></span>
					<button id="continue9" onclick="loadQuestion();">Continue</button>
				</div>

				<div id="question10">
					<p>There is no such thing as being too careful with children and guns. (page 8)</p>
					<div id="answers10">
						<input type="radio" name="question10" value="true">True <br/>
						<input type="radio" name="question10" value="false">False <br/><br/>
						<button onclick="checkAnswers();">Check Answer</button>
					</div>
					
					<span></span>
					<button id="continue10" onclick="loadQuestion();">Continue</button>
				</div>

				<div id="question11">
					<p>An important lesson children should learn is that guns are not toys. (page 9)</p>
					<div id="answers11">
						<input type="radio" name="question11" value="true">True <br/>
						<input type="radio" name="question11" value="false">False <br/><br/>
						<button onclick="checkAnswers();">Check Answer</button>
					</div>
					
					<span></span>
					<button id="continue11" onclick="loadQuestion();">Continue</button>
				</div>
			
				<div id="question12">
					<p>Child safety precautions only apply if you have children. (page 8)</p>
					<div id="answers12">
						<input type="radio" name="question12" value="true">True <br/>
						<input type="radio" name="question12" value="false">False <br/><br/>
						<button onclick="checkAnswers();">Check Answer</button>
					</div>
					
					<span></span>
					<button id="continue12" onclick="loadQuestion();">Continue</button>
				</div>
				
				<div id="question13">
					<p>The importance of the “dangerous range” is that a bullet can travel far 
					beyond the intended target. (page 27)</p>
					<div id="answers13">
						<input type="radio" name="question13" value="true">True <br/>
						<input type="radio" name="question13" value="false">False <br/><br/>
						<button onclick="checkAnswers();">Check Answer</button>
					</div>
					
					<span></span>
					<button id="continue13" onclick="loadQuestion();">Continue</button>
				</div>
				
				<div id="question14">
					<p>The safety on a semiautomatic pistol is not foolproof. (page 17)</p>
					<div id="answers14">
						<input type="radio" name="question14" value="true">True <br/>
						<input type="radio" name="question14" value="false">False <br/><br/>
						<button onclick="checkAnswers();">Check Answer</button>
					</div>
					
					<span></span>
					<button id="continue14" onclick="loadQuestion();">Continue</button>
				</div>
				
				<div id="question15">
					<p>Just because a cartridge fits into your firearm does not necessarily 
					mean it is safe to shoot. (page 26)</p>
					<div id="answers15">
						<input type="radio" name="question15" value="true">True <br/>
						<input type="radio" name="question15" value="false">False <br/><br/>
						<button onclick="checkAnswers();">Check Answer</button>
					</div>
					
					<span></span>
					<button id="continue15" onclick="loadQuestion();">Continue</button>
				</div>
				
				<div id="review">
					<button onclick="retake();">Retake Test</button>
				</div>
			</div>
			
			<div id="footer">
				<p>Copyright &copy; 2015 Adam Romero.<br/>
				<a href="http://github.com/adamromero/firearm-safety-test" target="_blank"/>Source code</a></p>
			</div>
			
	</body>
</html>

\$\endgroup\$
  • \$\begingroup\$ Your indentation is way too big - makes it harder to read the code and forces the reader to scroll horizontally. 4 characters is the standard! \$\endgroup\$ – Dmitri Zaitsev Apr 6 '15 at 3:18
  • \$\begingroup\$ @DmitriZaitsev There's nothing wrong with the indentation. The OP has graciously, consistently, and correctly used the tab character for indentation, which means you're free to configure your editor to represent it however you prefer. Anyone who insists that any specific number of spaces should be used is in fact wrong. \$\endgroup\$ – cimmanon Apr 6 '15 at 17:19
  • \$\begingroup\$ @cimmanon How can I configure my editor to view the code inside this site??? The lines are pushed far away to the right and I am forced to scroll horizontally back and forth. Can I change that in the browser? \$\endgroup\$ – Dmitri Zaitsev Apr 7 '15 at 2:55
1
\$\begingroup\$

I would avoid using hard-coded "magic" numbers inside your code, including ids like #question7 etc. The main question to ask:

  • Are these question numbers engraved in stone? Or will you want the freedom to insert/delete new questions?

If the freedom wins, any of these numbers is a recipe for future bugs.

And even if the numbers are engraved in stone, every marking element needs a reason to be there. So what is the reason for id=question7?

  • Is this to mark it as "question7" for the code reviewer? Then it should be a data attribute. Also note that question-7 is more readable than question7.

  • Is it to be used for styling? Is your styling uniform for all questions? Then you have to use a single class like question. If it is not uniform, use special classes like question-indented. This way your class is decoupled from the HTML meta-data, and has the dedicated styling purpose.

Warning. Keep in mind using id inside HTML, that your code becomes invalid as soon as those ids are not unique. This can easily happen by copy-paste and forgetting to change the id. Unless you use reliable validators, and consider this a feature to help your validators, I would avoid using ids whatsoever. Styling can be done merely with classes and is a recommended way.


This hard-coded answer hash is in the middle of your code is hard to spot and maintain:

var answerKey = {1 : "true", 2 : "true", 3 : "false", 4 : "d", 
    5 : "b", 6 : "b", 7 : "true", 8 : "false", 9 : "true", 10 : "true",
    11 : "true", 12 : "false", 13 : "true", 14 : "true", 15 : "true"};

It should really belong to your data object array like this:

questionsData = [
    {
        "question": "... question ...",
        "answer": true
    },
    {
         ...
    }
]

This way your data are completely separate and decoupled from the code. You can edit, add, delete your questions without making a single change in your code. Also you don't need to number them as those can change but your code won't break.

\$\endgroup\$
  • \$\begingroup\$ I'm not familiar with the data object syntax you used. I thought objects were created like this: var car = {type:"Fiat", model:500, color:"white"}; I like the idea of decoupling the questions and answers from the html. However, where would you put the multiple choice options? Would that stay in the html code? \$\endgroup\$ – Atom Apr 7 '15 at 7:12
  • \$\begingroup\$ I have edited it to "data object array" (though in JavaScript array is an object!). This is JS array, so you have to put it in a script. Then frameworks like AngularJS make it very easy to populate HTML with your data and update instantly on any change of data. \$\endgroup\$ – Dmitri Zaitsev Apr 7 '15 at 9:59
0
\$\begingroup\$

CSS and HTML

Using a font named "Oswald" for a quiz on gun safety seems kind of ironic.

A selector like #question1, #question2, #question3, #question4, #question5, #question6, #question7, #question8, #question9, #question10, #question11, #question12, #question13, #question14, #question15 should be smarter. The same goes for your #continueN selector.

One solution is to use the child selector #question_list > div.

Another is to write the selector as .question, and explicitly mark each of the questions as belonging to the "question" class:

<div class="question" id="question1">
  …
</div>

However, the question list is semantically an ordered list (<ol>). You should be using

<ol class="question_list">
  <li id="question1">
    <p>A safe practice…</p>
    <div id="answers1">
      …
    </div>
    <span></span>
    <button class="continue" onclick="loadQuestion();">Continue</button>
  </li>

  <li id="question2">
    …
  </li>
  …
</ol>

Radio buttons should always have their text enclosed in <label>s, so that the user can activate the button by clicking on the text.

label { display: block; }
<p>With <code>label</code> elements, text is clickable:</p>
<div id="answers1">
  <label><input type="radio" name="question1" value="true">True</label>
  <label><input type="radio" name="question1" value="false">False</label>
</div>

<p>Without <code>label</code> elements, text is not clickable:</p>
<div id="answers1">
  <input type="radio" name="question1" value="true">True<br>
  <input type="radio" name="question1" value="false">False
</div>

JavaScript

You have the questions and choices in HTML, and the answers stored in JavaScript. Maintenance would be easier if everything were in one place or the other. Personally, I would put all of the questions and answers in the JavaScript file, as it makes it slightly harder to cheat by using "View Source". You could also avoid hard-coding 15 in your code (which you did twice).

\$\endgroup\$
0
\$\begingroup\$

Markup

The br element is being misused here. You have a list of answers, which means you should be using a list element to contain them. If you want to increase the space between elements, use CSS.

The fieldset element would be more semantically appropriate than a div.

JavaScript

You have an excessive number of event handlers when one will do. You should be using event delegation instead. In addition to improved performance, it would reduce your dependency on ids.

You should not be hard coding styling information via JavaScript, this just adds an unnecessary maintenance cost. Instead, you should be modifying a class.

You should not need multiple buttons to go to the next question. A single button at the end (or beginning) should be sufficient. You'll just need to add a variable to keep track of which question is active so that you can efficiently move to the next one.

// pseudo code
var currentQuestion = 1;
var questions = document.getElementByTagName('fieldset');

loadQuestion() {
    hide(questions[currentQuestion]);
    currentQuestion++;
    show(questions[currentQuestion]);
}
\$\endgroup\$

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.