Scissors, Paper, Rock in Python

Question

The game works, but I was wondering if anyone could help me with better structure for the code, simplification, or just any other improvements.

from random import randrange

# assigns the amount of turns and starts the program
def Main():  
    print "~A game of Scissors, Paper, Rock!~"
    global turns
    turns = raw_input("How many times do you want to play? : ")
    # tests for int input and restarts if not an int
    try:
        turns = int(turns)
        print "\nYou will play", turns, "turn(s) against the computer! \n"
        play()
    except ValueError:
        print "That's not a valid input! Integers only, please! \n"
        Main()

#determines the player's choice and sets the computers choice
def play():
    global options
    options = {1: "Scissors", 2: "Paper", 3: "Rock"}
    global turns
# loops for different choices, and calls the comparison function
    while turns > 0:
        try:
            global pchoice
            pchoice = int(raw_input("Choose one! \n <1> Scissors \n <2> Paper \n <3> Rock \n"))
            global cchoice
            cchoice = randrange(1,4)
            comparison()
        except KeyError:
            print "an invalid input! Please try again. \n"
            play()
        turns -= 1

# compares player's choice with the computer's and returns a tie, win, or loss
def comparison():
    print "You chose:", options[pchoice], "and the computer chose:", options[cchoice]
    if int(pchoice) == cchoice:
        print "A tie! \n"
    elif pchoice == 1 and cchoice == 3 or pchoice == 2 and cchoice == 1 or pchoice == 3 and cchoice == 2:
        print "You lose! \n"
    else:
        print "You win! \n"

Main()

Answer 1

I'd like to focus on two key issues:

Global variables

You should strive to have no global variables in your program, and it is entirely possible to eliminate them from this program.

If your functions only ever use local variables, then you can understand your code by analyzing each function separately. However, when you use global variables, you will need to analyze the entire program at once, because the values could be affected by code anywhere in the program.

For example, there is no reason why turns needs to be global. By making it global, you cannot be sure, just by looking at the play() function, that the loop will execute turns number of times, because you have to worry about the possibility that comparison() modifies its value. Rather, turns should be local to play(). (Actually, it should be a parameter.)

def play(turns):
    # loops for different choices, and calls the comparison function
    while turns > 0:
        try:
            pchoice = int(raw_input("Choose one! \n <1> Scissors \n <2> Paper \n <3> Rock \n"))
            cchoice = randrange(1,4)
            comparison(pchoice, cchoice)
        except KeyError:
            print "an invalid input! Please try again. \n"
            play(turns)     # Problematic.  I'll say more about this later…
            return
        turns -= 1

Use of functions as if they were GOTO labels

Suppose that at the "How many times do you want to play?" prompt, you hit Enter many times, then hit ControlC. You would get a stack trace like this:

How many times do you want to play? : ^CTraceback (most recent call last):
  File "cr64216.py", line 45, in <module>
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 15, in Main
    Main()
  File "cr64216.py", line 7, in Main
    turns = raw_input("How many times do you want to play? : ")
KeyboardInterrupt

That deep stack trace is a sign of improper recursion. A properly constructed program should only have one frame in the stack at that point, namely Main().

Rather than recursing, you should implement the retry mechanism using a loop.

def Main():  
    print "~A game of Scissors, Paper, Rock!~"

    # Ask until a valid number of turns is given by the user
    while True:
        turns = raw_input("How many times do you want to play? : ")
        # tests for int input and restarts if not an int
        try:
            turns = int(turns)
            print "\nYou will play", turns, "turn(s) against the computer! \n"
            break
        except ValueError:
            print "That's not a valid input! Integers only, please! \n"

    play(turns)

Answer 2

Right now you have input or output in every function. Try to write functions that perform one specific function (pun), and consider that input/output is an other function than processing some logic (such as compare()) or performing some sort of procedure (such as play()).

Before writing any application, try to structure you application/classes/functions with the "loosly coupling, thightly cohesive"-principle. Start big, end small.

In your case, separate functions might be appropiate for:

• Getting the number of rounds
• Play one turn (I'd include user's input/output per turn in this function)
• Compare outcome (without user output)

Finally, as an exercise, you might want to try to add pre- and post conditions in the comments of your functions. That forces you to think about what a function is really for, as seen from a black box perspective.

Answer 3

In addition to the already excellent answers to this question, there are a few other issues that I'd like to pick out.

1. Turn management and IO should be left to the main loop, with functions splitting up work into small sections.
2. Use global constants to replace global variables
3. Find a more idiomatic way to find out if the player has won, lost or tied instead of a long if clause

I looked at your code closely and made a couple of major changes to it. You can take a look below:

from random import choice

OPTIONS = {0: "Scissors", 1: "Paper", 2: "Rock"}

# list of configurations for player A to win
# (choice of player A, choice of player B)
WIN = [(0, 1), (1, 2), (2, 0)]    

