Card-fighting game Part 2

Question

This is part 2 of the game I am building. After some good feedback on my first post I decided it was time to post my updated code.

The differences in this part are: Updated code after feedback, new dodge mechanic and better formatted code.

What I am looking for are: performance feedback, code style feedback, things I can do in a better way and JavaScript best practices.

For more information about the game I should advice you to visit my first question.

var cards = [1, 1, 1, 1, 2, 2, 2, 2, 3, 3, 3, 3, 4, 4, 4, 4, 5, 5, 5, 5, 6, 6, 6, 6, 7, 7, 7, 7, 8, 8, 8, 8, 9, 9, 9, 9, 10, 10, 10, 10];
var shuffledCards = shuffle(cards);
var playerDeck = shuffledCards.slice(0, 20);
var computerDeck = shuffledCards.slice(20);
var playerlife = 25;
var computerLife = 25;
var playerFeedback = document.getElementById('playerFeedback');
var playerName = '';
var swordSound = new Audio('sounds/sword.mp3');
var blockSound = new Audio('sounds/block.mp3');
var dodgeSound = new Audio('sounds/dodged.mp3');
var dodged = false;

/**
 * @param array
 * @returns {*}
 */
function shuffle(array) {
    "use strict";
    var m = array.length;
    var t;
    var i;

    // While there remain elements to shuffle…
    while (m) {

        // Pick a remaining element…
        i = Math.floor(Math.random() * m--);

        // And swap it with the current element.
        t = array[m];
        array[m] = array[i];
        array[i] = t;
    }

    return array;
}

/**
 * @returns {boolean}
 */
function getPlayerName() {
    "use strict";
    playerName = document.getElementById('userInput').value;
    if (playerName.length === 0) {
        return false;
    }
    document.getElementById('playerName').innerHTML = playerName;
    document.getElementById('overlay').style.display = 'none';
}

function checkIsGameOver() {
    "use strict";
    var gameResult = document.getElementById('gameResult');
    var gameResultContent = gameResult.innerHTML;
    var replay = document.getElementById('replay');
    var click = document.getElementById('click');
    var dodgeL = document.getElementById('dodgeL');
    var dodgeR = document.getElementById('dodgeR');

    function gameIsOver() {
        playerFeedback.innerHTML = 'Game Over, do you want to play again?';
        gameResult.innerHTML = gameResultContent;
        gameResult.style.display = 'block';
        replay.style.display = 'inline';
        click.style.display = 'none';
        dodgeL.style.display = 'none';
        dodgeR.style.display = 'none';
    }

    if (playerlife <= 0) {
        playerlife = 'Lost';
        computerLife = 'Won';
        gameResultContent = '<p>Hunter won the game!</p>';
        gameIsOver();
    } else if (computerLife <= 0) {
        computerLife = 'Lost';
        playerlife = 'Won';
        gameResultContent = '<p>' + playerName + ' won the game!</p>';
        gameIsOver();
    }
}

function whoWon() {
    "use strict";
    var playerTopCard = playerDeck[0];
    var computerTopCard = computerDeck[0];
    document.getElementById('computerDamageImage').setAttribute('src', 'images/empty.png');
    document.getElementById('playerDamageImage').setAttribute('src', 'images/empty.png');

    if (playerTopCard > computerTopCard) {
        computerLife = computerLife - (playerTopCard - computerTopCard);
        swordSound.play();
        document.getElementById('computerDamageImage').setAttribute('src', 'images/damage.gif');
        playerFeedback.innerHTML = 'Hunter lost: ' + (playerTopCard - computerTopCard) + ' HP';
    } else if (playerTopCard < computerTopCard) {
        playerlife = playerlife - (computerTopCard - playerTopCard);
        swordSound.play();
        document.getElementById('playerDamageImage').setAttribute('src', 'images/damage.gif');
        playerFeedback.innerHTML = playerName + ' lost: ' + (computerTopCard - playerTopCard) + ' HP';
    } else {
        blockSound.play();
        playerFeedback.innerHTML = 'It was a tie!';
    }
    checkIsGameOver();
    playerDeck.shift();
    computerDeck.shift();
}

