HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMajor

Rock, Paper, Scissors, Lizard and Spock

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
scissorspaperspockrockandlizard

Problem

I am currently done with this little Rock, Paper, Scissors, Lizard, Spock game in JavaScript. The game is working fine, but I am not pleased with the code. Is there a way to refactor these if statements? I was thinking about ternary operators and I tried doing so but failed. This is basically my function based on which I get the results.

```
function result(userChoice, cpuChoice) {
var result = '';

if(userChoice == 'rock' ) {
if (cpuChoice == 'rock') {
result = 'Tie';
ties++;
} else if ( cpuChoice == 'spock') {
result = 'Spock vaporizes rock';
loses++;
} else if ( cpuChoice == 'lizard' ) {
result = 'Rock crushes lizard';
wins++;
} else if ( cpuChoice == 'paper' ) {
result = 'Paper covers rock';
loses++;
} else if ( cpuChoice == 'scissors' ) {
result = 'Rock crushes scissors';
wins++;
};
} else if(userChoice == 'paper') {
if (cpuChoice == 'paper') {
result = 'Tie';
ties++;
} else if ( cpuChoice == 'spock') {
result = 'Paper disproves Spock';
wins++;
} else if ( cpuChoice == 'lizard' ) {
result = 'Lizard eats paper';
loses++;
} else if ( cpuChoice == 'rock' ) {
result = 'Paper covers rock';
wins++;
} else if ( cpuChoice == 'scissors' ) {
result = 'Scissors cuts paper';
loses++;
};
} else if(userChoice == 'scissors') {
if (cpuChoice == 'scissors') {
result = 'Tie';
ties++;
} else if ( cpuChoice == 'spock') {
result = 'Spock distroys scissors';
loses++;
} else if ( cpuChoice == 'lizard' ) {
result = 'Scissors beheads lizard';
wins++;
} else if ( cpuChoice == 'rock' ) {
result = 'Rock crushes scissors'

Solution

To review the code as it is:

  • It would be better not to even have result as a variable, and swap the order of operations inside the else-ifs that assign result from (result = X, gameResult++) to (gameResult++, return X)



  • You don't need semi-colons on the end of if statements:



} else {
    return false;
};


  • Rather than individually testing (userInput == X, then X == cpuInput), remove them and add the following if statement to the top.



if (userChoice === cpuChoice){
    ties++;
    return "Tie";
}
//The rest follows


Onto fixing the if-else!

So, instead of a long if-else statement, I'd use an object, which lets you specify key and value pairs, which really cleans the process up, because you don't have to iterate over all the options before finding the one you need.

Here's what I came up with:

var results = {
    wins: 0,
    loses: 0,
    ties: 0
};

function RPSLZ(userChoice, cpuChoice) {
    var RULES = {
        rock: {
            lizard: 'Rock crushes lizard',
            scissors: 'Rock crushes scissors'
        },
        paper: {
            spock: 'Paper disproves Spock',
            rock: 'Paper covers rock'
        },
        scissors: {
            lizard: 'Scissors beheads lizard',
            paper: 'Scissors cuts paper'
        },
        lizard: {
            spock: 'Lizard poisons Spock',
            paper: 'Lizard eats paper'
        },
        spock: {
            scissors: 'Spock distroys scissors',
            rock: 'Spock vaporizes rock'
        }
    };
    if (userChoice == cpuChoice) {
        results.ties++;
        return 'Tie';
    } else if (!userChoice in RULES){
        return 'Invalid Input';
    } else {
        return (cpuChoice in RULES[userChoice]
                    ? (results.wins++, RULES[userChoice][cpuChoice])
                    : (results.loses++, RULES[cpuChoice][userChoice])
                );
    }
}



How does it work?

That's a good question. It follows the following logic flow:

  • Declare the results outside the function, so it can continue recording results with more function calls (There is a way to do this inside the function, but I couldn't figure out how.)



  • Declaring the game RULES (Notice the all-caps, meaning it's a constant)



  • There's two special rules: tie and Invalid Input. They can be tested like described above, and like testing whether the input exists inside the RULES variable, because if it didn't, it would be undefined.



  • By using the same undefined testing rule, the cpuChoice can be tested to see whether it's inside the object of the userChoice, depending on the result, the respective counter is incremented, and the result returned.

Code Snippets

} else {
    return false;
};
if (userChoice === cpuChoice){
    ties++;
    return "Tie";
}
//The rest follows
var results = {
    wins: 0,
    loses: 0,
    ties: 0
};

function RPSLZ(userChoice, cpuChoice) {
    var RULES = {
        rock: {
            lizard: 'Rock crushes lizard',
            scissors: 'Rock crushes scissors'
        },
        paper: {
            spock: 'Paper disproves Spock',
            rock: 'Paper covers rock'
        },
        scissors: {
            lizard: 'Scissors beheads lizard',
            paper: 'Scissors cuts paper'
        },
        lizard: {
            spock: 'Lizard poisons Spock',
            paper: 'Lizard eats paper'
        },
        spock: {
            scissors: 'Spock distroys scissors',
            rock: 'Spock vaporizes rock'
        }
    };
    if (userChoice == cpuChoice) {
        results.ties++;
        return 'Tie';
    } else if (!userChoice in RULES){
        return 'Invalid Input';
    } else {
        return (cpuChoice in RULES[userChoice]
                    ? (results.wins++, RULES[userChoice][cpuChoice])
                    : (results.loses++, RULES[cpuChoice][userChoice])
                );
    }
}

Context

StackExchange Code Review Q#102057, answer score: 22

Revisions (0)

No revisions yet.