def main():
    def play():
        print "Choose one!"
        print "<0> Scissors"
        print "<1> Paper"
        print "<2> Rock"
        while True:
            try:
                pchoice = int(raw_input())
                if pchoice not in OPTIONS:
                    raise ValueError
                cchoice = choice(OPTIONS.keys())
                print "You chose: {} and the computer chose: {}".format(
                    OPTIONS[pchoice],
                    OPTIONS[cchoice]
                )

                outcome = compare_move(pchoice, cchoice)
                if outcome == 0:
                    print "A tie!\n"
                elif outcome > 0:
                    print "You win!\n"
                elif outcome < 0:
                    print "You lose!\n"

                break
            except ValueError:
                print "Invalid input! Please try again."

    print "~A game of Scissors, Paper, Rock!~"
    while True:
        try:
            turns = int(raw_input("How many times do you want to play? : "))
            if turns <= 0:
                raise ValueError

            print "\nYou will play", turns, "turn(s) against the computer!\n"
            for _ in xrange(turns):
                play()

            break
        except ValueError:
            print "That's not a valid input! Positive integers only, please!\n"

def compare_move(pchoice, cchoice):
    """
    Takes in the player's choice and compares against the computer's choice.

    Returns 0 if it is a tie.
    Returns a positive integer if the player has won.
    Returns a negative integer if the player has lost.
    """
    if pchoice == cchoice:
        return 0
    elif (pchoice, cchoice) in WIN:
        return 1
    elif (cchoice, pchoice) in WIN:
        return -1

main()

I chose to handle all IO in the main() loop (splitting up the work with the use of functions in the main() function) and leave all outside work to external functions.

The only external function in my code, compare_move(), uses a more idiomatic way to find out if the player has won, tied or lost.

Answer 4

I hope following example bring you some new idea.

#!/usr/bin/env python
# ~*~ coding: utf-8 ~*~
from random import randrange


class Choice():
    CHOICES = [1,2,3]

    @classmethod
    def get_random_choice(cls):
        return CHOICES[randrange(0, len(CHOICES))]


class Person():
    """ Contains meta info about players """
    def __init__(self, name):
        self.name = name


class LocalPersonController(Person):
    """ Implements comunication with player on local machine """
    INPUT_PROMPT = '''Choose one!
    <1> Scissors
    <2> Paper
    <3> Rock
    {0} choose: '''

    def get_choice(self):
        return int(raw_input(LocalPersonController.
                   INPUT_PROMPT.format(self.name)))


class NetworkPersonController(Person):
    """ Implements comunication with player on remote machine """
    def __init__(self, name, uri):
        super(NetworkPersonController, self).__init__(name)
        # TODO: init connection

    def get_choice(self):
        # TODO: replace stub
        return Choice.get_random_choice()


class AIController(Person):
    def __init__(self):
        super(AIController, self).__init__("AI")

    def get_choice(self):
        return Choice.get_random_choice()


class Round():
    def __init__(self, player1Ctrl, player2Ctrl):
        self.player1 = player1Ctrl
        self.player2 = player2Ctrl

    def run(self):
        self.turn1 = self.player1.get_choice()
        self.turn2 = self.player2.get_choice()


class Game():
    PlAY_PROMPT = '''Wish you play a round? Y/n '''
    ROUND_PROMPT = '''Select opponent:
    <1> local player
    <2> remote player
    <3> AI
    '''

    def start(self):
        print("Hello!")
        while self.play():
            round = self.select_round()
            round.run()
            self.show_victory(round)
        print("By!")

    def play(self):
        choise = raw_input(Game.PlAY_PROMPT)
        while choise.lower() not in ['n', 'y', '']:
            choise = raw_input(Game.PlAY_PROMPT)

        return not choise.lower() == 'n'

    def select_round(self):
        choise = raw_input(Game.ROUND_PROMPT)
        while choise.lower() not in ['1', '2', '3']:
            choise = raw_input(Game.ROUND_PROMPT)

        opponent = object
        if choise.lower() == '1':
            opponent = LocalPersonController('player2')
        elif choise.lower() == '2':
            opponent = NetworkPersonController('player2', 'some uri data')
        else:
            opponent = AIController()

        return Round(LocalPersonController('player1'), opponent)

    def show_victory(self, round):
        # TOD: show which player win
        pass


if __name__ == '__main__':
    Game().start()

Player input

You can add few ways of player input. This approach gives you way to implement AI. Look at classes: Person and inherited from it. All of them had common input interface, so nothing can stop you to add alien's behavior (:

Game mode

As for me, game mode is a part of Choice class, which implement different choices and common interface to find winner, get list choices and so on.

Game round

It is a single game round, perhaps it should be hard linked with game mode in case of behavior. Any way it should perform one game iteration. In my example I find winner outside of the Round class, but it is a part of single loop iteration so it should be there.

Game loop

You can add some preferences and don't ask for selecting opponent. Also add some defaults or even writing preferences into local settings.conf.