function didPlayerDodged() {
    "use strict";
    var playerTopCard = playerDeck[0];
    var computerTopCard = computerDeck[0];
    document.getElementById('computerDamageImage').setAttribute('src', 'images/empty.png');
    document.getElementById('playerDamageImage').setAttribute('src', 'images/empty.png');

    if (dodged === true) {
        computerLife = computerLife - playerTopCard;
        dodgeSound.play();
        document.getElementById('computerDamageImage').setAttribute('src', 'images/damage.gif');
        playerFeedback.innerHTML = 'You just dodged the attack! Hunter lost: ' + playerTopCard + ' HP';
    } else {
        playerlife = playerlife - computerTopCard;
        swordSound.play();
        document.getElementById('playerDamageImage').setAttribute('src', 'images/damage.gif');
        playerFeedback.innerHTML = 'You could not dodged the attack! ' + playerName + ' lost: ' + computerTopCard + ' HP';
    }

    checkIsGameOver();
    playerDeck.shift();
    computerDeck.shift();
}

/**
 * @param type
 */
function dealCards(type) {
    "use strict";
    var playerTopCard = playerDeck[0];
    var computerTopCard = computerDeck[0];
    var cardImage = "<img src='images/cardBack.png'/>";

    if (type === 'attack') {
        whoWon();
    } else if (type === 'dodgeTrue') {
        dodged = true;
        didPlayerDodged();
    } else {
        dodged = false;
        didPlayerDodged();
    }

    document.getElementById("card1").innerHTML = "<div class='cardNumber'>" + playerTopCard + '</div>' + cardImage;
    document.getElementById("card2").innerHTML = "<div class='cardNumber'>" + computerTopCard + '</div>' + cardImage;
    document.getElementById("scoreComputer").innerHTML = computerLife;
    document.getElementById("scorePlayer").innerHTML = playerlife;
}

function playAgain() {
    "use strict";
    window.location.reload();
}

/**
 * @param direction
 * @returns {boolean}
 */
function dodgeDirection(direction) {
    "use strict";
    var goodDirection = Math.floor(Math.random() * 2) + 1;
    return direction === goodDirection;
}

(function startGame() {
    "use strict";
    document.getElementById("replay").addEventListener("click", playAgain);
    document.getElementById("userSubmit").addEventListener("click", getPlayerName);

    document.getElementById("click").addEventListener("click", function () {
        dealCards('attack');
    });

    document.getElementById("dodgeL").addEventListener("click", function () {
        if (dodgeDirection(1)) {
            dealCards('dodgeTrue');
        } else {
            dealCards('dodgeFalse');
        }
    });

    document.getElementById("dodgeR").addEventListener("click", function () {
        if (dodgeDirection(2)) {
            dealCards('dodgeTrue');
        } else {
            dealCards('dodgeFalse');
        }
    });
}());

Answer 1

Organize the data

Sound Library

You're starting to get a lot of global variables in your project. As your project grows, you'll want to organize it more so that it is easier to grasp. Some of your data would be better organized into classes and objects. For example you could make a sounds library object for sound effects:

var sounds = {
  block: new Audio('sounds/block.mp3'),
  dodge: new Audio('sounds/dodged.mp3'),
  sword: new Audio('sounds/sword.mp3')
};

sounds.sword.play(); // usage

Now when you want to add another sound or edit the ones you have, all that logic will be contained in the sounds object.

Player Class

You might want to consider making a player class. There are a few ways to make classes in JavaScript, but I prefer a factory method (a function that constructs an instance of your class from parameters):

function createPlayer(name, maxLife) { // factory functions
  var player = {
    name: name,
    life: maxLife,
    deck: undefined // no cards yet
  };

  return player;      
}

var player = createPlayer(undefined, 25); // usage
var computer = createPlayer("Hunter", 25);

Deck Class

