2
\$\begingroup\$

I'm sure this game gets put up here all the time but I'd appreciate it if you could have a look over my code and give me some suggestions on how to improve it.

Here is a live example so you can play the game and here is the code on GitHub if you want to fork it and tinker with it.

(function(){

/*
 * Rock, paper, scissors  
 * 
 * The classic game recreated in Javascript for playing in the browser.
 * 
 */

// create the choices
var choices = [
    'rock',
    'paper',
    'scissors'
];

var CHOICES_LENGTH = choices.length;

// create the text for winning or drawing
var USER_WINS = "You win!";
var COMP_WINS = "Computer wins";
var DRAW = "Draw"

var MEH = '<i class="fa fa-meh-o" aria-hidden="true"></i>';
var SMILE = '<i class="fa fa-smile-o" aria-hidden="true"></i>';
var FROWN = '<i class="fa fa-frown-o" aria-hidden="true"></i>';

var score = 0;
var computer_score = 0;

var gameType;

// score elements
var userScore = getById('score');
var compScore = getById('computerScore');

userScore.textContent = score;
compScore.textContent = computer_score;

// get the game area and get access to all the buttons
var game = getById('game');
var userChoices = game.getElementsByTagName('button');

var comp = getById('computer');
var compChoices = comp.getElementsByTagName('div');

// get the results element and hide it initially
var results = getById('results');
hide(results);

// the gameover screen and final results
var gameOver = getById('gameOver');
hide(gameOver);
var gameResults = getById('gameResults');
var finalResult = getById('finalResult');

    // get the intro element and the buttons for choosing a game type
var intro = getById('intro');
var bestOf3 = getById('bestOf3');
var bestOf5 = getById('bestOf5');

// start the best of 3 game
bestOf3.onclick = function() {
    enableGame();
    gameType = 3;
}

bestOf5.onclick = function() {
    enableGame();
    gameType = 5;
}

function enableGame() {
    enable(userChoices);
    hide(intro);
    hide(gameOver);
    score = 0;
    computer_score = 0;
    userScore.textContent = score;
    compScore.textContent = computer_score;
}

// add an onclick event to each button and disable them initially
for(var i = 0; i < userChoices.length; i++) {
    userChoices[i].onclick = selection;
    userChoices[i].disabled = true;
}

function computerSelection() {
    var randomIndex = Math.floor(Math.random() * CHOICES_LENGTH);
    var compChoice = choices[randomIndex];
    return compChoice;
}

function selection() {
    // get the user and computer choice 
    var chosen = this.id;
    var comp = computerSelection();

    // get the users chosen item
    var chosenItem = getById(chosen);

    // prepare the chosenCompItem so we can assign it to a dynamic id
    var chosenCompItem;

    if(comp === 'rock') {
        chosenCompItem = getById('computerRock');
    } 
    else if(comp === 'paper') {
        chosenCompItem = getById('computerPaper');
    }
    else if(comp === 'scissors') {
        chosenCompItem = getById('computerScissors');
    }

    // show results and disable all choices so no more can 
    // be made while waiting for the pop up to fade out 
    show(results);
    reappear(results);
    disable(userChoices);
    disable(compChoices);


    // make the selected item stand out from the rest
    chosenItem.classList.add('selected');
    chosenCompItem.classList.add('selected');

    // decide who wins  


    if(chosen === comp) {
        results.textContent = DRAW;
        results.innerHTML += MEH;
        timeout();
    } 
    else if(chosen === 'rock' && comp === 'scissors') {

        results.textContent = USER_WINS;
        results.innerHTML += SMILE;

        score += 1;
        userScore.textContent = score;
        timeout();

    }
    else if(chosen === 'paper' && comp === 'rock') {

        results.textContent = USER_WINS;
        results.innerHTML += SMILE;

        score += 1;
        userScore.textContent = score;
        timeout();

    }
    else if(chosen === 'scissors' && comp === 'paper') {
        results.textContent = USER_WINS;
        results.innerHTML += SMILE;

        score += 1;
        userScore.textContent = score;
        timeout();
    }
    else {
        results.textContent = COMP_WINS;
        results.innerHTML += FROWN;

        computer_score +=1;
        compScore.textContent = computer_score;
        timeout();
    }

}


// utilities
function getById(id) {
    return document.getElementById(id);
}   

function hide(element) {
    element.style.display = 'none';
}

function show(element) {
    element.style.display = 'block';
}

function disappear(element) {
    element.className = 'disappear';
}

function reappear(element) {
    element.className = 'reappear';
}

function disable(elements) {
    for(var i = 0; i < elements.length; i++) {
        elements[i].disabled = true;
        elements[i].classList.add('unselected');
    }
}

function enable(elements) {
    for(var i = 0; i < elements.length; i++) {
        elements[i].disabled = false;
        elements[i].classList.add('default');
        elements[i].classList.remove('selected', 'unselected');
    }
}

function timeout() {
    setTimeout(function(){
        disappear(results);
            enable(userChoices);
            enable(compChoices);
            if(score + computer_score == gameType) {
                show(gameOver);
                if(score > computer_score) {
                    finalResult.textContent = "You won " + score + " - " + computer_score + "!";
                }
                else {
                    finalResult.textContent = "You lost " + computer_score + " - " + score;
                }
                gameResults.appendChild(bestOf3);
                gameResults.appendChild(bestOf5);
            }


    }, 2000)
}
})();
\$\endgroup\$
2
\$\begingroup\$

