9
\$\begingroup\$

I have looked at many different approaches to this game online, but as someone who isn't very experienced with Python, I may not be aware of what others are doing right.

import random

def RPS():
    print("You are playing Rock Paper Scisscors")
    comp_possible  = 1,2,3
    score = [0,0]
    flag = 0
    while True:
        print("Enter your choice:")
        while True:
            choice = input('->')
            if choice == 'r' or choice == 'R' or choice == 'Rock' or choice == 'rock' or choice == '1':
                choice_identifier  = 1
                break
            elif choice == 'S' or choice == 's' or choice == 'Scissors' or choice == 'sciccors' or choice == '2':
                choice_identifier =  2
                break
            elif choice == 'P' or choice == 'p' or choice == 'Paper' or choice == 'paper' or choice == '3':
                choice_identifier =  3
                break
            else:
                print('That\'s not an option in this game :)')
                print('Try again:')
                continue

        comp_choice = random.choice(comp_possible)
        if choice_identifier == comp_choice:
            print('It\'s a draw!')
            score[0] = score[0] + 1
            score[1] = score[1] + 1
        elif (choice_identifier == 1 and comp_choice == 2) or (choice_identifier == 2 and comp_choice == 3) or (choice_identifier == 3 and comp_choice == 1):
            print('You win!')
            score[0] = score[0] + 1
        else:
            print('You lose...')
            score[1] = score[1] + 1


        while True:
            answer = input('Play another round?')
            if answer == 'y' or answer == 'Y' or answer == 'yes' or answer == 'Yes' or answer == 'ye' or answer == 'Ye' or answer == 'sure' or answer == 'Sure':
                print(' Current score: You - ',score[0],' Computer - ',  score[1])
                flag = 0
                break
            elif answer == 'n' or answer == 'N' or answer == 'no' or answer == 'No' or answer == 'nah' or answer == 'Nah':
                print('Thanks for playing! Final score: You - ',score[0],' Computer - ',  score[1])
                flag = 1
                break
            else:
                print('Yay or nay...')
                continue
        if flag == 0:
            continue
        else:
            break

RPS()

What things in my code are, for example, horribly inefficient or are bad practices?

\$\endgroup\$
1
  • \$\begingroup\$ I am sure that Scisscors was meant to be Scissors \$\endgroup\$
    – user92053
    Commented Dec 11, 2015 at 18:12

5 Answers 5

11
\$\begingroup\$

The game "Rock Paper Scissors" can be specified in terms of states.

Specification

The game is played by two players, playerA and playerB. Each player selects from among a set of three options {null, rock, paper, scissors}. null is used to represent the state before a player has chosen. Using an ordered pair (playerA_choice, playerB_choice) creates the possible game states:

 (null,    rock)
 (null,    paper)
 (null,    scissors)
 (rock,    null)
 (rock,    rock)
 (rock,    paper)
 (rock,    scissors)
 (paper,   null)
 (paper,   rock)
 (paper,   paper)
 (paper,   scissors)
 (scissors, null)
 (scissors, rock)
 (scissors, paper)
 (scissors, scissors)

There are three final states and three transition functions to them:

playerA_wins = (rock, scissors) | (paper, rock) | (scissors, paper)
playerB_wins = (rock, paper) | (paper, scissors) | (scissors, rock)
draw = (rock, rock) | (paper, paper) | (scissors, scissors)

The start state is:

 (null, null)

The states:

 (null,    rock)
 (null,    paper)
 (null,    scissors)
 (rock,    null)
 (paper,   null)
 (scissors, null)

are blocking and require further input from one of the players.

Implementing The Specification

This is a sketch of the game:

# Some Useful Names
null = "null"
rock = "rock"
paper = "paper"
scissors = "scissors"

# A Thesaurus (implemented as a dictionary)
synonyms = {"rock": rock,
            "paper": paper,
            "scissors": scissors,
            "stone": rock,
            "vellum": paper,
            "shears": scissors}

# Final States
game_is_draw = "Game is a Draw"

playerA_wins = "Player A Wins"

playerB_wins =  "Player B Wins"

# Initial State
both_players_must_choose = "Both Players Must Choose"

# Transition States
playerA_must_choose = "Player A Must Choose"

playerB_must_choose = "Player B Must Choose"

