OOP in Rock, Paper, Scissors program

Question

This is an OOP Rock, Paper, Scissors program I wrote in Python. I am just learning about using classes and I'd appreciate some advice on improving it.

# rock.py
# example of OOP
import random
class Rock:
 def main(self):
 self.Make_players()
 print("Best of Five - Let's play!\n")
 done = False
 while done == False:
 self.p1.go_player()
 self.p2.go_player()
 print()
 if self.p2.go == self.p1.go:
 print("No winner!\n")
 continue
 else:
 temp = self.check(self.p1, self.p2)
 if temp == False:
 temp = self.check(self.p2, self.p1)
 print(self.message, end = " ")
 print(temp.name + " won this round.")
 temp.scored()
 print(self.p1.name + ": " + str(self.p1.score))
 print(self.p2.name + ": " + str(self.p2.score))
 if self.p1.score == 3:
 self.winner = self.p1
 done = True
 elif self.p2.score == 3:
 self.winner = self.p2
 done = True
 else:
 done = False
 input()
 print("The winner was " + self.winner.name + "!")
def __init__(self):
 print("**** Welcome to Rock, Paper, Scissors!****\n")
 self.winner = False
 self.main()
def Make_players(self):
 temp = (input("What shall we call Player 1? "))
 self.p1 = Player(temp)
 temp = (input("What shall we call Player 2? "))
 self.p2 = Player(temp)
def check(self, p_a, p_b):
 if p_a.go == "rock" and p_b.go == "scissors":
 self.message = "Rock breaks scissors."
 return p_a
 elif p_a.go == "paper" and p_b.go == "rock":
 self.message = "Paper wraps stone."
 return p_a
 elif p_a.go == "scissors" and p_b.go == "paper":
 self.message = "Scissors cut paper."
 return p_a
 else:
 return False
class Player:
 def __init__(self, name):
 self.choices = ["rock", "paper", "scissors"]
 self.score = 0
 self.name = name
 print("Player:", self.name, "created!\n")
 def go_player(self):
 self.go = random.choice(self.choices)
 print(self.name + " chose " + self.go, end = ". ")
 return self.go
 def scored(self):
 self.score += 1
# Main
game = Rock()

Answer 1

The best OOP advice I can give for this code is: stop writing classes.

Ideally, watch the video first (it’s not that long) and come back later. But here’s the gist:

A class that has only one publicly called method (or less) isn’t a legitimate class. It should be a method.

Consequently, Rock shouldn’t be a class. It should be a method.

The rest is detail criticism. Most importantly, read PEP8 (the Python style guide) and adhere to it. Notably, this includes naming conventions, and using meaningful names.

Next, separate concerns clearly. go_player shouldn’t print text, its sole task should be to generate a random game choice. The printing should happen elsewhere.

Similarly (i.e. still a separation-of-concerns problem), neither all possible choices (i.e. choices) nor the current choice (i.e. go) logically belong in the Player. They are separate entities.

self.winner = False is logically wrong. The variable doesn’t contain a boolean value later on, why does it contain one now? Same goes for check – it should return None, not False.

Next, use a standard entry point. The following is pretty much an established pattern in Python that all code should follow:

def main():
 ...
if __name__ == '__main__':
 sys.exit(main())

This way, the code can be executed directly or reused as a module.

And a check like while done == False is like fingernails across a blackboard for professional programmers: explicit checks against boolean literals never (!) make sense. Use while not done instead.

In summary, here’s how I’d do it:

