8
\$\begingroup\$

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');
        }
    });
}());
\$\endgroup\$
6
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Wow this is much more then I expected, and I am talking about your great help! Thanks a lot, you helped me a lot! \$\endgroup\$ – Cylion May 21 '16 at 21:05
  • \$\begingroup\$ Very good answer! One weak spot is the unnecessary wrapping of querySelector(All) into $($), especially since you advise only querying the elements once globally rather than in each individual function that needs them. \$\endgroup\$ – Tibos May 22 '16 at 2:49
  • \$\begingroup\$ Ha thanks @tibos! I was thinking the same thing as I was coding up the element library example. A few answerers had been bringing up jquery, and I've found those functions useful for shortening element selection, especially since the second returns an ARRAY instead of a NodeList. Glad I could help @cylion. Good luck with your project! \$\endgroup\$ – Joshua Dawson May 22 '16 at 5:32
  • \$\begingroup\$ @JoshDawson Hey men, I was wondering the player.deck == a object but the computer.deck == to an array. How come? \$\endgroup\$ – Cylion May 22 '16 at 21:15
  • \$\begingroup\$ @cylion Ack a bug! If you look at my deal function, I don't call createDeck on the second hand. I'll fix my answer. \$\endgroup\$ – Joshua Dawson May 22 '16 at 21:19
5
\$\begingroup\$

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

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)))

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ First of all, I would like to thank you for your time. To be honest, the use strict is only used so that JsLint can quiet down. Also, I am not allowed to use jQuery. About the getPlayerName(), this function is kinda strange as it gets the username from one input field. After that, it does more random naming stuff. For the rest, I will update my code. \$\endgroup\$ – Cylion May 21 '16 at 19:55
  • \$\begingroup\$ @Cylion I'm honestly glad you can't use jQuery. Don't. Ever, if you can :-) \$\endgroup\$ – Insane May 21 '16 at 22:46
  • \$\begingroup\$ Hahaha how come? \$\endgroup\$ – Cylion May 21 '16 at 22:57
  • 1
    \$\begingroup\$ @Cylion Personal experience wise I've learned a lot more about JavaScript by doing everything in pure JavaScript rather than relying on jQuery. I'm no expert, but (some) experts share my opinion. As a beginner myself, I know using jQuery for everything won't teach me anything. So keep doing what you're doing. \$\endgroup\$ – Insane May 22 '16 at 2:01
  • \$\begingroup\$ @insane I totally agree with the learning thing. As for jquery, from my understanding, jquery's best quality was that it handled browser differences and added the convenient query selector. It still can be good at that for that of you need to support before IE9 (which is rare). But now browsers have most of jquery's features implemented, so often it isn't necessary. \$\endgroup\$ – Joshua Dawson May 22 '16 at 5:46

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.