Rock, paper, scissors

Question

This is something I wrote as a reply to a deleted "Rock/paper/scissors [updated]" question. Since I can't post it as an answer (and the line-by-line analysis is gone as well), it might be nice to get a code review.

"""A game of rock/paper/scissors."""

import random

SIGNS = ("rock", "paper", "scissors")
VERBS = ("crushes", "covers", "cut")
YES = ("yes", "y")


class Counter(object):
    """Keep track of game session scores."""

    def __init__(self):
        self.rounds = 0
        self.losses = 0
        self.wins = 0
        self.draws = 0
        self.rounds_total = 0
        self.losses_total = 0
        self.wins_total = 0
        self.draws_total = 0

    def increment(self, state):
        """Increment losses, wins, draws.

        Args:
            state: "losses", "wins", or "draws".

        Raises:
            ValueError: If input is invalid.
        """
        if state not in ["losses", "wins", "draws"]:
            raise ValueError("Acceptable input: losses, wins, or draws.")
        for attr in (state, "%s_total" % state, "rounds", "rounds_total"):
            setattr(self, attr, getattr(self, attr) + 1)

    def flush(self):
        """Restart the current game counters, keep the total counters."""
        self.rounds = 0
        self.losses = 0
        self.wins = 0
        self.draws = 0

    def print_results(self):
        print "Games played: ", self.rounds
        print "Games won: ", self.wins
        print "Games lost: ", self.losses
        print "Games drawn: ", self.draws
        print "Total played: ", self.rounds_total
        print "Total won: ", self.wins_total
        print "Total lost: ", self.losses_total
        print "Total drawn: ", self.draws_total


class Sign(object):
    """A rock/paper/scissors sign."""

    def __init__(self, sign):
        global SIGNS

        if sign not in SIGNS:
            raise ValueError("Allowed signs are: %s." % SIGNS.join(", "))
        self.sign = sign

    def __eq__(self, other):
        try:
            if self.sign == other.sign:
                return True
        except AttributeError:
            pass
        return False

    def __gt__(self, other):
        if (self.sign == "rock" and other.sign == "scissors" or
            self.sign == "scissors" and other.sign == "paper" or
            self.sign == "paper" and other.sign == "rock"):
            return True
        return False

    def __lt__(self, other):
        return not self.__gt__(other)


def main(max_rounds=5):
    global SIGNS
    global VERBS
    global YES

    counter = Counter()
    while True:
        if counter.rounds == max_rounds:
            counter.print_results()
            again = raw_input("Play/again? (Y/n) ").lower()
            if again in YES:
                counter.flush()
            else:
                print "Bye!"
                break
        while True:
            player_sign = raw_input("Rock/paper/scissors? ").lower()
            if player_sign not in SIGNS:
                print "Only following words are valid: %s." % SIGNS.join(" ,")
                continue
            break
        player = Sign(player_sign)
        computer = Sign(random.choice(SIGNS))
        if player > computer:
            counter.increment("wins")
            verb = VERBS[SIGNS.index(player.sign)]
            print "Player wins: %s %s %s." % (player.sign, verb, computer.sign)
        elif player == computer:
            counter.increment("draws")
            print "It's a draw! Both played %s." % player.sign
        else:
            counter.increment("losses")
            verb = VERBS[SIGNS.index(computer.sign)]
            print "Player lost: %s %s %s." % (computer.sign, verb, player.sign)


if __name__ == "__main__":
    main(max_rounds=5)

Answer 1

So this all looks pretty good, with not much to review. I didn't see the previous question, so I will throw my two cents in here.

You define the following at the top of your file:

SIGNS = ("rock", "paper", "scissors")
VERBS = ("crushes", "covers", "cut")
YES = ("yes", "y")

which is fine. However in all the classes where you use these variable you denote them with the global keyword. This isn't code breaking, but it is unnecessary. The global key word is used for global variables. What you are using is global constants that are defined at module scope, so you can get rid of those redundant lines.

It was suggested in the comments that you replace the print_results function by __str__, but I would actually suggest replacing it by __repr__. In cases where __str__ is not defined __repr__==__str__ anyways. So:

def __repr__(self):
    return "\n".join(
        ["Games played: %d"%self.rounds,
         "Games won: %d"%self.wins,
         "Games lost: %d"%self.losses,
         "Games drawn: %d"%self.draws,
         "Total played: %d"%self.rounds_total,
         "Total won: %d"%self.wins_total,
         "Total lost: %d"%self.losses_total,
         "Total drawn: %d"%self.draws_total])

This will allow you to print the object nicely with print counter.

These are all ultra minor things now:

The block

def __eq__(self, other):
    try:
        if self.sign == other.sign:
            return True
    except AttributeError:
        pass
    return False

has a redundant pass and redundant branch. This could be condensed to:

def __eq__(self, other):
    try:
        return (self.sign == other.sign)
    except AttributeError:
        return False

In a similar vain, there is no need to branch in __gt__ method:

def __gt__(self, other):
    return (self.sign == "rock" and other.sign == "scissors" or
            self.sign == "scissors" and other.sign == "paper" or
            self.sign == "paper" and other.sign == "rock")

and the __lt__ method:

def __lt__(self, other):
    return not self.__gt__(other)

is both redundant and incorrect (as pointed out in the comments, this is actually a less than or equal to that is written here, as any value that is not greater than will return true) and can be removed (the default version calls __gt__ and should handle your problem fine).

Finally for clarity I would define VERBS as a dictionary. It is more code, but it is also more readable:

VERBS = {"rock":"crushes", "paper":"covers", "scissors":"cut"}

this allows you to do the following in the main function:

verb = VERBS[player.sign]

and

verb = VERBS[computer.sign]

Answer 2

• Since we apparently speak the same language, I understand why you picked sign, but for most of the audience symbol would me more meaningful.

Consider the following a design (rather than code) review.

• The game has a nice mathematical property which begs to be exploited. Notice that if we assign numerical values to the objects, namely Rock = 0, Paper = 1, Scissors = 2, then

  (val1 - val2) % 3
  

defines the outcome of the round (0 is draw, 1 means first player wins, and 2 means second player wins). This totally eliminates a need for a Sign class.

• It would be nice to separate a generic game playing functionality from the specifics of the particular game. It would also be nice to abstract players (at least to test separate strategies).

That said, here is the core of the game (sorry, no play again feature):

# Game specific code
import random

value = { 'r': 0, 'p': 1, 's': 2 }
text = [ 'R', 'P', 'S' ]

def human_play():
    while True:
        move = raw_input('R/P/S?').lower()
        try:
            return value[move]
        except KeyError:
            print 'invalud input'

def comp_play():
    return random.randrange(0, 3)

def round(p1, p2):
    v1 = p1()
    v2 = p2()
    return v1, v2, (v1 - v2) % 3

# Generic code
outcome = [ 'drawn', 'won', 'lost' ]

class Counter:
    def __init__(self):
        self.played = 0
        self.results = [0] * 3
    def add(self, result):
        self.played += 1
        self.results[result] += 1
    def show(self):
        print 'played: %d' % self.played
        print '; '.join(['%s: %d' % (o, r) for (o, r) in zip(outcome, self.results)])

def game(p1, p2, max_rounds = 5):
    counter = Counter()
    for _ in range(max_rounds):
        v1, v2, result = round(p1, p2)
        print '%s .. %s: %s' % (text[v1], text[v2], outcome[result])
        counter.add(result)
    return counter

def main():
    results = game(human_play, comp_play)
    results.show()

if __name__ == '__main__':
    main()