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.)

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])