22
\$\begingroup\$

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';
            loses++;
        }  else if ( cpuChoice == 'paper' ) {
            result = 'Scissors cuts paper';
            wins++;
        };  
    } else if(userChoice == 'lizard') {
        if (cpuChoice == 'lizard') {
            result = 'Tie';
            ties++;
        } else if ( cpuChoice == 'spock') {
            result = 'Lizard poisons Spock';
            wins++;
        } else if ( cpuChoice == 'scissors' ) {
            result = 'Scissors beheads lizard' ;
            loses++;
        }  else if ( cpuChoice == 'rock' ) {
            result = 'Rock crushes lizard';
            loses++;
        }  else if ( cpuChoice == 'paper' ) {
            result = 'Lizard eats paper';
            wins++;
        };  
    } else if(userChoice == 'spock') {
        if (cpuChoice == 'spock') {
            result = 'Tie';
            ties++;
        } else if ( cpuChoice == 'lizard') {
            result = 'Lizard poisons Spock';
            loses++;
        } else if ( cpuChoice == 'scissors' ) {
            result = 'Spock distroys scissors';
            wins++;
        }  else if ( cpuChoice == 'rock' ) {
            result = 'Spock vaporizes rock';
            wins++;
        }  else if ( cpuChoice == 'paper' ) {
            result = 'Paper disproves Spock';
            loses++;
        };  
    } else {
        return false;
    };

    return result;
};
\$\endgroup\$
22
\$\begingroup\$

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:

  1. 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.)
  2. Declaring the game RULES (Notice the all-caps, meaning it's a constant)
  3. 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.
  4. 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.
\$\endgroup\$
  • 2
    \$\begingroup\$ You shouldn't use typeof RULES[userChoice] === 'undefined'. You should use the in operator. \$\endgroup\$ – Ismael Miguel Aug 27 '15 at 14:25
  • 1
    \$\begingroup\$ You can also get rid of your last two elses \$\endgroup\$ – Kevin Aug 27 '15 at 21:04
7
\$\begingroup\$

I've posted this answer for review on: Rock, Paper, Scissors, Lizard and Spock with OOP

One thing that I quickly noticed is the repetitiveness. One that highly ticked me off is the amount of tries. You could use an early return, like this:

if(userChoice == cpuChoice) {
    tries++;
    return 'Tie';
}

By doing that, you can clean up the ties!


You have misspelled destroys twice.


For your code to work, you have to know what's what.

You can try to create a fully Object-Oriented thing:

function RockPaperScissors(name, hard_action, soft_action) {
    this.name = (name + '').toLowerCase();
    this.hard_action = hard_action + '';
    this.soft_action = (soft_action || hard_action) + '';
    this.strengths = {};
    this.weaknesses = {};
}

RockPaperScissors.prototype = {
    getName: function(){
        return this.name;
    },
    addStrength: function(strength) {
        if( !(strength instanceof RockPaperScissors) ) {
            throw new TypeError('A strength must be an instance of RockPaperScissors');
        }

        this.strengths[strength.getName()] = strength;
    },
    getStrengths: function() {
        var strengths = {};
        for(var k in this.strengths)
        {
            strengths[k] = this.strengths[k];
        }
        return strengths;
    },
    addWeakness: function(weakness, soft) {
        if( !(weakness instanceof RockPaperScissors) ) {
            throw new TypeError('A weakness must be an instance of RockPaperScissors');
        }

        this.weaknesses[weakness.getName()] = {
            enemy: weakness,
            soft: !!soft
        };
    },
    getWeaknesses: function() {
        var weaknesses = {};
        for(var k in this.weaknesses)
        {
            weaknesses[k] = this.weaknesses[k];
        }
        return weaknesses;
    },
    getAction(soft){
        return soft? this.soft_action : this.hard_action;
    },
    winsTo: function(enemy) {
        if( !(enemy instanceof RockPaperScissors) ) {
            throw new TypeError('An enemy must be an instance of RockPaperScissors');
        }

        return (enemy.getName() in this.strengths);
    }
};

function RockPaperScissorsGame(){
    this.score = {
        won: 0,
        lost: 0,
        tied: 0
    };
    this.games = 0;
}

RockPaperScissorsGame.prototype = {
    match: function(user, enemy){
        if( !(user instanceof RockPaperScissors) ) {
            throw new TypeError('The user must be an instance of RockPaperScissors');
        }
        if( !(enemy instanceof RockPaperScissors) ) {
            throw new TypeError('The enemy must be an instance of RockPaperScissors');
        }

        this.games++;

        if( user == enemy ) {
            this.score.tied++;
            return 'Tie';
        }

        var userName = user.getName();
        var enemyName = enemy.getName();

        if( user.winsTo(enemy) ) {
            this.score.won++;
            var enemyWeakness = enemy.getWeaknesses()[userName] || {enemy: enemy, soft: false};

            return userName + ' ' + user.getAction(enemyWeakness.soft) + ' ' + enemyName;

        } else {
            this.score.lost++;
            var userWeakness = user.getWeaknesses()[enemyName] || {enemy: enemy, soft: false};

            return enemyName + ' ' + enemy.getAction(userWeakness.soft) + ' ' + userName;
        }
    },
    getMatches: function(){
        return this.games;
    },
    getWins: function(){
        return this.won;
    },
    getLosses: function(){
        return this.lost;
    },
    getTies: function(){
        return this.tied;
    }
};

This allows for more abstraction, since you don't have to know that, in a specific case, this wins to that and all the conditions.

You run a method, on a new game, and it does everything for you. You just have to worry to capitalizing it properly. You simply have to create the elements, add strengths and weaknesses and you're done!

To run it, you simply can do like this:

function RockPaperScissors(name, hard_action, soft_action) {
	this.name = (name + '').toLowerCase();
	this.hard_action = hard_action + '';
	this.soft_action = (soft_action || hard_action) + '';
	this.strengths = {};
	this.weaknesses = {};
}

RockPaperScissors.prototype = {
	getName: function(){
		return this.name;
	},
	addStrength: function(strength) {
		if( !(strength instanceof RockPaperScissors) ) {
			throw new TypeError('A strength must be an instance of RockPaperScissors');
		}
		
		this.strengths[strength.getName()] = strength;
	},
	getStrengths: function() {
		var strengths = {};
		for(var k in this.strengths)
		{
			strengths[k] = this.strengths[k];
		}
		return strengths;
	},
	addWeakness: function(weakness, soft) {
		if( !(weakness instanceof RockPaperScissors) ) {
			throw new TypeError('A weakness must be an instance of RockPaperScissors');
		}
		
		this.weaknesses[weakness.getName()] = {
			enemy: weakness,
			soft: !!soft
		};
	},
	getWeaknesses: function() {
		var weaknesses = {};
		for(var k in this.weaknesses)
		{
			weaknesses[k] = this.weaknesses[k];
		}
		return weaknesses;
	},
	getAction(soft){
		return soft? this.soft_action : this.hard_action;
	},
	winsTo: function(enemy) {
		if( !(enemy instanceof RockPaperScissors) ) {
			throw new TypeError('An enemy must be an instance of RockPaperScissors');
		}
		
		return (enemy.getName() in this.strengths);
	}
};

function RockPaperScissorsGame(){
	this.score = {
		won: 0,
		lost: 0,
		tied: 0
	};
	this.games = 0;
}

RockPaperScissorsGame.prototype = {
	match: function(user, enemy){
		if( !(user instanceof RockPaperScissors) ) {
			throw new TypeError('The user must be an instance of RockPaperScissors');
		}
		if( !(enemy instanceof RockPaperScissors) ) {
			throw new TypeError('The enemy must be an instance of RockPaperScissors');
		}
		
		this.games++;
		
		if( user == enemy ) {
			this.score.tied++;
			return 'Tie';
		}
		
		var userName = user.getName();
		var enemyName = enemy.getName();
		
		if( user.winsTo(enemy) ) {
			this.score.won++;
			var enemyWeakness = enemy.getWeaknesses()[userName] || {enemy: enemy, soft: false};
			
			return userName + ' ' + user.getAction(enemyWeakness.soft) + ' ' + enemyName;
			
		} else {
			this.score.lost++;
			var userWeakness = user.getWeaknesses()[enemyName] || {enemy: enemy, soft: false};
			
			return enemyName + ' ' + enemy.getAction(userWeakness.soft) + ' ' + userName;
		}
	},
	getMatches: function(){
		return this.games;
	},
	getWins: function(){
		return this.score.won;
	},
	getLosses: function(){
		return this.score.lost;
	},
	getTies: function(){
		return this.score.tied;
	}
};


var rock = new RockPaperScissors('rock', 'crushes');
var paper = new RockPaperScissors('paper', 'wraps', 'disproves');
var scissor = new RockPaperScissors('scissor', 'cuts', 'beheads');

scissor.addStrength(paper);
rock.addStrength(scissor);
paper.addStrength(rock);

var game = new RockPaperScissorsGame();

alert([
	game.match(rock,paper),
	game.match(rock,rock),
	game.match(scissor,paper),
	game.getWins(),
	game.getLosses(),
	game.getTies(),
	game.getMatches()
].join('\r\n'));

By doing this, you abstract the winning conditions and the gameplay, easing the programming, readability, mantainability and flexibility. You can add as many enemies as you like, with all sorts of strengths and weaknesses.

\$\endgroup\$
5
\$\begingroup\$

Use Javascript's switch statement instead of long if else chains. It basically allows you to pass in one parameter ie. userChoice or cpuChoice and then run different code based on what value it is.

    switch (cpuChoice) {
        case 'rock':
            result = 'Tie';
            ties++;
            break;
        case 'spock':
            result = 'Spock vaporizes rock';
            loses++;
            break;
        case 'lizard':
            result = 'Rock crushes lizard';
            wins++;
            break;
        case 'paper':
            result = 'Paper covers rock';
            loses++;
            break;
        case 'scissors':
            result = 'Rock crushes scissors';
            wins++;
            break;
    }

You can also nest switch statements to put switches inside a switch's case block. You can also use default: as your last case, if none of the other cases evaluated as true.

\$\endgroup\$
  • 1
    \$\begingroup\$ I will try and see if I can do it your way as well, I find it´s a good practice to try several ways to get to the same result. Cheers! Will post the code here after I am pleased with it :D. \$\endgroup\$ – Daria M Aug 27 '15 at 11:48
1
\$\begingroup\$

You can get rid of the if-else entirely if you imagine the objects being on a wheel in a particular order. You then figure out who wins by comparing the distance each symbol is away from the other (negative, equal or positive).

Here is some Python Code:

I know this is tagged JavaScript, but it's easy enough to translate to JavaScript.

# The key idea of this program is to equate the strings
# "rock", "paper", "scissors", "lizard", "Spock" to numbers
# as follows:
#
# 0 - rock
# 1 - Spock
# 2 - paper
# 3 - lizard
# 4 - scissors

import random

# helper functions

def number_to_name(number):
    """
    Converts a number to its name equivalent
    and returns said name
    """

    if number == 0:
        return "rock"
    elif number == 1:
        return "Spock"
    elif number == 2:
        return "paper"
    elif number == 3:
        return "lizard"
    elif number == 4:
        return "scissors"
    else:
        return "That is not a valid number!"


def name_to_number(name):
    """
    Converts a name to its number equivalent
    and returns said number
    """

    if name == "rock":
        return 0
    elif name == "Spock":
        return 1
    elif name == "paper":
        return 2
    elif name == "lizard":
        return 3
    elif name == "scissors":
        return 4
    else:
        return "That is not a valid name!"


def rpsls(name): 

    # convert name to player_number
    player_number = name_to_number(name)

    # compute random guess for comp_number
    comp_number = random.randrange(0, 4)

    # compute difference of player_number and comp_number modulo five
    difference = (player_number - comp_number) % 5

    # determine winner
    if difference == 0:
        winner = "It's a tie!"
    elif difference <= 2:
        winner = "Player wins!"
    elif difference >= 3:
        winner = "Computer wins!"
    else:
        return "Something went horribly wrong"

    # convert comp_number to name using number_to_name
    comp_name = number_to_name(comp_number)

    # print results
    print ""
    print "Player chooses", name
    print "Computer chooses", comp_name
    print winner


# test the code
rpsls("rock")
rpsls("Spock")
rpsls("paper")
rpsls("lizard")
rpsls("scissors")
\$\endgroup\$
  • \$\begingroup\$ please include code that you wish to be part of your answer, a link can be added as well, but keep in mind that links sometimes rot. \$\endgroup\$ – Malachi Aug 27 '15 at 15:56
  • \$\begingroup\$ Python and Javascript are different languages. One of the differences is the print statement. There's no such thing in Javascript. There is, however, the document.writeln() but that is a bad practice. \$\endgroup\$ – Ismael Miguel Aug 27 '15 at 16:18
  • 4
    \$\begingroup\$ The key point is the method to determine who wins... Not the stupid way to print the winner. \$\endgroup\$ – Chimera Aug 27 '15 at 16:21
  • 1
    \$\begingroup\$ You can easily remove those number_to_name() and name_to_number() functions with arrays (using array.indexOf() for the reverse lookup) \$\endgroup\$ – IQAndreas Aug 28 '15 at 7:03
  • \$\begingroup\$ @IQAndreas Thanks. You are correct. I wasn't focusing on other optimizations other than showing a way to avoid if-else or lookup methods to determine the winner of a game. \$\endgroup\$ – Chimera Aug 28 '15 at 19:51
1
\$\begingroup\$

To "flatten" the rules, and avoid nested switch statements, try something like:

switch (userChoice + cpuChoice) {
  case "rockscissors":
    msg = "Rock crushes scissors";
    wins++;
    break;
  case "scissorsrock":
    msg = "Rock crushes scissors";
    losses++;
    break;    
  case "spockrock":
    msg = "Spock vaporizes rock";
    wins++;
    break;
// etc
}

Why? It's a lot easier to have one rule entry per atomic rule. You could put all the possible responses into an object, and have a much more generic function to calculate the outcome:

var rules = {
  rockscissors: {
    msg: "Rock crushes scissors", score: 1
  },
  scissorsrock: {
    msg: "Rock crushes scissors", score: -1
  },
  spockspock: {
    msg: "Tie", score: 0
  }
};

var r = rules[userChoice + cpuChoice];

if (r) {
  msg = r.msg;
  wins += r.score == 1 ? 1 : 0;
  losses += r.score == -1 ? 1 : 0;
  ties += r.score == 0 ? 1 : 0;
}
\$\endgroup\$
1
\$\begingroup\$

Despite there being an already accepted answer with 14 upvotes, I feel obliged to point out this this program is screaming out for a lookup table.

This code should be data driven and could possibly be reduced to a single line (certainly to just a few).

Why not ...

if (superior[spock][lizard])

to show what I mean (it is always false).

In your case

if (superior[userChoice][cpuChoice])

I leave it as an exercise for the reader to hard code the array superior[][], and to build an apporpriate text string.

\$\endgroup\$

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.