#!/usr/bin/env python
import sys
import random
# The extended, cool rules:
# http://en.wikipedia.org/wiki/Rock-paper-scissors-lizard-Spock
CHOICES = ['rock', 'paper', 'scissors', 'lizard', 'Spock']
RULES = {
 'rock': {
 'scissors': 'blunts',
 'lizard': 'crushes'},
 'paper': {
 'rock': 'covers',
 'Spock': 'disproves'},
 'scissors': {
 'paper': 'cuts',
 'lizard': 'decapitates'},
 'lizard': {
 'paper': 'eats',
 'Spock': 'poisons'},
 'Spock': {
 'rock': 'vaporizes',
 'scissors': 'smashes'}
}
class Player:
 def __init__(self, name):
 self.name = name
 self.score = 0
 @classmethod
 def create(cls, num):
 name = raw_input('What shall we call player {}? '.format(num))
 return cls(name)
 def go(self):
 return random.choice(CHOICES)
 def scored(self):
 self.score += 1
 def __str__(self):
 return self.name
def get_winner(players, choices):
 """
 Determine winner of a rock-paper-scissors round.
 Returns a tuple of (winner, winning move) if exists, else None.
 """
 def get_rule(cs):
 return RULES[cs[0]].get(cs[1], None)
 move = get_rule(choices)
 if move is not None:
 winner = players[0]
 else:
 choices = choices[::-1]
 move = get_rule(choices)
 winner = players[1]
 if move is None:
 return None
 return (winner, (choices[0], move, choices[1]))
def game(how_many_rounds):
 if how_many_rounds % 2 == 0:
 raise RuntimeError('Rounds must be an uneven number')
 players = [Player.create(1), Player.create(2)]
 winning_score = how_many_rounds // 2 + 1
 print('Best of {} - let\'s play!\n'.format(how_many_rounds))
 while True:
 choices = [p.go() for p in players]
 for p, c in zip(players, choices):
 print p, c
 win = get_winner(players, choices)
 if win is None:
 print('No winner.\n')
 continue
 winner, move = win
 winner.scored()
 print('{} {} {}. {} won this round.'.format(
 move[0].capitalize(), move[1], move[2], winner))
 for p in players:
 print('{}: {}'.format(p, p.score))
 if winner.score == winning_score:
 break
 raw_input()
 print('The winner was {}!'.format(winner))
def main():
 game(99)
if __name__ == '__main__':
 sys.exit(main())

To be honest, I’m not entirely satisfied with this since I prefer having immutable objects as much as possible, and the player objects here are not immutable. In fact, I’d probably get rid of the Players entirely and store the game state inside the game, where it belongs.

(Proper documentation is left as an exercise for the reader. I’ve only documented get_winner since it would be incomprehensible without documentation.)

• ecatmur: score isn't an attribute on Player; you should have a scoreboard := map(Player, int) or even := Counter(Player).

Answer 2

I would change check to this

def check(self, p_a, p_b):
 messages = {"rock":"Rock breaks scissors.",
 "paper":"Paper wraps stone.",
 "scissors":"Scissors cut paper.",
 }
 if p_a.go == p_b.go:
 return False
 elif p_a.go == "rock" and p_b.go == "scissors":
 self.message = messages[p_a.go]
 return p_a
 elif p_a.go == "paper" and p_b.go == "rock":
 self.message = messages[p_a.go]
 return p_a
 elif p_a.go == "scissors" and p_b.go == "paper":
 self.message = messages[p_a.go]
 return p_a
 else:
 # if moves are not same and player1 couldn't win than it should be
 # player2 who wins that round
 self.message = messages[p_b.go]
 return p_b

And the part where you check moves:

temp = self.check(self.p1, self.p2)
if not temp: 
 print("No winner!\n")
 continue

Edit after antiloquax's comment:

Even it will work that way I don't like it. you can use a class variable messages = {"rock":"...",} as I did in check method. While printing you just say

print(self.messages[temp.go])

• antiloquax: Thanks savruk. I was thinking about something like that - but the "else:" here would just return the player without printing the message "Paper wraps stone" etc. I think.
• savruk: antiloquax, you are right. I didn't think about it. See my edited answer. As a move can only win against a certain move(rock can only beat scissors, scissors can only beat paper ...), it is easy to use a dictionary to define a message for the move.