Now that we have a working example, the first thing that I’d do is to lint the code using BeautifyJS and JSLint which removes as many of the easy-to-notice issues as possible.
After linting, doing the least possible to get the code passing, that has fixed the first problem that I noticed. The second problem still exists though. Here is the linted code.
/*jslint browser */
/*global $ */
$(document).ready(function () {
var count = 0;
var guessArray;
var newArray;
var guess;
var n;
var score;
var z;
var correct;
var k;
var jumbled = [
{
question: "LTPOHSIA",
letters: ["A", "I", "S", "H", "O", "P", "T", "L"],
answer: "HOSPITAL"
},
{
question: "HMANSODE",
letters: ["M", "E", "H", "N", "D", "O", "A", "S"],
answer: "HANDSOME"
},
{
question: "ADITNOID",
letters: ["A", "I", "D", "O", "N", "T", "I", "D"],
answer: "ADDITION"
},
{
question: "BSNISUES",
letters: ["S", "E", "U", "S", "I", "N", "S", "B"],
answer: "BUSINESS"
},
{
question: "XEREESIC",
letters: ["X", "E", "R", "E", "S", "I", "C", "E"],
answer: "EXERCISE"
}
];
// Showing letters in divs
function showQuestions() {
document.getElementById("questionDiv").innerHTML = x;
jumbled[i].letters.forEach(function (letter) {
if (i < jumbled.length) {
$("#questionDiv").append(
"<div id=" + letter + " class='letter'>" + letter + "</div>"
);
}
});
// Showing Questions
if (i < jumbled.length) {
$("#questionDiv").append(
"<div id='query'>" + jumbled[i].question + "</div>"
);
i += 1;
}
$(".box").hide();
count = 0;
}
$(document).on("click", "#btn", showQuestions);
var x = "";
var i = 0;
showQuestions();
// Creating multiple divs in answerDiv
function createDivs() {
k = 0;
var div;
while (k <= 8) {
div = document.createElement("div");
div.className = "box";
document.getElementById("answerDiv").appendChild(div);
k += 1;
}
}
createDivs();
// Click to move letter to box div
guessArray = [];
count = 0;
function hello(evt) {
var letter = evt.currentTarget;
$(".box:eq(" + count + ")").html($(letter).text());
$(letter).html("");
$(letter).prop("disabled", true);
$(".box:eq(" + count + ")").show();
guess = $(".box:eq(" + count + ")").html();
guessArray.push(guess);
newArray = guessArray.join("");
count += 1;
}
$(document).on("click", ".letter", hello);
// Checking Answers
n = 0;
score = 0;
function world() {
if (n <= jumbled.length) {
correct = jumbled[n].answer;
n += 1;
}
if (newArray === correct) {
score += 1;
}
if (score <= jumbled.length) {
z = score;
document.getElementById("score").innerHTML = z;
}
if (n >= jumbled.length) {
$("#summary").fadeIn();
$("#questionDiv").fadeOut();
$("#answerDiv").fadeOut();
$("#score").fadeOut();
$("#btn").fadeOut();
$("#summary p").text(
`Your Score: ${score} out of ${jumbled.length} correct `
);
}
guessArray = [];
window.console.log(correct, newArray);
}
$("body").on("click", "#btn", world);
// Restarting Quiz
function restart() {
i = 0;
k = 0;
n = 0;
score = 0;
$("#summary").fadeOut();
$("#questionDiv").fadeIn();
$("#answerDiv").empty();
$("#answerDiv").fadeIn();
$("#score").fadeIn();
$("#btn").fadeIn();
showQuestions();
hello();
world();
createDivs();
}
$(document).on("click", "#summary a", restart);
});
Undefined letters
When the summary screen shows, I’m given the following error:
index.js:44 Uncaught TypeError: Cannot read property 'letters' of undefined
jumbled[i].letters.forEach(function (letter) {
if (i < jumbled.length) {
$("#questionDiv").append(
"<div id=" + letter + " class='letter'>" + letter + "</div>"
);
}
});
The if statement can be moved out of the loop to fix that issue.
if (i < jumbled.length) {
jumbled[i].letters.forEach(function (letter) {
// if (i < jumbled.length) {
$("#questionDiv").append(
"<div id=" + letter + " class='letter'>" + letter + "</div>"
);
// }
});
}
Undefined currentTarget
The next issue occurs when clicking for a new game:
index.js:85 Uncaught TypeError: Cannot read property ‘currentTarget’ of undefined
That’s in the hello function, and is caused by this code:
showQuestions();
hello();
world();
The hello function seems to only be useful when a letter is clicked on. I should be able to remove that call to the hello function to fix that issue.
showQuestions();
// hello();
world();
Further investigation
The code is now stable enough to run several games can be played in a row without triggering. We can now investigate things more closely.
I placed a breakpoint in the world function to follow through what happens when the a guess is scored.
After the new game is selected, the world function is entered again before I get to make a guess. Then when making a guess things look different in the code. The first time through the game n was equal to 0 for the first guess, but after doing a new game n instead equals 1 for the first guess.
I reckon that’s because the world function is executed when doing a new game, so let’s remove it.
showQuestions();
// world();
createDivs();
And the code now runs and plays new games with no problem now.
Refactoring
Now that the code seems to be working correctly, I can work on refactoring the code.
Remove unneeded variables
Currently text is added to the x variable, then the showQuestions function sets the html text to that variable. However, the x variable is always empty.
var x = "";
var i = 0;
function showQuestions() {
document.getElementById("questionDiv").innerHTML = x;
Just get rid of x and set the questionDiv element to an empty string instead.
// var x = "";
var i = 0;
function showQuestions() {
// document.getElementById("questionDiv").innerHTML = x;
document.getElementById("questionDiv").innerHTML = "";
Check length before executing function
The next issue is in regard to checking the jumbled length.
if (i < jumbled.length) {
jumbled[i].letters.forEach(function (letter) {
$("#questionDiv").append(
"<div id=" + letter + " class='letter'>" + letter + "</div>"
);
});
}
// Showing Questions
if (i < jumbled.length) {
$("#questionDiv").append(
"<div id='query'>" + jumbled[i].question + "</div>"
);
i += 1;
}
...
$(document).on("click", "#btn", showQuestions);
showQuestions();
The showQuestions function shouldn’t need to know that much about all of the questions. By moving the if statement check and the increment outside of the function, that makes it easier for further improvements to be made.
// if (i < jumbled.length) {
jumbled[i].letters.forEach(function (letter) {
$("#questionDiv").append(
"<div id=" + letter + " class='letter'>" + letter + "</div>"
);
});
// }
// Showing Questions
// if (i < jumbled.length) {
$("#questionDiv").append(
"<div id='query'>" + jumbled[i].question + "</div>"
);
// i += 1;
// }
...
$(document).on("click", "#btn", function () {
if (i < jumbled.length) {
showQuestions();
i += 1;
}
});
showQuestions();
i += 1;
Give function the information that it needs
We can now pass information to the showQuestions function, and it doesn’t need to deal with anything else. Because the function doesn’t deal with anything else, we should also rename it to indicate that it only shows one question at a time.
// function showQuestions() {
function showQuestion(questionInfo) {
document.getElementById("questionDiv").innerHTML = "";
// jumbled[i].letters.forEach(function (letter) {
questionInfo.letters.forEach(function (letter) {
$("#questionDiv").append(
"<div id=" + letter + " class='letter'>" + letter + "</div>"
);
});
// Showing Questions
$("#questionDiv").append(
"<div id='query'>" + questionInfo.question + "</div>"
);
// i += 1;
$(".box").hide();
count = 0;
}
$(document).on("click", "#btn", function () {
if (i < jumbled.length) {
// showQuestions();
showQuestion(jumbled[i]);
i += 1;
}
});
// showQuestions();
showQuestion(jumbled[i]);
i += 1;
Remove duplication
The call to the showQuestion function and updating of the i variable is now clearly occurring in two different places.
Instead of calling showQuestion, we can just trigger the click event instead.
$(document).on("click", "#btn", function () {
if (i < jumbled.length) {
// showQuestions();
showQuestion(jumbled[i]);
i += 1;
}
});
// showQuestion(jumbled[i]);
// i += 1;
$("#btn").trigger("click");
We now have a much more tightly controlled showQuestion function, all except for that count variable.
Make count more local, then remove it
Instead of having a global count, it’s only used in the hello function so it’s best if it stays in there.
To start with, we can reduce the use of count to just one line, by using a variable called guessed.
function hello(evt) {
var letter = evt.currentTarget;
var $guessed = $(".box:eq(" + count + ")");
// $(".box:eq(" + count + ")").html($(letter).text());
$guessed.html($(letter).text());
$(letter).html("");
$(letter).prop("disabled", true);
// $(".box:eq(" + count + ")").show();
$guessed.show();
guess = $guessed.html();
guessArray.push(guess);
newArray = guessArray.join("");
count += 1;
}
Apart from that count piece in the showQuestion function, we only have the following small section to consider:
count = 0;
function hello(evt) {
...
var $guessed = $(".box:eq(" + count + ")");
...
count += 1;
}
Taking into consideration how the game works, that seems to be getting the first non-visible box.
So let’s do that with jQuery instead, which lets us remove the count variable.
// count = 0;
function hello(evt) {
...
// var $guessed = $(".box:eq(" + count + ")");
var $guessed = $(".box:not(:visible)").first();
...
// count += 1;
}
We can now remove count from everywhere, including the showQuestion function!
function showQuestion(questionInfo) {
...
// count = 0;
}
That’s much better.
Progress can occur on improving the code in many other ways, but what we have above is a good start to things.
