4
\$\begingroup\$

I've started learning Python recently and I wanted to test myself. So, can you tell me how many I would get out of 10 and how I could improve my future coding?

import random
import time
print('Welcome to Rock, Paper, Scissors')
print(' '*25) 
wins = 0
loses = 0
draws = 0
point= int(input('How many rounds do you want to play?'))
list = ['r','p','s']
for x in range (1,(point+1)):
    computer = random.choice(list)
    human=str(input('Choose (R)ock, (P)aper, or (S)cissors?'))
    if human == computer:
        draws = draws + 1
        print('''Human: {}         Computer: {}           A draw
        Wins = {}   Loses = {}   Draws = {}'''.format(human,computer,wins,loses,draws))
        print(' '*25)
    elif human == 'r' and computer == 'p':
        loses = loses + 1
        print('''Human: ROCK       Computer: PAPER        Computer wins
        Wins = {}   Loses = {}   Draws = {}'''.format(wins,loses,draws))
        print(' '*25)
    elif human == 'r' and computer == 's':
        wins = wins + 1
        print('''Human: ROCK       Computer: SCISSORS     Human wins
        Wins = {}   Loses = {}   Draws = {}'''.format(wins,loses,draws))
        print(' '*25)
    elif human == 'p' and computer == 's':
        loses = loses + 1
        print('''Human: PAPER      Computer: SCISSORS     Computer wins 
        Wins = {}   Loses = {}   Draws = {}'''.format(wins,loses,draws))
        print(' '*25)
    elif human == 'p' and computer == 'r':
        wins = wins + 1
        print('''Human: PAPER      Computer: ROCK        Human wins
        Wins = {}   Loses = {}   Draws = {}'''.format(wins,loses,draws))
        print(' '*25)
    elif human == 's' and computer == 'p':
        wins = wins + 1
        print('''Human: SCISSORS   Computer: PAPER    Human wins
        Wins = {}   Loses = {}   Draws = {}'''.format(wins,loses,draws))
        print(' '*25)
    elif human == 's' and computer == 'r':
        loses = loses + 1
        print('''Human: SCISSORS   Computer: ROCK     Computer wins
        Wins = {}   Loses = {}   Draws = {}'''.format(wins,loses,draws))
        print(' '*25)
    else:
        print('Error')
\$\endgroup\$
1
  • 5
    \$\begingroup\$ The purpose of code review is to improve the quality of the code, not to evaluate the coder, so I would hope that no-one actually starts giving marks out of ten. \$\endgroup\$ Dec 1 '13 at 15:51
4
\$\begingroup\$

Your program is adequate, but it doesn't "scale". Each player can choose from 3 moves; of those 9 possibilities, the 3 ties can be handled in common, for a total of 7 code branches. Handling all those cases with cut-and-paste code violates the "Don't Repeat Yourself" principle, and it makes the code hard to maintain.

I would start by introducing a class to represent the moves, and define how they relate to each other:

from collections import namedtuple

class RPSMove(namedtuple('RPSMove', ['name', 'short_name', 'beats'])):
    def __gt__(self, other):
        return self.beats == other.short_name

    def __lt__(self, other):
        return not(self == other or self > other)

    def __str__(self):
        return self.name

    all = dict()

RPSMove.all['r'] = RPSMove('rock',     short_name='r', beats='s')
RPSMove.all['p'] = RPSMove('paper',    short_name='p', beats='r')
RPSMove.all['s'] = RPSMove('scissors', short_name='s', beats='p')

Next, I would extract the way the computer and human take their turns into functions:

import random
from sys import exit

def computer_play():
    return random.choice(list(RPSMove.all.values()))

def human_play():
    while True:
        try:
            choice = input('Choose (R)ock, (P)aper, or (S)cissors? ')
            return RPSMove.all[choice[0].lower()]
        except EOFError:
            print('')
            exit(0)
        except:
            print('Error')

The human_play() function is more complex than your original due to handling of invalid choices and end-of-file (Control+D in Unix or Control+Z in Windows).

With those preliminaries out of the way, the heart of the game can look quite nice.

random.seed()
human_wins, computer_wins, draws = 0, 0, 0

print('Welcome to Rock, Paper, Scissors')
print('')
rounds = int(input('How many rounds do you want to play? '))
for _ in range(rounds):
    human_move, computer_move = human_play(), computer_play()
    if human_move > computer_move:
        result = 'Human wins'
        human_wins += 1
    elif human_move < computer_move:
        result = 'Computer wins'
        computer_wins += 1
    else:
        result = 'A draw'
        draws += 1

    print('Human: %-17s Computer: %-17s %s' %
          (human_move, computer_move, result))
    print('Human wins = %-11d Computer wins = %-11d Draws = %-11d' %
          (human_wins, computer_wins, draws))
    print('')

