4
\$\begingroup\$

I've made a simple rock paper scissors game using javascript. Any feedback appreciated. This is a first project I've done with almost no help

let computerScore = 0;
let humanScore = 0;

function getComputerChoice() {
  let computerChoice = Math.floor((Math.random() * 100)/40); //generates a number between 0 and 2

  if (computerChoice === 0) {
    computerChoice = 'rock';
  } else if (computerChoice === 1) {
    computerChoice = 'paper';
  } else {
    computerChoice = 'scissors';
  }

  return computerChoice;
}


function getHumanChoice() {
  let humanChoice = prompt('Choose rock, paper, or scissors:');

  if (humanChoice === null) return null; // if player clicks cancel on the prompt stop the function

  humanChoice = humanChoice.toLowerCase(); // makes case insensitive

  while (humanChoice !== 'rock' && humanChoice !== 'paper' && humanChoice !== 'scissors') {
    alert('Please enter a valid choice');
    humanChoice = prompt('Choose rock, paper, or scissors:');
  }

  return humanChoice;
}


function playRound(humanChoice, computerChoice) {
  let result;

  if (humanChoice === computerChoice) {
    result = 'it\'s a tie.';
  } else if (humanChoice === 'rock' && computerChoice === 'scissors' || humanChoice === 'paper' && computerChoice === 'rock' || humanChoice === 'scissors' && computerChoice === 'paper') {
    result = 'you win.';
    humanScore++;
  } else {
    result = 'you loose.'
    computerScore++;
  } 

  console.log(` You chose ${humanChoice} computer chose ${computerChoice} ${result}`)

  console.log(` Your score: ${humanScore}\n Computer score: ${computerScore}`);
}


function playGame() {
  // calls playRound function until someone gets the score of 3
  while (computerScore < 3 && humanScore < 3) {
    const humanTurn = getHumanChoice();
    const computerTurn = getComputerChoice();

    if (humanTurn === null) { // if prompt was canceled function stops
      console.log("Game exited.");
      return;
    }

    playRound(humanTurn, computerTurn);
  } 

  if (computerScore < humanScore) {
    alert('You win!');
  } else if (computerScore > humanScore) {
    alert('Computer wins :\'(');
  } else {
    alert('It\'s a tie.');
  }
}

playGame();
  
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$
Math.floor((Math.random() * 100)/40);

This will not result in equal probabilities of rock, paper and scissors. It's also more complicated than it could be.

Math.floor(Math.random() * 3); should do the trick. Since it is exclusive on the upper bound, this should result in numbers from (roughly) 0.0 to 2.99999, which are then rounded down equally to 0, 1, or 2.


It can be very confusing to use the variable computerChoice to first hold the random number, and then use the same variable to hold the respective string, only to then return it.

Consider returning immediately:

function getComputerChoice() {
  let computerChoice = // ...

  if (computerChoice === 0) {
    return 'rock';
  } else if (computerChoice === 1) {
    return 'paper';
  } else {
    return 'scissors';
  }
}

Consider changing the order of the functions so that the logic is easier to follow when read top to bottom. It looks like each time you created a new function you added it at the top.

I would define them in this order, high-level to detailed:

function playGame() {
  // ...
}

function playRound() {
  // ...
}

function getComputerChoice() {
  // ...
}

function getHumanChoice() {
  // ...
}

// calls playRound function until someone gets the score of 3

This comment doesn't really hurt, but it's also not necessary. It's fairly obvious from the code that this is what it does, and if you ever decide to change the required score to 5 (for example), or rename the playRound() function, there's a risk that you forget to also update the comment, which can be very confusing.


You might be able to simplify this long expression

humanChoice !== 'rock' && humanChoice !== 'paper' && humanChoice !== 'scissors'

as something like this:

if (!['rock', 'paper', 'scissors'].includes(humanChoice)) {
  // ...
}

function getHumanChoice() {
  let humanChoice = prompt('Choose rock, paper, or scissors:');

  if (humanChoice === null) return null; // if player clicks cancel on the prompt stop the function

  humanChoice = humanChoice.toLowerCase(); // makes case insensitive

  while (humanChoice !== 'rock' && humanChoice !== 'paper' && humanChoice !== 'scissors') {
    alert('Please enter a valid choice');
    humanChoice = prompt('Choose rock, paper, or scissors:');
  }

  return humanChoice;
}

There is some duplication with the same prompt being used twice. Also, you only check once whether the player cancels the prompt (humanChoice is null) and only convert it to lower case once. If the first input was invalid (let's say a typo like "Rick" instead of "Rock"), the player will stay in the loop forever even when typing "Rock" afterwards. And by cancelling the prompt after the first invalid input, they will also stay in the loop, since the choice will be null and thus not one of the options. (Note that I didn't execute the code, but this is how I understand it from reading it.)

Let's simplify it and fix those problems:

function getHumanChoice() {
  while (true) {
    let humanChoice = prompt('Choose rock, paper, or scissors:');
    if (humanChoice === null) {
      return null;
    }
    if (['rock', 'paper', 'scissors'].includes(humanChoice.toLowerCase())) {
      return humanChoice.toLowerCase();
    }
    alert('Please enter a valid choice');
  }
}

Note that I didn't test it, but I hope the intention is clear.


const humanTurn = getHumanChoice();
const computerTurn = getComputerChoice()

I suggest sticking to one consistent naming pattern, i.e., either humanTurn/computerTurn, or humanChoice/computerChoice. The second option seems more appropriate to me.

\$\endgroup\$

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