function computerPlay() {
let choices = ['rock', 'paper', 'scissors'];
return choices[Math.floor(Math.random() * choices.length)];
}
In the above function, you create the choices array again and again. This is a waste of time and memory. It's better to create the choices once and for all:
const choices = ['rock', 'paper', 'scissors'];
function computerPlay() {
return choices[Math.floor(Math.random() * choices.length)];
}
On the other hand, the JavaScript compiler may be able to prove that creating this array each time is not necessary. In that case the code might stay as it is. I don't know how advanced the optimization techniques for JavaScript compilers are in 2020. You would have to measure this using a memory profiler.
The playRound function is quite big, and having all these inner functions does not increase the readability. Especially since you are inconsistent: The message for a tie is in the lower half while the two other messages are in the upper half. All these messages belong together, therefore they should be written in the same area of the code.
function playRound(playerSelection, computerSelection) {
const capitalize = word => { return word.charAt(0).toUpperCase() + word.slice(1); }
const winStatement = (winner, loser) => { return `You Win! ${capitalize(winner)} beats ${capitalize(loser)}`; }
const loseStatement = (winner, loser) => { return `You Lose! ${capitalize(winner)} beats ${capitalize(loser)}`; }
const winningChoice = {'rock': 'paper', 'paper': 'scissors', 'scissors': 'rock'};
In all these inner functions, you can remove the { return at the beginning and the } at the end. This makes them shorter and easier to understand since they now look more similar to the mathematical concept of a function, which is written as \$f\colon A \to B\$, and not \$f\colon A \to \left\{ \mathop{\text{return}} B \right\}\$.
The winningChoice should be moved outside of this function, just like the choices in computerPlay.
playerSelection = playerSelection.toLowerCase();
Having to deal with uppercase and lowercase words makes this function more complicated than necessary. Normalizing the user input should be done somewhere else. This way, the playRound function can focus on its single purpose, which is deciding the outcome of the game.
if(playerSelection === computerSelection) return "Oh! It's a tie";
else if(playerSelection === winningChoice[computerSelection]) return winStatement(playerSelection, computerSelection);
else return loseStatement(computerSelection, playerSelection);
}
This code looks quite condensed. The usual style is to write if (, with a space.
A general guideline is that your code should not require horizontal scrollbars. Therefore you should place all the return statements into separate lines, like this:
if (playerSelection === computerSelection)
return "Oh! It's a tie";
else if (playerSelection === winningChoice[computerSelection])
return winStatement(playerSelection, computerSelection);
else
return loseStatement(computerSelection, playerSelection);
This way you can clearly see the structure of the code by reading only the leftmost word of each line: if return else return else return.
When I copied your code duringthe review, I made some other changes, and this is the final result:
const choices = ['rock', 'paper', 'scissors'];
const beats = {'rock': 'paper', 'paper': 'scissors', 'scissors': 'rock'};
const upper = {'rock': 'Rock', 'paper': 'Paper', 'scissors': 'Scissors'};
function computerPlay() {
return choices[Math.floor(Math.random() * choices.length)];
}
function playRound(human, computer) {
return human === computer
? `Oh! It's a tie`
: human === beats[computer]
? `You win! ${upper[human]} beats ${computer}`
: `You lose! ${upper[computer]} beats ${human}`;
}
const playerSelection = choices[0];
const computerSelection = computerPlay();
console.log(playRound(playerSelection, computerSelection));
As you can see, I decided to only capitalize one of the words, and I also created a fixed translation table instead of capitalizing the words on-the-fly. Again, I did this to avoid unnecessary object creation. This kind of performance optimization is not necessary for a simple Rock Paper Scissors game, but I tried to be consistent in the code. Oh well, except that the You win and You lose strings are still templates that have to be created dynamically.
I also made the variable names a bit shorter: human instead of playerSelection. I removed the selection since that was not an essential part of the name. I renamed the variable from player to human since both the human and the computer are players of the game, which makes the variable name player ambiguous.
In playRound it's a stylistic choice whether to use the ?: operator or an if else chain. At first I used an if (human === computer) return 'tie' combined with a ?: operator for deciding between you win and you lose. I didn't like this mixture because it felt inconsistent. I had to settle on either using the ?: operator throughout or the if else chain. I chose the ?: operator because it is shorter. On the other hand, the if else chain would have probably been clearer.
