11
\$\begingroup\$

I am currently building my first game/app in javascript. It is a very simple script that has about 130~ lines of code. I feel like what I did so far is not optimized, can anyone please help me optimize the code also, can you please explain your changes?

How the game works: At the start of the game every player gets 20 random cards from the cards array. Every time the player clicks on the Attack button the top card from the player and the computer are compared the different of the number is then converted in dmg. Let's say I draw a 4 and the computer a 3, the computer will receive 1 dmg point. Whenever the computer or the player's HP reach 0 the game is over. Functions that I still have to build but that I can't figure it out for now are: the players top card is always visible and the second function I want is a dodge one. So the player can choose dodge left or right, the game picks one at random and if the player got the right one he will deal the full dmg from his card and receives 0 dmg. But if he got it wrong he will get the full dmg and deal 0 dmg.

Game Screenshot: enter image description here

var cards = [1, 1, 2, 2, 3, 3, 4, 4, 5, 5, 6, 6, 7, 7, 8, 8, 9, 9, 10, 10,
             1, 1, 2, 2, 3, 3, 4, 4, 5, 5, 6, 6, 7, 7, 8,8, 9, 9, 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 sword = new Audio('sounds/sword.mp3');
var block = new Audio('sounds/block.mp3');

function getPlayerName() {
    playerName = document.getElementById('userInput').value;
    if (playerName.length == 0) {
        return false;
    }
    document.getElementById('playerName').innerHTML = playerName;
    document.getElementById('playerName').innerHTML = playerName;
    document.getElementById('overlay').style.display = 'none';
}

function shuffle(array) {
    var m = array.length, t, 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;
}

function getTopCard(deck) {
    return deck[0];
}

function dealCards() {
    var playerTopCard = getTopCard(playerDeck);
    var computerTopCard = getTopCard(computerDeck);
    var cardImage = "<img src='images/cardBack.png'/>";

    whoWon();

    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 whoWon() {
    var playerTopCard = getTopCard(playerDeck);
    var computerTopCard = getTopCard(computerDeck);
    document.getElementById('computerDamageImage').setAttribute('src', 'images/empty.png');
    document.getElementById('playerDamageImage').setAttribute('src', 'images/empty.png');

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

shuffle(cards);

document.getElementById("click").addEventListener("click", dealCards);
document.getElementById("replay").addEventListener("click", replay);
document.getElementById("userSubmit").addEventListener("click", getPlayerName);
 function replay() {
     window.location.reload();
 }

function checkIsGameOver() {
    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();
    }
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Row 19 is a duplicate I believe \$\endgroup\$ May 19, 2016 at 23:56
  • \$\begingroup\$ Hello! Please don't edit in changes to your code after it has been reviewed, as it invalidates answers! See this meta post for more information. \$\endgroup\$
    – syb0rg
    May 20, 2016 at 20:34
  • \$\begingroup\$ Woops sorry! din't know... Should I put the old one back? EDIT: Never mind the old one is already back. \$\endgroup\$
    – Cylion
    May 20, 2016 at 20:51
  • \$\begingroup\$ Follow-up question \$\endgroup\$ May 21, 2016 at 20:56

1 Answer 1

6
\$\begingroup\$

There has always been an interesting debate between number of lines and legibility of the code. You could have less lines but not a readable or more lines and bad code. Don't focus so much on the number of lines but on the efficiency and scalability of the code.

Some ways to possibly improve are:

  • Suggestion: You could reduce some code if you implement jQuery (It'll make certain things simpler, but not necessarily reduce number of lines)
    • Example: Instead of writing document.getElementById('name') everytime, with jQuery you can simply do $('name')
  • The code document.getElementById('playerName').innerHTML = playerName; is repeated twice
  • In the whoWon() some lines of code are repeated in every condition such as playerDeck.shift(); and computerDeck.shift();. This can be written once after all the conditions are done.
  • When you validate playerName.length == 0, this can be bypassed if spaces are entered, try to trim the value input.value.trim == ''
  • Maybe add documentation to function headers

Side note: The game looks interesting and the design is nice. Good luck with it :)

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the tip! I will fix them right now. Just wondering, as you can see I have one very big if-statement is this ok? Should I maybe make it shorter using more functions? Also what I was wondering should I use var's to store my dom locations? Right now I use getElementbyID like 100000 times. \$\endgroup\$
    – Cylion
    May 20, 2016 at 18:03
  • \$\begingroup\$ @Cylion You could try storing the reference to the object retrieved by getElementById through a variable or like I said use $() w/ jQuery. I personally don't store them in variables as I use variables for what really needs to be stored and used such as numbers, names, etc. For the if I am not sure which one you are specifically referring to. \$\endgroup\$ May 24, 2016 at 20:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.