3
\$\begingroup\$

This is a console version of the famous Rock Paper Scissors game part of The Odin Project curriculum. Please review this code and provide suggestions. Please comment whether the code is DRY and if it is simplified.

function computerPlay() {
    let choices = ['rock', 'paper', 'scissors'];
    return choices[Math.floor(Math.random() * choices.length)];
}

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'};
    
    playerSelection = playerSelection.toLowerCase();
    
    if(playerSelection === computerSelection) return "Oh! It's a tie";
    else if(playerSelection === winningChoice[computerSelection]) return winStatement(playerSelection, computerSelection);
    else return loseStatement(computerSelection, playerSelection);
}

const playerSelection = 'rock';
const computerSelection = computerPlay();
console.log(playRound(playerSelection, computerSelection));

\$\endgroup\$
  • \$\begingroup\$ Welcome to CodeReview@SE. Can you tell a) why you coded this b) what concerns about it make you ask for suggestions? \$\endgroup\$ – greybeard Jan 4 at 19:03
  • \$\begingroup\$ (This is the third Rock, Paper, Scissors implementation I notice here in a week.) \$\endgroup\$ – greybeard Jan 4 at 19:03
  • \$\begingroup\$ @greybeard This is part of The Odin Project curriculum. I would like to know if there's a more simplified code for the question. \$\endgroup\$ – tcd Jan 4 at 19:33
  • 3
    \$\begingroup\$ Inspecting a solution for possible simplifications: look for things that occur more than once: literally, structurally, temporally. Check whether what you read is what you want have said. When asked something in comments, consider enhancing your post first. When you find your post to be clear and complete enough, comment on comments - but, fundamentally, this is not chat. \$\endgroup\$ – greybeard Jan 4 at 21:35
  • \$\begingroup\$ Noted @greybeard. \$\endgroup\$ – tcd Jan 5 at 7:04
3
\$\begingroup\$

All in all, a basic review of your question shows that you are creating objects numerous times, when they do not need to be. Other things are pointed out in comments.

const choices = ['rock', 'paper', 'scissors'],
      capitalize = word => word.charAt(0).toUpperCase() + word.slice(1),
      // Should these be made into 1 function, that takes another argument, 'condition', which is 'Win' or 'Lose'?
      winStatement = (winner, loser) => `You Win! ${capitalize(winner)} beats ${capitalize(loser)}`,
      loseStatement = (winner, loser) => `You Lose! ${capitalize(winner)} beats ${capitalize(loser)},
      winningChoice = {'rock': 'paper', 'paper': 'scissors', 'scissors': 'rock'};

function computerPlay() {
    return choices[Math.floor(Math.random() * choices.length)];
}

function playRound(playerSelection, computerSelection) {
    playerSelection = playerSelection.toLowerCase();

    // Also, Consecutive if statements that all return need not be else-if.
    if(playerSelection === computerSelection) {
        return "Oh! It's a tie";
    }
    if(playerSelection === winningChoice[computerSelection]) {
        return winStatement(playerSelection, computerSelection);
    }
    return loseStatement(computerSelection, playerSelection);
}

// Should be improved upon? Actually take input?
const playerSelection = 'rock';
const computerSelection = computerPlay();
console.log(playRound(playerSelection, computerSelection));
| improve this answer | |
\$\endgroup\$
2
\$\begingroup\$
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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the in-depth report - I always get a nag when I make an answer that is just a scraping and fearing that the user might accept it and think that is all there is to fix - and that the community may think that it is all - but at the same time I don't want to not put it down, because it may get missed. \$\endgroup\$ – FreezePhoenix Jan 6 at 1:47
  • \$\begingroup\$ However, I personally would not use continuous ternaries like that, I would use parenthesis. Variables names are not as much of a problem really \$\endgroup\$ – FreezePhoenix Jan 6 at 2:18

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.