6
\$\begingroup\$

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:

Pig Game Files

Specific Questions to Address:

  1. 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)?
  2. Am I creating module containers correctly? Are there any other methods to create modules that might hold an advantage over this structure?
  3. 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?

  4. 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?

  5. 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')
    };
    
  6. 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?

  7. If I had added an extra indent to my entire / -- Buttons -- / subsection in the game module for visual clarity, is that bad practice?

  8. 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?

  9. 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 ----- */
    }();
}
\$\endgroup\$
2
\$\begingroup\$

Your questions

Wow, a lot of questions. Let's get crackin' :-)

  1. 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)?

I'm not a JS expert, but I think it's safe to trust that const works properly and effectively prevents the objects from leaking into the global scope.

  1. Am I creating module containers correctly? Are there any other methods to create modules that might hold an advantage over this structure?

Although the program is correct, I would design the modules differently. DOMCache is effectively a simply grouping of objects: players, die and buttons. But these objects are conceptually independent, there's no reason to put them in one group, and to set apart from game.

Also, the name DOMCache is misleading. I expected to be a simple cache of DOM objects, to avoid repeated lookups in the DOM. But it's not about that. And the objects are not simple DOM objects, but custom user objects.

I would remove the unnecessary grouping, and have all these objects alongside game. These objects are all instrumental for the proper functioning of game. Since there is nothing outside DOMCache that belongs to something else other than game, it's ok to have all these objects in the same namespace.

  1. 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?

If you mean having player1 and player2 as direct attributes of DOMCache instead of in an array, yes that would be fine. But I would keep the players array, to keep it easy to extend to more than 2 players.

  1. 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?

Yes, I think it's a good idea for Player to have its own reset method. This is good encapsulation.

  1. 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?

No. For the same reason that I argued against DOMCache itself earlier. These distinct buttons each stand for an important aspect of the game. Whether the game accesses them directly, or indirectly through a grouping will make no practical difference.

Notice how this is distinct from having the players in an array instead of standing individually. The game flow won't fundamentally change whether you have 2, 3, or 10 players, and having them in an array makes it easy to perform operations on all of them, such as reset. The same is not true for the buttons. If you were to add a new button, it will likely represent a completely new functionality, and it's unlikely that grouping them will bring any significant benefits.

  1. 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?

I don't like this kind of "decoration" in code, I find it noise.

  1. If I had added an extra indent to my entire / -- Buttons -- / subsection in the game module for visual clarity, is that bad practice?

I don't think it would make an important difference.

  1. On line 120 using the setTimeout function: The following code would not execute.

    setTimeout(DOMCache.die.hide, 300);
    

The first parameter of setTimeout should be a function. Although DOMCache.die.hide is a function, as opposed to DOMCache.die.hide() which is an expression that is evaluated immediately. Unfortunately my JavaScript-fu is not good to be able to explain why the code above doesn't work.

  1. 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?

See the next section :-)

Design

As mentioned earlier, the objects in DOMCache are already more than just simple DOM objects. They are custom user objects, with added functionality. However, the added functionality is not enough to be able to use these objects in a natural way. The operations you need to do on them are not at all intuitive, and also require lengthy, complex code. I'm talking about things like this:

DOMCache.btnRoll.addEventListener('click', roll);
DOMCache.btnHold.addEventListener('click', hold);

DOMCache.player[activePlayer].name.textContent = 'Winner!';
DOMCache.player[activePlayer].panel.classList.add('winner');
DOMCache.player[activePlayer].panel.classList.remove('active');

Wouldn't it be great if you could instead write:

btnRoll.onClick(roll);
btnHold.onClick(hold);

activePlayer.setWinner();

I think it would be. I would rewrite game like this:

const game = function() {
    let activePlayerIndex;
    let activePlayer;

    reset();

    btnNew.onClick(reset);

    function reset() {
        players.forEach(p => p.reset());
        die.hide();
        selectPlayer(Math.floor(Math.random() * players.length));
        btnRoll.onClick(roll);
        btnHold.onClick(hold);
    }

    function selectPlayer(index) {
        activePlayerIndex = index;
        activePlayer = players[activePlayerIndex];
        activePlayer.activate();
    }

    function switchToNextPlayer() {
        activePlayer.deactivate();
        selectPlayer((activePlayerIndex + 1) % players.length);
        die.hide();
    }

    function roll() {
        let n = Math.floor(Math.random() * 6) + 1;
        die.setFace(n);  // implies die.show()

        if (n === 1) {
            activePlayer.setCurrentScore(0);
            switchToNextPlayer();
        } else {
            activePlayer.addCurrentScore(n);
        }
    }

    function hold() {
        activePlayer.updateTotalScore();
        activePlayer.setCurrentScore(0);

        if (activePlayer.score >= 100) {
            btnRoll.reset();
            btnHold.reset();
            activePlayer.setWinner();
        } else {
            switchToNextPlayer();
        }
    }
}();

To make this work, the rest of the code would have to change substantially. But I think it would be quite straightforward. The ideas here:

  • Let the custom player, die, and button objects hide the messy details of DOM manipulations, so that the Game can focus on what is important for its mechanics.

  • Simplify many operations by using an activePlayer object directly, instead of the array of players and the player index (which I moved to activatePlayerIndex, and it's no longer used directly).

  • This refactoring of focusing on the idiomatic elements of the game also helped eliminate a lot of comments. With the noise reduced, the code is hopefully easier to read.

\$\endgroup\$
  • 2
    \$\begingroup\$ +1 - great answers! re: #8 - any function that is called in the settimout loses it's context. so this inside the hide function will refer to the window. the only way to preserve context is to wrap it in an anonymous function :) \$\endgroup\$ – iwrestledabearonce Aug 22 '17 at 20:33
  • \$\begingroup\$ Really appreciate the feedback, very helpful! Some of the things that stick out to me a lot as a beginner: the explanation that objects I created in DOMCache should be viewed as custom objects instead of pure caching, keeping the buttons separate as opposed to children of an object in comparison with your explanation of keeping the players in object (both explanations really helped me see when to use which style). Lastly the rewrite is awesome and gives me a better taste of readable code. Thanks a ton! \$\endgroup\$ – ChillyPenguin672 Sep 21 '17 at 16:21

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.