First of all you can have JavaScript run more efficiently by using less 'var' keywords.

I'm not sure how familiar you are with variable scoping in JavaScript but you could 'group' multiple variables into one 'var' keyword.

i.e

var USER_WINS = "You win!";
var COMP_WINS = "Computer wins";
var DRAW = "Draw";

becomes

var USER_WINS = "You win!",
    COMP_WINS = "Computer wins",
    DRAW = "Draw";

Notice that by using the comma (,) to separate the variable names lessens the use of how many 'var' keywords are declared. Whenever possible try to group declarations of variables to use one 'var' keyword.

When using click listeners or any other type of listener such as keyup, keydown etc try to use the 'addEventListener' method.

i.e

var bestOf3 = getById('bestOf3');
bestOf3.onclick = function() {
   enableGame();
   gameType = 3;
}

becomes

var bestOf3 = getById('bestOf3');
// first param: The type of event. In this case equivalent to 'onclick'
// second param: What to do when the event is triggered
// third param: Whether or not to use capture. Most cases leave false
bestOf3.addEventListener('click', function() {
   enableGame();
   gameType = 3;
}, false)

I recognize you are using a lot of loops a simple helper function would help eliminate the excessive use of for loops. In ES6 (new version of JavaScript) you can use the forEach method on arrays. To recreate this you can create a function that takes a function in as a paramter as well as the array. That function which it would take in would have two parameters called index and element (how ever you choose to name it)

i.e

function loop(array, function){
   for(var i = 0; i < array.length; i++){
      // use the function passed in to do what ever you want
      // pass in the index of the element and the current element
      function(i, array[i]);
   }
}

Now you can use it like this:

var fruits = ['apple', 'banana', 'orange'];
loop(fruits, function(index, element){
   // each time this method is called 'element' becomes apple, banana, orange in order for each run
   console.log(element);
});

Now you can loop without having to use excessive for loops.

Hope I helped! The game works quite great I love the User Interface for it. Very unique maybe try adding some animations to it?

\$\endgroup\$
  • \$\begingroup\$ "you can have JavaScript run more efficiently by using less 'var' keywords" -- are you sure? How does that work? \$\endgroup\$ – janos Nov 23 '16 at 23:22
  • \$\begingroup\$ I may be wrong with that statement. However when the var keyword is used JavaScript has to go back and make space in memory for it. Compared to having multiple variables defined by one var keyword consecutively eliminates the need for 'looking up and making space'. \$\endgroup\$ – Daniel Prince Nov 24 '16 at 13:58
  • \$\begingroup\$ Theoretically it may seem faster but there shouldn't be much difference other than it looks cleaner and as well eliminate the use of unwanted global variables. \$\endgroup\$ – Daniel Prince Nov 24 '16 at 13:59
  • \$\begingroup\$ I'm pretty sure that using one or more var is only a matter of style. It's true that declaring all variables with a var is recommended. I would drop the weak remark about performance. \$\endgroup\$ – janos Nov 24 '16 at 14:18
  • \$\begingroup\$ Btw, especially considering that you mentioned scope, var inside for is not recommended, as it misleads many developers about the scope of that variable. \$\endgroup\$ – janos Nov 24 '16 at 14:19

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.