After a long time, I once again did a few code reviews on Code Review Stack Exchange. One of them (a beginner’s implementation of a rock-paper-scissors game) stuck out, since it contained many of the typical rookie mistakes I see in code from newcomers (although, some of them even in code from more experienced programmers).
I thought I’d share with you the original code and my review of it. If you’re an experienced programmer, this might not be very interesting for you, but maybe you know someone who might find it helpful.
Note: The following code and text is provided under CC BY-SA 4.0. The question was stated by user24687626, the answer was written by me.
rock paper scissors game in console
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();
My code review
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.
Raimund Krämer 