I have a dice game called Pig to play (a simple google search will show rules, they are included in my .js file as well). I am new to JavaScript and the main goal of this code review is to give me an understanding of whether or not I am designing my code well (in terms of performance, as well as readability/beautifully).
Any and all feedback is much appreciated, I have a slew of questions I will fire of here but if there is something I may have overlooked/missed please let me know. If I am doing something ineffective/stupid in my code please tear into me with some criticisms/recommendations/preferences with explanations!
My program uses a small number (7) of images as part of the functionality so here is a link to my github repo instead of a jsfiddle:
Specific Questions to Address:
- Are {} now acceptable to wrap my entire program in instead of an IIFE to prevent memory leaks because const has blockscope (as opposed to var)?
- Am I creating module containers correctly? Are there any other methods to create modules that might hold an advantage over this structure?
When returning my player objects in the array to the DOMCache object, is it recommended to structure my document queries in an object as shown vs. opposed to being direct attributes to my DOMCache object?
In the spirit of separating concerns, is adding a reset function to my Player object bad practice vs. adding the functionality in later in the game control module?
Should I have bundled my btnNew, btnHold, btnRoll into one object that I would return to my DOMCache object similar to how my player0 and player1 objects were structured?
For Example:
const btn = { New: document.querySelector('.btn-roll'), Hold: document.querySelector('.btn-hold'), Roll: document.querySelector('.btn.roll') };Should I section my code like I did with the / ----- Player ---- / and bundle my prototype and creating my objects together? Same question with regard to my function section of the game modules?
If I had added an extra indent to my entire / -- Buttons -- / subsection in the game module for visual clarity, is that bad practice?
On line 120 using the setTimeout function:
The following code would not execute.
setTimeout(DOMCache.die.hide, 300);Also this set of code would execute but would be invoked immediately after being called (as expected).
setTimeout(DOMCache.die.hide(), 300);Do I need to always wrap my intended function in a function declaration when using setTimeout?
I only needed to access the textContent node of the DOM for multiple query references. Is it better to be more specific when creating my player objects?
For example, should I have done this:
init: function(playerIndex) { this.playerIndex = playerIndex; this.name = document.getElementById(`name-${playerIndex}`).textContent; this.score = document.getElementById(`score-${playerIndex}`).textContent; this.current = document.getElementById(`current-${playerIndex}`).textContent; this.panel = document.querySelector(`.player-${playerIndex}-panel`).classList; return this;
/*
GAME RULES:
- The game has 2 players, playing in rounds
- In each turn, a player rolls a dice as many times as he whishes. Each result get added to his ROUND score
- BUT, if the player rolls a 1, all his ROUND score gets lost. After that, it's the next player's turn
- The player can choose to 'Hold', which means that his ROUND score gets added to his GLBAL score. After that, it's the next player's turn
- The first player to reach 100 points on GLOBAL score wins the game
*/
{
// DOM Module
const DOMCache = function() {
/* ----- Player ----- */
// Player Prototype
const Player = {
init: function(playerIndex) {
this.playerIndex = playerIndex;
this.name = document.getElementById(`name-${playerIndex}`);
this.score = document.getElementById(`score-${playerIndex}`);
this.current = document.getElementById(`current-${playerIndex}`);
this.panel = document.querySelector(`.player-${playerIndex}-panel`);
return this;
},
reset: function() {
this.name.textContent = `Player-${this.playerIndex}`;
this.score.textContent = '0';
this.current.textContent = '0';
this.panel.classList.remove('active', 'winner');
}
};
// Player Objects
const player0 = Object.create(Player).init(0);
const player1 = Object.create(Player).init(1);
/* ----- END ----- */
// Buttons
const btnRoll = document.querySelector('.btn-roll');
const btnHold = document.querySelector('.btn-hold');
const btnNew = document.querySelector('.btn-new');
// Die
const die = {
reference: document.querySelector('.dice'),
setFace: function(face) {
this.reference.src = `resources/img/dice-${face}.png`;
},
hide: function() {
this.reference.style.opacity = '0';
},
show: function() {
this.reference.style.opacity = '100';
}
};
return {
player: [ player0, player1 ],
die: die,
btnRoll: btnRoll,
btnHold: btnHold,
btnNew: btnNew
}
}();
// Game Function Module
const game = function() {
// Setup
const score = [0, 0];
const current = [0,0];
let activePlayer = 0;
reset();
// Add btn-new event listner
DOMCache.btnNew.addEventListener('click', reset);
/* ----- Functions ----- */
// reset
function reset() {
// 1) Reset DOM scores & player name
DOMCache.player[0].reset();
DOMCache.player[1].reset();
// 2) Reset game scores
score[0] = 0;
score[1] = 0;
current[0] = 0;
current[1] = 0;
// 3) Hide die
DOMCache.die.hide();
activePlayer = Math.floor(Math.random() * 2);
DOMCache.player[activePlayer].panel.classList.add('active');
// 4) Add Event Listeners
DOMCache.btnRoll.addEventListener('click', roll);
DOMCache.btnHold.addEventListener('click', hold);
}
// Switch active player
function switchActivePlayer() {
DOMCache.player[activePlayer].panel.classList.remove('active');
activePlayer === 0 ? activePlayer = 1 : activePlayer = 0;
DOMCache.player[activePlayer].panel.classList.add('active');
}
/* -- Buttons -- */
// btn-roll
function roll() {
// Generate roll #
n = Math.floor(Math.random() * 6) + 1;
DOMCache.die.setFace(n);
if (n === 1) {
// 1) Current -> 0
current[activePlayer] = 0;
DOMCache.player[activePlayer].current.textContent = `${current[activePlayer]}`;
setTimeout(function(){DOMCache.die.hide()}, 300);
// 2) Switch activePlayer
switchActivePlayer();
}
else {
// Add # to current
current[activePlayer] += n;
DOMCache.player[activePlayer].current.textContent = `${current[activePlayer]}`;
DOMCache.die.show();
}
}
// btn-hold
function hold() {
// 1) Update game scores
score[activePlayer] += current[activePlayer];
current[activePlayer] = 0;
// 2) Update DOM scores
DOMCache.player[activePlayer].current.textContent = '0';
DOMCache.player[activePlayer].score.textContent = `${score[activePlayer]}`;
// 3) Hide the die
DOMCache.die.hide();
// 4) Check for winner
if (score[activePlayer] >= 100) {
// 1) Remove event listeners
DOMCache.btnRoll.removeEventListener('click', roll);
DOMCache.btnHold.removeEventListener('click', hold);
// 2) name -> Winner!
DOMCache.player[activePlayer].name.textContent = 'Winner!';
DOMCache.player[activePlayer].panel.classList.add('winner');
DOMCache.player[activePlayer].panel.classList.remove('active');
}
else {
switchActivePlayer();
}
}
/* -- END -- */
/* ----- END ----- */
}();
}
