Problem
I’m a few weeks into javascript and had to make a simple “crystals collector” app. The app works as intended, but the code is obtuse and repetitive.
I’d like to make the app work using just one on click event, rather than using four for each crystal. I can’t make an array of each color crystal and loop through them since the num variables change as well.
Here is my code below:
$(document).ready(function () {
// Global variables
var targetNumber;
var num1;
var num2;
var num3;
var num4;
var userTotal = 0;
var wins = 0;
var losses = 0
// Functions
function reset() {
num1 = Math.floor(Math.random() * 11 + 1);
num2 = Math.floor(Math.random() * 11 + 1);
num3 = Math.floor(Math.random() * 11 + 1);
num4 = Math.floor(Math.random() * 11 + 1);
targetNumber = Math.floor(Math.random() * 101 + 19);
userTotal = 0;
$("#total-score").text(userTotal);
$("#target-score").text(targetNumber);
}
function initialize() {
num1 = Math.floor(Math.random() * 11 + 1);
num2 = Math.floor(Math.random() * 11 + 1);
num3 = Math.floor(Math.random() * 11 + 1);
num4 = Math.floor(Math.random() * 11 + 1);
targetNumber = Math.floor(Math.random() * 101 + 19);
$("#target-score").text(targetNumber);
$("#wins").text(wins);
$("#losses").text(losses);
$("#total-score").text(userTotal);
}
function logic() {
if (userTotal === targetNumber) {
alert("You Win!");
reset();
wins++;
$("#wins").text(wins);
}
else if (userTotal > targetNumber) {
alert("You lose!");
reset();
losses++;
$("#losses").text(losses);
}
}
// Run Game (main)
// something like...
// var array = ["#blue","#green","#red","#yellow"]
// for (var i =0; i < array.length;i++) {
// }
initialize();
$("#blue").on("click", function () {
userTotal = userTotal + num1;
$("#total-score").text(userTotal);
console.log(userTotal);
logic();
})
$("#green").on("click", function () {
userTotal = userTotal + num2;
$("#total-score").text(userTotal);
console.log(userTotal);
logic();
})
$("#red").on("click", function () {
userTotal = userTotal + num3;
$("#total-score").text(userTotal);
console.log(userTotal);
logic();
})
$("#yellow").on("click", function () {
userTotal = userTotal + num4;
$("#total-score").text(userTotal);
console.log(userTotal);
logic();
})
});.img {
width: 150px;
height: 150px;
}
#crystal-main {
width: 650px;
border: 2px solid gray;
padding: 25px;
background: black;
color: whitesmoke;
}<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<meta http-equiv="X-UA-Compatible" content="ie=edge">
<link rel="stylesheet" href="https://stackpath.bootstrapcdn.com/bootstrap/4.3.1/css/bootstrap.min.css"
integrity="sha384-ggOyR0iXCbMQv3Xipma34MD+dH/1fQ784/j6cY/iJTQUOhcWr7x9JvoRxT2MZw1T" crossorigin="anonymous">
<link rel="stylesheet" href="assets/css/style.css">
<title>Crystal Game</title>
<script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.2.1/jquery.min.js"></script>
<script src="assets/javascript/game.js"></script>
</head>
<body>
<h1>Crystals Collector!</h1>
<hr>
<div class="container-fluid">
<div class="container" id="crystal-main">
<div class="row">
<h2>Target Score: <span id="target-score"></span></h2>
</div>
<div class="row">
<h2>Total Score: <span id="total-score"></span></h2>
</div>
<div class="row">
<div class="col-3-md crystal">
<img src="assets/images/blue.png" alt="blue" class="img" id="blue">
</div>
<div class="col-3-md crystal">
<img src="assets/images/green.png" alt="green" class="img" id="green">
</div>
<div class="col-3-md crystal">
<img src="assets/images/red.png" alt="red" class="img" id="red">
</div>
<div class="col-3-md crystal">
<img src="assets/images/yellow.png" alt="yellow" class="img" id="yellow">
</div>
</div>
<div class="row">
<h4>Wins: <span id="wins"></span></h4>
</div>
<div class="row">
<h4>Losses: <span id="losses"></span></h4>
</div>
</div>
</div>
</body>
</html>Solution
Here’s my answer. Note I did not touch the html. We could attack both the javascript and the html and condense them both, but I think that just one file change is necessary right now.
$(document).ready(function () {
// Global variables
var targetNumber;
var userTotal = 0;
var wins = 0;
var losses = 0;
// Functions
function reset() {
for(var i = 0; i < crystals.length; i++) {
crystals[i].setAttribute("score", Math.floor(Math.random() * 11 + 1))
}
targetNumber = Math.floor(Math.random() * 101 + 19);
userTotal = 0;
$("#total-score").text(userTotal);
$("#target-score").text(targetNumber);
}
function initialize() {
crystals = document.getElementsByTagName("img")
for(var i = 0; i < crystals.length; i++) {
crystals[i].setAttribute("score", Math.floor(Math.random() * 11 + 1))
crystals[i].addEventListener("click", (args) => {
value = Math.round(args.target.getAttribute("score"))
userTotal = userTotal + value;
$("#total-score").text(userTotal);
console.log(userTotal);
logic();
})
}
targetNumber = Math.floor(Math.random() * 101 + 19);
$("#target-score").text(targetNumber);
$("#wins").text(wins);
$("#losses").text(losses);
$("#total-score").text(userTotal);
}
function logic() {
if (userTotal === targetNumber) {
alert("You Win!");
reset();
wins++;
$("#wins").text(wins);
}
else if (userTotal > targetNumber) {
alert("You lose!");
reset();
losses++;
$("#losses").text(losses);
}
}
initialize();
});
You’re right when you say it would be difficult to loop through each element and increase the user’s score by a preset random value. Instead, we make that random score part of the element. We can get all the crystals by getting all the images in the document (with the document.getElementsByTagName(tagname) function). This returns a list of the html elements.
We can set the random value in the initialize function, with the setAttribute() function. This takes in two parameters: first, the new attribute name; second, the attribute’s value. At the outset (and in each reset of the game), we set it to the new random value.
Now we have an iterating loop through the four crystals, and each crystal has a new html attribute which contains its score. Now all we need to do is add the event listeners. We’ll do that in the same for loop. Simply use the addEventListener(event, f) function. It takes in a string for what event to activate on, and a function to execute when that event is triggered. When the crystal is clicked, we’ll want to get the value in the attribute, add it to the user’s total, change the text, log it, and call logic!
That’s pretty much it. I also removed the declarations for the ‘numx’ variables.
General feedback
The code is quite redundant so to adhere to the Don’t Repeat Yourself principle, there are a couple things that can be done:
- Abstract common lines from functions like
reset()andinitialize()into a separate function that can be called in both places. That way if you need to update logic it can be done in one place instead of multiple. - Store the numbers representing the crystals in an array, Object
, Set etc. that can be indexed into and also iterated over. And you could use an id attribute or data attributes from the elements to determine which element was clicked.
The function name logic seems quite vague. Somebody reading the code might wonder what “logic” it contains. If it had a more descriptive name, like “checkOutcome“
Targeted Feedback
For constructs like this
wins++;
$("#wins").text(wins);
You could use a pre-increment operator instead. That way there is no need to have the first line:
$("#wins").text(++wins);
For adding a number to a variable:
userTotal = userTotal + num4;
There is a shorthand Addition assignment operator that can be used here to simplify this:
userTotal += num4;
The deprecated syntax (as of jQuery 3.0) for the .ready() method:
$(document).ready(function() {
can be changed to
$(function() {
Since that is the recommended format1
There is little benefit to making a class name img, since your CSS could select the images with a tag name selector:
img {
width: 150px;
height: 150px;
}
But in a larger application/page, perhaps a class name like crystal would be more descriptive.