I have the following code, repeated 7 times in total for; Red, Yellow, Green, Blue, Pink, Black on the basis that repeating code is not good how can I refactor this to make it easier to read without affecting functionality?
// Add click event listeners to red balls (1 point)
const redBalls = document.querySelectorAll('.ball-img[src*="red"]');
redBalls.forEach(ball => {
ball.addEventListener('click', function() {
const column = this.closest('.col-4');
const isPlayerOneColumn = column.contains(document.getElementById('player-one-name'));
if ((activePlayer === 'player-one' && isPlayerOneColumn) ||
(activePlayer === 'player-two' && !isPlayerOneColumn)) {
currentBreak += 1;
document.getElementById('current-break').textContent = currentBreak;
} else {
console.log("Not your turn!");
}
});
});
// Add click event listeners to yellow balls (2 points)
const yellowBalls = document.querySelectorAll('.ball-img[src*="yellow"]');
yellowBalls.forEach(ball => {
ball.addEventListener('click', function() {
const column = this.closest('.col-4');
const isPlayerOneColumn = column.contains(document.getElementById('player-one-name'));
if ((activePlayer === 'player-one' && isPlayerOneColumn) ||
(activePlayer === 'player-two' && !isPlayerOneColumn)) {
currentBreak += 2;
document.getElementById('current-break').textContent = currentBreak;
} else {
console.log("Not your turn!");
}
});
});
New to JS and struggled to write this so far, not sure what to try
I don't think this is the right place to ask this question (you may have better luck at the Code Review StackExchange), but here is an answer anyway.
It seems the only differences between the given code is the CSS selector and the increment amount of the currentBreak
variable.
Thus, you could use a mapping to define the different CSS selectors and how much each one increments currentBreak
. That is, we associate each color with how much it adds to currentBreak
by defining an object with the color as the key and the amount as the value. By looping over this map, we can access each color and currentBreak
value without needing to duplicate the rest of the handling code.
const ballMap = { "red" : 1, "yellow" : 2 }; // Add more colors
for (const [color, breakAmount] of Object.entries(ballMap)) // Iterate over each pair of values in ballMap. Downgrade this according to your ECMAScript version.
{
const balls = document.querySelectorAll(`.ball-img[src*='${color}']`); // String interpolation in the CSS selector.
balls.forEach(ball => {
ball.addEventListener('click', () => {
const column = this.closest('.col-4');
const isPlayerOneColumn = column.contains(document.getElementById('player-one-name'));
if ((activePlayer === 'player-one' && isPlayerOneColumn) || (activePlayer === 'player-two' && !isPlayerOneColumn)) {
currentBreak += breakAmount; // Use the number specified in the mapping.
document.getElementById('current-break').textContent = currentBreak;
}
else
{
console.log("Not your turn!");
}
});
});
}
Edit to add that I agree with Phil's answer that it would be more optimized to avoid adding an event listener to each and every "ball" element and instead put a single event listener on a parent element:
parent element <-- put event listener here, discriminate click by ball color
-- red ball elements
-- yellow ball elements
-- ...etc