Basic in-browser rock paper scissors

Question

I'm currently going through The Odin Project, in an attempt to learn some Javascript. I'm a junior in a CS program, and I realize that I am not very good at programming, but I want to change that.

I made a very simple rock-paper-scissors implementation with JS and HTML. I plan on adding CSS later and making it not look horribly ugly, but for the time being I am only concerned with the Javascript.

There are a few things that I am not happy with in my code, that I couldn't really figure out how to improve. One of them being the win checking; I essentially used 9 if statements for all the possible outcomes, and its very inelegant. Another thing I am not a fan of is my usage of global variables for the round results. I have been told often to avoid using global vartiables when possible. Another thing is the implementation of the eventlisteners. I wanted to just directly use the playRound() function on click, but I couldn't figure out how to pass in the computerPlayer() result as a parameter.

Here is the HTML:

<body>
    <h1>Rock Paper Scissors</h1>
    <div id="button-container">
        <button id="rock">Rock</button>
        <button id="paper">Paper</button>
        <button id="scissors">Scissors</button>
    </div>
    <div id="results">
        <p id="player-score">Your score is: 0</p>
        <p id="computer-score">The computer score is: 0</p>
        <p id="result-tie">Tie rounds: 0</p>
    </div>
    <script src="script.js"></script>
</body>

And here is the Javascript:

// Accessing the buttons and text labels
const rockButton = document.getElementById("rock");
const paperButton = document.querySelector("#paper");
const scissorsButton = document.querySelector("#scissors");
const playerScore = document.getElementById("player-score");
const computerScore = document.getElementById("computer-score");
const tieText = document.getElementById("result-tie");
// Global variables to keep track of round results that I am not a fan of
let computerWinAmount = 0;
let playerWinAmount = 0;
let tieAmount = 0;

//Event listeners to monitor button press and assign
//corresponding choice
rockButton.addEventListener("click", function(){
    let playerSelection = "rock";
    let computerSelection = computerPlay();
    playRound(playerSelection,computerSelection);
});

paperButton.addEventListener("click", function() {
    let playerSelection = "paper";
    let computerSelection = computerPlay();
    playRound(playerSelection, computerSelection);
});

scissorsButton.addEventListener("click", function() {
    let playerSelection = "scissors";
    let computerSelection = computerPlay();
    playRound(playerSelection, computerSelection);
});

// Function that pseudo-randomly generates a computer selection
function computerPlay() {
    // Uses Math.random() to generate a number between 0-2
    let choice = Math.floor(Math.random() * 3);
    // Reassigns string value depending on which number
    switch(choice) {
        case 0:
            choice = "rock";
            break;
        case 1:
            choice = "paper";
            break;
        case 2:
            choice = "scissors";
            break; 
    }
    return choice;
}

// Group of functions to change text labels after rounds
function playerWin() {
    ++playerWinAmount;
    playerScore.innerHTML = "Your score is: " + playerWinAmount;
}

function computerWin() {
    ++computerWinAmount;
    computerScore.innerHTML = "The computer score is: " + computerWinAmount;
}

function tieWin() {
    ++tieAmount;
    tieText.innerHTML = "Tie rounds: " + tieAmount;
}

// Function that checks winning very inelegantly
function playRound(playerSelection, computerSelection) {
    if (playerSelection == "rock" &&
    computerSelection == "rock") {
        tieWin();
    } else if (playerSelection == "rock" &&
    computerSelection == "scissors") {
        playerWin();
    } else if (playerSelection == "rock" &&
    computerSelection == "paper") {
        computerWin();
    }

    if (playerSelection == "paper" &&
    computerSelection == "rock") {
        playerWin();
    } else if (playerSelection == "paper" &&
    computerSelection == "paper") {
        tieWin();
    } else if (playerSelection == "paper" &&
    computerSelection == "scissors") {
        computerWin();
    }

    if(playerSelection == "scissors" &&
    computerSelection == "rock") {
        computerWin();
    } else if (playerSelection == "scissors" &&
    computerSelection == "paper") {
        playerWin();
    } else if (playerSelection == "scissors" &&
    computerSelection == "scissors") {
        tieWin();
    }

    if(computerWinAmount == 5) {
        alert("You lost! Better luck next time :(");
        reset();
    } else if (playerWinAmount == 5) {
        alert("You won! Congratulations! :)")
        reset();
    }
}