You have a lot of global functions that you're applying to a deck of cards. And a lot of your functions need to know that your decks are arrays. This deck class abstracts your deck logic into one place:

function createDeck(array) {
  var cards = array; // private variable

  return {
    // removes the card off the top of the deck
    draw: function () {
      return cards.pop(); // the backside of the array the top of the deck
    },

    // returns the value of the card on the top of the deck
    peek: function () {
      return cards[cards.length - 1];
    },

    // returns the size of the deck
    size: function () {
      return cards.length;
    },

    // randomizes the order of the cards in the deck
    shuffle: function () {
      cards = shuffle(cards); // call your global shuffle function
    },

    // returns an array to be assigned to two players. Second array get's the extra card if the deck's length is odd
    deal: function () {
      var half = Math.floor(cards.length / 2);
      var deck0 = createDeck(cards.slice(0, half));
      var deck1 = createDeck(cards.slice(half));

      return [deck0, deck1];
    },

    // returns a string of the deck
    toString: function () {
      return cards.toString();
    }
  };
}

var deck = createDeck([1, 1, 2, 2, 3, 3, 4, 4, 5, 5, 6, 6, 7, 7, 8, 8];
deck.shuffle();

var hands = deck.deal();
player.deck = hands[0];
computer.deck = hands[1];

Now you can refactor your whoWon function like so:

function whoWon() {
    "use strict";
    var playerCard = player.deck.draw();
    var computerCard = computer.deck.draw();

    document.getElementById('computerDamageImage').setAttribute('src', 'images/empty.png');
    document.getElementById('playerDamageImage').setAttribute('src', 'images/empty.png');

    if (playerCard > computerCard) {
        computerLife = computerLife - (playerTopCard - computerTopCard);
        sounds.sword.play();
        document.getElementById('computerDamageImage').setAttribute('src', 'images/damage.gif');
        playerFeedback.innerHTML = computer.name + ' lost: ' + (playerTopCard - computerTopCard) + ' HP';
    } else if (playerCard < computerCard) {
        playerlife = playerlife - (computerTopCard - playerTopCard);
        sounds.sword().play();
        document.getElementById('playerDamageImage').setAttribute('src', 'images/damage.gif');
        playerFeedback.innerHTML = playerName + ' lost: ' + (computerTopCard - playerTopCard) + ' HP';
    } else {
        sounds.block.play();
        playerFeedback.innerHTML = 'It was a tie!';
    }
    checkIsGameOver();
}

This might not seem like much, but now your decks are able to manage themselves. They've become their own module. Your whoWon function doesn't need to know that the cards are stored as an array; It doesn't need to know that the top of the deck is the front of the array or the back. All it knows is that the decks have a draw method that "draws" a card from a players deck.

DOM Element Library - Calling document.getElementById like 100000 times

Nearly every single one of your functions needs to access a DOM element at some point. You could introduce jquery to simplify the redundancy of document.getElementById, but then you would have to include jquery, adding a dependency to your code that you don't really need.

I really like these two functions that I found in a cheeky little article about the "harmfulness" of jquery:

// Returns first element that matches CSS selector {expr}.
// Querying can optionally be restricted to {container}’s descendants
function $(expr, container) {
    return typeof expr === "string"? (container || document).querySelector(expr) : expr || null;
}

// Returns all elements that match CSS selector {expr} as an array.
// Querying can optionally be restricted to {container}’s descendants
function $$(expr, container) {
    return [].slice.call((container || document).querySelectorAll(expr));
}

Using the first function, $, you could simplify document.getElementById('playerFeedback') to: $('#playerFeedback').

As of getting the elements over and over again, I think you can assume the browser will be fast enough. But if you want, you could create a library of the DOM elements you want to use.

var elements = {
  playerFeedback: $('#playerFeedback'),
  computerDamageImage: $('#computerDamageImage')
}

Let's do some more refactoring to whoWon:

function whoWon() {
    "use strict";
    var playerCard = player.deck.draw();
    var computerCard = computer.deck.draw();

    elements.computerDamageImage.setAttribute('src', 'images/empty.png');
    elements.playerDamageImage.setAttribute('src', 'images/empty.png');

    if (playerCard > computerCard) {
        computer.life = computer.life - (playerCard - computerCard);
        sounds.sword.play();
        elements.computerDamageImage.setAttribute('src', 'images/damage.gif');
        elements.playerFeedback.innerHTML = computer.name + ' lost: ' + (playerTopCard - computerTopCard) + ' HP';
    } else if (playerCard < computerCard) {
        player.life = player.life - (computerCard - playerCard);
        sounds.sword().play();
        elements.playerDamageImage.setAttribute('src', 'images/damage.gif');
        elements.playerFeedback.innerHTML = player.name + ' lost: ' + (computerCard - playerCard) + ' HP';
    } else {
        sounds.block.play();
        elements.playerFeedback.innerHTML = 'It was a tie!';
    }
    checkIsGameOver();
}

There are a few other things I'd like to help you with, but this is good for now. Atillio makes some really good points. I particularly agree with the Logic/displaying section; look into MVC (Model-View-Controller) Architecture.

Answer 2

Logic/displaying

In general, the logic of the game is very "entwined" with the displaying of the results. I would separate this at two levels:

• Wrap the calls affecting HTML (e.g. displaying a value) into functions that clearly describe the purpose. E.g. instead of:

  document.getElementById('playerName').innerHTML = playerName;

Use the following:

function showPlayerName(playerName) {
    document.getElementById('playerName').innerHTML = playerName;
}

• First, you can just call these wrappers from the functions implementing the logic. A further improvement, which decouples more the logic from the displaying could be generating events. E.g., instead of doing:

  playerFeedback.innerHTML = 'You just dodged the attack! Hunter lost: ' + playerTopCard + ' HP';

(Which by now should be a separate function, as per the above ;) ) You could generate a playerDodgedAttack event, which you connect to the function for giving feedback.