I've renamed wins and loses to human_wins and computer_wins to avoid anthropocentrism.

\$\endgroup\$
3
\$\begingroup\$
  • There is no overall structure to this program, just an undifferentiated mass of code.

  • There is a lot of redundancy to the 'if... elif... elif...' structure which could be pulled out into a function; better yet, find a way to structure the decision-making regularities as a calculation.

  • print(' '*25) is very strange; print() would suffice.

  • list is not a good variable name because it shadows the built-in function list().

Here is a rewritten version:

from random import choice
import sys

# compatibility shim for Python 2/3
if sys.hexversion >= 0x3000000:
    inp = input
    rng = range
else:
    inp = raw_input
    rng = xrange

def get_int(prompt):
    while True:
        try:
            return int(inp(prompt))
        except ValueError:
            pass

def get_ch(prompt):
    while True:
        res = inp(prompt).strip()
        if res:
            return res[:1]

class RockPaperScissors():
    moves = 'rps'
    names = {
        'r': 'ROCK',
        'p': 'PAPER',
        's': 'SCISSORS'
    }
    win_string = {
        0:  'Draw',
        1:  'Human wins',
        2: 'Computer wins'
    }

    def __init__(self):
        self.wins   = 0
        self.draws  = 0
        self.losses = 0

    def play(self):
        cmove = choice(RockPaperScissors.moves)
        hmove = get_ch('Choose [r]ock, [p]aper, [s]cissors: ').lower()
        win_state = (RockPaperScissors.moves.index(hmove) - RockPaperScissors.moves.index(cmove)) % 3
        [self.draw, self.win, self.lose][win_state]()
        print('Human plays {:<12s}  Computer plays {:<12s}  {}'.format(RockPaperScissors.names[hmove], RockPaperScissors.names[cmove], RockPaperScissors.win_string[win_state]))

    def win(self):
        self.wins += 1

    def draw(self):
        self.draws += 1

    def lose(self):
        self.losses += 1

def main():
    print('Welcome to Rock, Paper, Scissors')

    game = RockPaperScissors()
    rounds = get_int('How many rounds do you want to play? ')
    for round in rng(rounds):
        game.play()
        print('{} wins  {} draws  {} losses\n'.format(game.wins, game.draws, game.losses))

if __name__=="__main__":
    main()
\$\endgroup\$
5
  • \$\begingroup\$ This is in some respects worse than the original code, particularly the lack of transparency in how the winner is decided. \$\endgroup\$
    – Stuart
    Nov 30 '13 at 1:03
  • \$\begingroup\$ @Stuart: um... sorry? I thought it was pretty dead-obvious; you index into the 'rock-paper-scissors' cycle, subtract the human's move from the computer's, and modulo 3. The answer is then 0 = draw, 1 = human wins, 2 = human loses. You then use that answer to call the appropriate method to update the score. \$\endgroup\$ Nov 30 '13 at 1:09
  • \$\begingroup\$ You have lost readability by using modulos, a mysterious variable called "res", and a look up in tables. Why not use if...elif...else? Also, your class structure is overly complicated for the nature of the programme. This would make sense only if the plan is to extend it. \$\endgroup\$
    – Stuart
    Nov 30 '13 at 1:30
  • \$\begingroup\$ @Stuart: yes, res is not a great name; I should probably have called it something like win_state. I could have then used 6 lines of if-else to call the appropriate methods - and if the operation were less cleanly structured I would have - but frankly I think this looks cleaner. The class properties are probably overkill - too used to Java-style setters - but I doubt it took you more than two seconds to figure out what they do; and inheritance is not the only reason to use classes. \$\endgroup\$ Nov 30 '13 at 1:48
  • \$\begingroup\$ Removed properties, renamed res. \$\endgroup\$ Nov 30 '13 at 2:02
3
\$\begingroup\$

You can reduce if...elif... by noting that there are only three possible outcomes and finding a simple way to describe the conditions for each outcome. You can store the outcome (win, lose, draw) in a variable and avoid repeating the same print command. You could also use a dictionary to store the three options and their abbreviations.

