HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Card-fighting game Part 2

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
gamefightingcardpart

Problem

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

Solution

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
fun

Code Snippets

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

sounds.sword.play(); // usage
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);
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];
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();
}
// 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));
}

Context

StackExchange Code Review Q#128955, answer score: 6

Revisions (0)

No revisions yet.