Another remark, regarding logic: the functions whoWon and didPlayerDodged have a very similar structure. I suggest moving the common parts (e.g. changing the images at the beginning and updating game logic at the end) into common functions, in order to avoid copy-paste code.

Idiomatic JavaScript

• More elegant" way to initialize the cards array: I suggest trying a combination of repeating an element and extending an array.

EDIT: re comment -- here is a possible way of using these functions:

[].concat(new Array(4).fill(1), new Array(4).fill(2), ... new Array(10).fill(2));

If you do not want to type the array for 1, 2, 3, ... 10 manually, you can do something like this:

[].concat.apply([], new Array(10).fill().map((_,k) => new Array(4).fill(k+1)))

• Also, there are more concise ways of shuffling an array.

More effective JavaScript

• What is the reason for adding "use strict" to each function and not adding it once globally? While I'm not totally familiar with the concept, based on this page, it is possible to add it globally.

• I think the functions should not be global, in order not to pollute the global namespace. Instead, I suggest making them internal functions of startGame.

Naming, documentation

• There are some doc strings like @param array. This adds redundancy, without adding useful information (the parameter list can already be seen from the declaration of the function). So I suggest either removing these empty documentations, or properly describing the meaning of the parameters. The situation may become even worse: if the parameters of these functions are changed in any way, but the documentation is not updated, then it may contradict what the function really does (which, in this case, is even worse than no documentation at all!)

• Various function names are misleading, e.g. from getPlayerName() I would expect that it returns a string that represents the name of the player, not that it displays the name of the player, and returns a boolean telling whether this was possible or not. (Btw.: the function never returns true, so I suggest double checking its logic.) Based on what the function does, I would rather call it displayPlayerNameIfExists, or something similar. Similarly, I would rename gameIsOver to displayGameOver (btw., gameIsOver is a good example for a "wrapper" function that I mentioned in the first point.)

Various remarks

• I recommend, to review playAgain, and think of a way in which the state of the game can be reset, without needing to reload the page.

• I suggest that you look into JQuery: many tasks (such as displaying texts, generating/handling events) have a more concise syntax.