# Transition Table (implemented as a dictionary)
transitions = {(null,    null): both_players_must_choose,
               (null,    rock): playerA_must_choose,
               (null,    paper): playerA_must_choose,
               (null,    scissors): playerA_must_choose,
               (rock,    null): playerB_must_choose,
               (rock,    rock): game_is_draw,
               (rock,    paper): playerB_wins,
               (rock,    scissors): playerA_wins,
               (paper,   null): playerB_must_choose,
               (paper,   rock): playerA_wins,
               (paper,   paper): game_is_draw,
               (paper,   scissors): playerB_wins,
               (scissors, null): playerB_must_choose,
               (scissors, rock): playerB_wins,
               (scissors, paper): playerA_wins,
               (scissors, scissors): game_is_draw}



# Simulate Initialization
playerA_choice = null
playerB_choice = null

# Simulate Players Choosing
playerA_choice = synonyms["stone"]
playerB_choice = synonyms["shears"]

# Main Logic
state = (playerA_choice, playerB_choice)
print(outcomes[state])

Details to handle input output should be at a higher layer of abstraction. It shouldn't matter to the game engine if the game is between a human and a computer, two humans, or two computers. It shouldn't matter if it is being played using a laptop or over the internet.

Data Structures

A good rule of thumb is to replace complex logic with a data structure, and

if choice == 'r' or choice == 'R' or choice == 'Rock' or choice == 'rock' or choice == '1':

is the sort of code that is hard to understand and hard to maintain. Perhaps to the point where it is better to forgo the user friendly approach? Nay! A thesaurus is a good place to look for synonyms. Although, Python lacks Thesauri, a dictionary will probably do. Now the game can be sold at Ye Local Renaissance Faire as Stone, Vellum, Shears!

Indeed, a dictionary is a good way to map each possible game state to the next state. The code does in lieu of directly implementing the logic the specification uses to describe the final states.

The reason is maintainability. The mobile version of the game will offer an upgrade to Rock, Paper, Scissors, Spock, Lizard currently in development. Once we get around to updating the dictionaries synonyms and transitions the game will be done, profits will role in, and we will never have to work again unless sleeping on a stack of money counts as work.

But Really, Why All the Ceremony?

One of the real values that comes from using dictionaries is that a dictionary can be used to dispatch functions.

# Abbreviated Rock Paper Scissors

# Some Useful Names
rock = "rock"
paper = "paper"

def playerA_wins():
    print("Player A Wins")

def playerB_wins():
    print("Player B Wins")

transitions = {(paper, rock): playerA_wins,
               (rock, paper): playerB_wins}

transitions[(paper, rock)]()
transitions[(rock, paper)]()

Getting user input can be dispatched similarly by the dictionary. However, the initial (null, null) state, threading might be justified so that playerA_choice() doesn't block playerB_choice() or vice versa...that's IO for 'ya.

Recommendations

  1. Better variable names. Write about rocks and paper and scissors: rock = 1 makes the code more readable. Readability particularly helps the person writing the code.
  2. Make the code more modular. Put all the initialization together. Put all the user interface someplace else. Separate out the string matching. Only put high level abstractions in the main loop.
  3. Consider specifying the problem before writing code for a solution. At the core Rock, Paper, Scissors does not deal with synonyms. Writing a specification makes it clear that synonyms are a feature and encourages keeping their logic outside the main loop. This makes the code more readable.
\$\endgroup\$
1
  • 1
    \$\begingroup\$ I think this is by far the best answer! I want to emphasize the use of synonyms to deal with the differing ways to give inputs, and to a lesser the extent the idea of replacing the "who wins" logic with a lookup. \$\endgroup\$
    – user14393
    Commented Apr 25, 2015 at 22:56
7
\$\begingroup\$

I would change the choice input case statement to this: choice = input('->').upper(). You can then compare to 'ROCK' and 'R', and any combination of upper/lower case characters will match it. I may also create a dictionary to store the possible answers in, as keys, and use the choice_identifier as the values: dict = {'R':1, 'ROCK':1, 'S':2, 'SCISSORS':2, 'P':3, 'PAPER':3}, then you can set choice_identifier = dict.get(input('->').upper()). I'm not sure if this would be more efficient but I think it would be a little nicer to read.

\$\endgroup\$
2
  • \$\begingroup\$ That's one of the parts I thought must have an easier way, great help thanks. \$\endgroup\$
    – Adrian09
    Commented Feb 9, 2015 at 18:01
  • \$\begingroup\$ Note that get() will return None for an unexpected value. You would still need to check for that before comparing the result to the random value. \$\endgroup\$ Commented Feb 9, 2015 at 18:08