// Helper function to reset after game end
function reset() {
    playerScore.innerHTML = "Your score is: 0";
    computerScore.innerHTML = "The computer score is: 0";
    tieText.innerHTML = "Tie rounds: 0";
}
```

The functions playRound, etc. are also in global scope. Wrap the entire JS program inside a function, say RockAroundTheClock. This function must be executed to instantiate all the variables, functions, etc. A technique for doing this is called IIFE - Immediately Invoked Function Expression. GoogleIt. This is rare in code examples one comes across. This is because we get lucky and don't have some other code stomping on our variable names at run time. I've had that happen. It's not fun. — radarbob, Commented Jan 21, 2021 at 2:20
addEventListener("click", function( arguments is your friend ) - pass 'playerSelection' and 'computerSelection' as parameters. Now only one eventHandler function is needed. — radarbob, Commented Jan 21, 2021 at 2:38
"pass in the computerPlayer() result as a parameter." --- aFunction( computerPlayer() ) passes its return value; aFunction( computerPlayer ) passes the function itself. — radarbob, Commented Jan 21, 2021 at 2:42

RoToRa · Accepted Answer · 2021-01-21 12:57:39Z

Constants

The first thing to look at is the use of strings to represent the signs. The problem is, that if you mistype a string somewhere (for example, "rook" instead "rock") you'll have a bug, that you'll may be difficult to find.

Instead define constants, that you use in place of the string literals:

const ROCK = "rock";
const PAPER = "paper";
const SCISSORS = "scissors";

and that way, if you mistype the name of a constant, you'll get a runtime error with a line number so you'll know exactly where the mistake is.

Repeated code

The event listeners you are using are are fine the way they are. The problem is however you have three virtually identical listeners. Instead of hard coding the name of each sign into three copies, you can read the sign from the id of the clicked button:

function clickListener(event) {
    // Use `const` instead of `let` when the value of the variable doesn't change.
    const playerSelection = event.target.id;
    const computerSelection = computerPlay();
    playRound(playerSelection,computerSelection);
 }

rockButton.addEventListener("click", clickListener);
paperButton.addEventListener("click", clickListener);
scissorsButton.addEventListener("click", clickListener);

Further more you could instead assign a single event handler to the surrounding element (#button-container) and check in the listener, if a button has been clicked:

function clickListener(event) {
    // Exit, if not a button was clicked
    if (event.target.tagName !== "BUTTON") {
       return;
    }
    const playerSelection = event.target.id;
    const computerSelection = computerPlay();
    playRound(playerSelection,computerSelection);
 }

document.getElementbyId("#button-container").addEventListener("click", clickListener);

Simplify `computerPlay()`

computerPlay() can be simplified by using a array instead of the switch:

const signs = [ROCK, PAPER, SCISSORS];

function computerPlay() {
    // Uses Math.random() to generate a number between 0-2
    let choice = Math.floor(Math.random() * 3);
    return signs[choice];
}

Optimizing `playRound()`

The function playRound() as it is, is not bad.

The first three if/if-else/else blocks should be joined to one big if/if-else/else because as it's now, the conditions of the following blocks are unnecessarily checked, even if a previous block already determined a winner.

Always use strict comparison (=== instead of ==) when comparing values in JavaScript.

A bit of repetition can be avoided by first checking for a tie:

if (playerSelection === computerSelection) {
   tieWin();
} else if (...) {

Also you can combine the win/lose comparisons into one if to avoid repeating the check on playerSelection. (Continuing from above):

...
} else if (playerSelection === ROCK) {
  if (computerSelection === SCISSIORS) {
    playerWin();
  } else { // No need to check. If should be PAPER.
    computerWin();
  }
} else if (playerSelection === PAPER) {
   if (computerSelection === ROCK) {
    playerWin();
  } else { // No need to check. If should be PAPER.
    computerWin();
  }
} else if (...) {
   // etc.

More repetition

You have some more minor repetition with the string "Your score is:", etc. repeated three times. Instead of replacing the whole line, just change the values. Also use textContent instead of innerHtml unless you actually are writing HTML.

<div id="results">
    <p>Your score is: <span id="player-score">0</span></p>
    <!-- ... -->
</div>

playerScore.textContent = playerWinAmount;

Alternative implementation

A way to simplify playRound() would be to use numbers instead of string to identify the signs and use some mathematics to determine the winner. (Partial) Example:

<body>
    <h1>Rock Paper Scissors</h1>
    <div id="button-container">
        <button data-sign="0">Rock</button>
        <button data-sign="1">Paper</button>
        <button data-sign="2">Scissors</button>
    </div>
    <!-- ... -->
    <script src="script.js"></script>
</body>

function clickListener(event) {
    // Exit, if not a button was clicked
    if (event.target.tagName !== "BUTTON") {
       return;
    }
    const playerSelection = event.target.dataset.sign;
    const computerSelection = computerPlay();
    playRound(playerSelection,computerSelection);
 }

document.getElementbyId("#button-container").addEventListener("click", clickListener);

function computerPlay() {
    return Math.floor(Math.random() * 3);
}

function playRound(playerSelection, computerSelection) {
   const result = determineWinner(playerSelection, computerSelection);
   switch (result) {
      case 0: 
        playerWin();
        break;
     case 1:
        computerWin();
        break;
     case 2:
        tieWin();
        break;
    }

    // ...
}

// Returns: 
//   0 if the first player ("a") wins
//   1 if the second player ("b") wins
//   2 in case of a tie
function determineWinner(a, b) {
   return (a - b + 2) % 3;
}

Explanation:

The three signs are encoded (as seen in the HTML) as 0, 1 and 2. The difference (a - b) thus results in a value from 2 to -2, or (after adding 2) 4 to 0. (2 is added mostly in order to simplify the modulo operation (%) because modulo can be defined differently for negative numbers).

In case of a tie "a" and "b" are equal, so that results in a - b = 0 and 0 + 2 = 2.

In case of "a" winning, it results in 0 or 3 and both 0 % 3 and 0 % 3 result in 0.

In case of "b" winning, it results in 1 or 4 and both 1 % 3 and 4 % 3 result in 1.

Great, thank you for this! Very thorough, and I am glad to hear that what I had was not BAD per se, just needed some improvements. Thanks again! — Produces, Commented Jan 21, 2021 at 23:38
<span id="player-score">0</span> may be <output id="player-score">0</output> — tsh, Commented Mar 2, 2021 at 6:26

Scotty Jamison · Accepted Answer · 2021-01-22 03:16:40Z

Code Quality

As already pointed out, it's preferable to use === over ==.
I noticed a bug where after you win/loose the entire game, you can't keep playing. You need to reset the score, not just the UI.
You should almost never use .innerHTML due to ease of misuse resulting in security holes (XSS attacks). While that's not an issue with how you're using it here, it's best to get into the habit of using other DOM-manipulation functions instead. In this case, .innerText would be the better option.
You're using both getElementById and querySelector to get an element by id. Be consistant and just pick one.

Separate view from logic

Think of it like this: If you were to also create a command-line version of rock-paper-scissors (i.e. make a second UI for the same program), what parts of this program would be shared and what parts would be unique to the UI being used? Keep the changeable UI-dependent logic completely separated from everything else. Doing this will help improve reuse-ability, test-ability, and readability, among other things.

In my example rewrite I did this by following the model-view-controller pattern, but there's other ways to achieve this behavior.

Is there a better way to check for a winner?

If you were explaining the rules of rock-paper-scissors in English, how might you do it? Probably along the lines of "Paper beats rock, scissors beats paper, and rock beats scissors". We can put these win rules into a map, then do some lookups to determine a winner. The result is a very easy to read and understand.

const whatBeatsWhat = {
  paper: 'rock',
  scissors: 'paper',
  rock: 'scissors',
}

function getWinnerOfRound(playerSelection, computerSelection) {
  if (whatBeatsWhat[playerSelection] === computerSelection) return 'player'
  if (whatBeatsWhat[computerSelection] === playerSelection) return 'computer'
  return 'tie'
}

How to avoid global variables?

Sometimes it's impracticable to completely get rid of them, but there are things you can do to mitigate the issue if you're worried about it.

Group your global data together into an object called "state". This object represents the current state of your website. This is the only global variable that needs to be modified.
Make your functions as pure as possible. Instead of updating the global state, make them take a state object, and return a new, updated state object. Of course at some point you'll need to override the old state with the new state, but that can be done at the last moment possible.
Don't go overboard. This is just one way to do it, it's not "the way".

(I'm using the word "global" here to refer to module-level modifiable variables. Usually when we talk about global variables in javascript, we're referring to variables that are on the window object, and are accessible anywhere).

How to pass parameters to event listeners?

Like this:

function showAlert(message) {
  alert(message)
}

element1.addEventListener(() => showAlert('Hey There!'))
element2.addEventListener(() => showAlert('Hi!'))

Or this:

const showAlertFactory = message => () => {
  alert(message)
}

element1.addEventListener(showAlertFactory('Hey There!'))
element2.addEventListener(showAlertFactory('Hi!'))

I usually prefer the first style as it tends to be easier to follow, but on occasion I'll use the second style (I did in the example rewrite).

Rewrite

I separated out the model, view, and controller logic into these three separate files.

model.js

const WINNING_SCORE = 5

export const getInitialState = () => ({
  scores: {
    computer: 0,
    player: 0,
    tie: 0,
  },
})

const whatBeatsWhat = {
  paper: 'rock',
  scissors: 'paper',
  rock: 'scissors',
}

export function playRound(state, playerSelection, computerSelection) {
  const getWinnerOfRound = () => {
    if (whatBeatsWhat[playerSelection] === computerSelection) return 'player'
    if (whatBeatsWhat[computerSelection] === playerSelection) return 'computer'
    return 'tie'
  }
  const winner = getWinnerOfRound({ playerSelection, computerSelection })
  const newScores = { ...state.scores, [winner]: state.scores[winner] + 1 }
  return { ...state, scores: newScores }
}

export function getOverallWinner(state) {
  if (state.scores.player === WINNING_SCORE) return 'player'
  if (state.scores.computer === WINNING_SCORE) return 'computer'
  return null
}

view.js

const query = selector => document.querySelector(selector)

export function init(scores) {
  updateScores(scores)
}

export function updateScores(scores) {
  query('#player-score').innerText = `Your score is: ${scores.player}`
  query('#computer-score').innerText = `The computer score is: ${scores.computer}`
  query('#result-tie').innerText = `Tie rounds: ${scores.tie}`
}

export function showGameOverMessage(winner) {
  alert(winner === 'player'
    ? 'You won! Congratulations! :)'
    : 'You lost! Better luck next time :('
  )
}

controller.js

import * as model from './model.js'
import * as view from './view.js'

const randomChoice = list => list[Math.floor(Math.random() * list.length)]
const query = selector => document.querySelector(selector)

let state = model.getInitialState()

const createSelectionListener = playerSelection => () => {
  const computerSelection = randomChoice(['rock', 'paper', 'scissors'])
  state = model.playRound(state, playerSelection, computerSelection)
  view.updateScores(state.scores)

  const winner = model.getOverallWinner(state)
  if (winner) {
    view.showGameOverMessage(winner)
    state = model.getInitialState()
    view.updateScores(state.scores)
  }
}

query('#rock').addEventListener('click', createSelectionListener('rock'))
query('#paper').addEventListener('click', createSelectionListener('paper'))
query('#scissors').addEventListener('click', createSelectionListener('scissors'))
window.addEventListener('DOMContentLoaded', () => {
  view.init(state.scores)
})

index.html

<html>
    <body>
        <h1>Rock Paper Scissors</h1>
        <div id="button-container">
            <button id="rock">Rock</button>
            <button id="paper">Paper</button>
            <button id="scissors">Scissors</button>
        </div>
        <div id="results">
            <p id="player-score"></p>
            <p id="computer-score"></p>
            <p id="result-tie"></p>
        </div>
        <script src="controller.js" type="module"></script>
    </body>
</html>

(you'll notice in the index.html file I'm using <script ... type="module"> - this enabled me to use import/export syntax and also puts the scripts into strict mode. This also allows me to create module-level variables without accidentally polluting the global scope)

Stack Exchange Network

Basic in-browser rock paper scissors

2 Answers 2

Constants

Repeated code

Simplify `computerPlay()`

Optimizing `playRound()`

More repetition

Alternative implementation

Code Quality

Separate view from logic

Is there a better way to check for a winner?

How to avoid global variables?

How to pass parameters to event listeners?

Rewrite

Your Answer

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

Hot Network Questions

Basic in-browser rock paper scissors

2 Answers 2

Constants

Repeated code

Simplify computerPlay()

Optimizing playRound()

More repetition

Alternative implementation

Code Quality

Separate view from logic

Is there a better way to check for a winner?

How to avoid global variables?

How to pass parameters to event listeners?

Rewrite

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions

Simplify `computerPlay()`

Optimizing `playRound()`

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