import random
print('Welcome to Rock, Paper, Scissors\n')
wins = 0
loses = 0
draws = 0
options = {'r': 'rock', 'p': 'paper', 's': 'scissors'}
rounds = int(input('How many rounds do you want to play?'))
for _ in range(rounds):
    computer = random.choice(list(options.keys()))
    human = str(input('Choose (R)ock, (P)aper, or (S)cissors?')).lower()
    if human == computer:
        outcome = 'A draw'
        draws += 1
    elif (human, computer) in (('r', 'p'), ('p', 's'), ('s', 'r')):
        outcome = 'Computer wins'
        loses += 1
    else:
        outcome = 'Human wins'
        wins += 1
    print("Human: {}\tComputer: {}\t{}".format(options[human], options[computer], outcome))
    print("Wins = {}\tLoses = {}\tDraws = {}".format(wins, loses, draws))

If you want to improve the programme further, consider adding error checking in case the user enters the wrong type of input, e.g.:

while True:
    try:
        rounds = int(input('How many rounds do you want to play?'))
        if rounds <= 0:
            raise ValueError
        break
    except ValueError:
        print ("Please enter a positive whole number.")
\$\endgroup\$
4
  • \$\begingroup\$ random.choice(options.keys()) is redundant; random.choice(options) will do. \$\endgroup\$ Nov 30 '13 at 2:09
  • \$\begingroup\$ @HughBothwell no, random.choice takes a sequence. I don't think it works with a dictionary. \$\endgroup\$
    – Stuart
    Nov 30 '13 at 2:21
  • \$\begingroup\$ my bad; I was thinking of how for k in mydict: iterates on the keys, but you are right, random.choice won't. Actually, in Python 3, it won't accept random.choice(mydict.keys()) either, it has to be random.choice(list(mydict)) or random.choice(list(mydict.keys())). \$\endgroup\$ Nov 30 '13 at 2:29
  • \$\begingroup\$ Thanks have edited - I think random.choice(list(mydict.keys())) will work \$\endgroup\$
    – Stuart
    Nov 30 '13 at 2:33
0
\$\begingroup\$

Rather than making the game a class, consider making the choices a class. This makes it easy to define the behaviors:

import sys
from random import choice

class RPS(object):
    def __init__(self, name, rank, *lose_to):
        self.Name = name
        self.Rank = rank
        self.LoseTo = lose_to

    def fight(self, other):
        if self.Rank in other.LoseTo: return 'wins'
        if other.Rank in self.LoseTo: return 'loses'
        return 'draws'

    def __str__(self):
        return self.Name

ROCK = RPS('rock', 0, 1)
PAPER = RPS('paper', 1, 2)
SCISSORS = RPS('scissors', 2, 0)
OPTIONS = {'r':ROCK, 'p':PAPER, 's':SCISSORS} 
PROMPT = "\nEnter [r]ock, [p]aper, [s]cissors or [q]uit :"
results = []

while True:
    choice_string = raw_input(PROMPT)
    try:
        player_choice = OPTIONS[choice_string]
        computer_choice = choice(OPTIONS.values())
        results.append(player_choice.fight(computer_choice))
        sys.stdout.writelines("\tplayer: %s\n\tcomputer: %s\n\tresult: player %s\n"  % (player_choice, computer_choice, results[-1]))

    except KeyError:  # something other than 'r', 'p' or 's' was entered...
        if choice_string == 'q':
            break  # exits the game
        else:
            sys.stdout.writelines("'%s' is not a valid choice" % choice_string)

# print an exit summary
sys.stdout.writelines("\nThanks for playing!\n")
sys.stdout.writelines("\t%i wins\n" % results.count('wins'))
sys.stdout.writelines("\t%i losses\n" % results.count('loses'))
sys.stdout.writelines("\t%i draws\n" % results.count('draws'))

The advantage of pushing the comparison down into the RPS class is that the behavior can be extended without going ELIF crazy. For example the above can be changed like so:

ROCK = RPS('rock', 0, 1, 4)
PAPER = RPS('paper', 1, 2, 3)
SCISSORS = RPS('scissors', 2, 0, 4)
LIZARD = RPS('lizard', 3, 0, 2)
SPOCK = RPS('spock', 4, 1, 3)
OPTIONS= {'r':ROCK, 'p':PAPER, 's':SCISSORS, 'l':LIZARD, 'k':SPOCK } 
PROMPT = "\nEnter [r]ock, [p]aper, [s]cissors, [l]izard,  Spoc[k] or [q]uit :"       

with minimal effort and no changes to the overalls structure of the program. In a real application you'd probably return something more definitive from the 'fight' method than a string - a result class with its own message, or something -- but by keeping the details of the comparison out of the main flow of the program it's easier to extend as needs change. Overkill for rock paper scissors, but a big plus for rock, paper, scissors, lizard, spock. Unfortunately real life is more like the latter than the former ;)

\$\endgroup\$
0

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.