7
\$\begingroup\$

There are a couple things you could improve.

First, you have this:

while True:
    print("Enter your choice:")
    while True:
        choice = input('->')
        if choice == 'r' or choice == 'R' or choice == 'Rock' or choice == 'rock' or choice == '1':
            choice_identifier  = 1
            break
        elif choice == 'S' or choice == 's' or choice == 'Scissors' or choice == 'sciccors' or choice == '2':
            choice_identifier =  2
            break
        elif choice == 'P' or choice == 'p' or choice == 'Paper' or choice == 'paper' or choice == '3':
            choice_identifier =  3
            break
        else:
            print('That\'s not an option in this game :)')
            print('Try again:')
            continue

I would change that into something like this:

choice = " "
while choice.upper() is not "R" and choice.upper() is not "P" and choice.upper() is not "S":
    choice = input("Enter your choice - R, P, or S: ")

if choice.upper() is "R":
    choice_identifier = 1
elif choice.upper() is "P":
    choice_identifier = 2
elif choice.upper() is "S":
    choice_identifier = 3

I changed S to 3 as the game is called "Rock, Paper, Scissors."

Second, you should also change this:

RPS()

It is good that you have that in a separate function, but you should put the code that is always executed in this statement:

if __name__ == "__main__":

This needs to be done when you are using multiple files, so you might as well always do it.

Third, why not create a separate method for determining whether to play the game again? Something like this:

def PlayAgain(score):
    while True:
        answer = input('Play another round?')
        if answer == 'y' or answer == 'Y' or answer == 'yes' or answer == 'Yes' or answer == 'ye' or answer == 'Ye' or answer == 'sure' or answer == 'Sure':
            print(' Current score: You - ',score[0],' Computer - ',  score[1])
            return True
        elif answer == 'n' or answer == 'N' or answer == 'no' or answer == 'No' or answer == 'nah' or answer == 'Nah':
            print('Thanks for playing! Final score: You - ',score[0],' Computer - ',  score[1])
            return False
        else:
            print('Yay or nay...')

Then, you could call this method from any of your future games, instead of writing it many times.

Fourth, you should also separate the win/lose method like this.

Finally, I will say that I do not have Python installed in my computer at the moment, so this code may not work as is. It should be close, though.

\$\endgroup\$
7
\$\begingroup\$

Output

You seem to overworking yourself in some areas. Especially in the parts where you take input. For example, when the user is entering rock, paper, or scissors, you make a check on all the most common things that they might type.

To ease this, when you are prompting the user, you can show them their options.

For example, instead of:

print("Enter your choice:")

You could write:

print("Enter your choice ('r', 'p', 's'):")

Then, when you come to the conditionals, you only have to check if the user entered a lowercase 'r', 'p', or 's'.

Also, I noticed that you tend to split up your messages a bit.

By that, I mean that you split up your prints and your inputs. Since the input function takes an argument that it logs to the console, why don't you just put whatever you would've initially printed as that argument?

For example, these lines:

print("Enter your choice:")
while True:
    input("->")

Can become:

while True:
    input("Enter your choice:")

Functions

At the very last line of your code, you call a function. Since there are no conditionals surround that call, why do you have it out there? There is really a point in putting all of your code into a function if you are just going to call the function on the next line.

And, since this function is the only part of the code (at least that's my impression of it), you can just remove it and when your code runs, it will run as if it will run the same way.

\$\endgroup\$
1
  • \$\begingroup\$ I originally made it as a function because I plan to add more mini games which can be called independently in the same project. The merging of print and input, although less decorative, I see will probably be more efficient so thanks :) \$\endgroup\$
    – Adrian09
    Commented Feb 9, 2015 at 18:05
5
\$\begingroup\$

A lot of good observations have already been written by others but here are a couple of suggestions for general code readability.

Comparison

In general, should you have a need to compare a value with a list of others like so:

if choice == 'r' or choice == 'R' or choice == 'Rock' ...

you could instead use this shorter form:

if choice in ('r', 'R', 'rock', 'Rock', '1'):
    ...

and preferably define the tuple of choices somewhere to give it a meaningful name.

Incrementing and decrementing

Python doesn't have a unary increment or decrement operator in the style of ++i but you can write the following:

score[0] = score[0] + 1

in a shorter, and IMHO more readable, way like so:

score[0] += 1
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.