antiloquax
Posts: 406
Joined: Sun Nov 20, 2011 11:37 am
Contact: Website

Could someone who knows about OOP look at my code?

Sat Aug 11, 2012 12:56 pm

I've written a little OOP program that plays "Rock Paper Scissors". It works okay, but I am not very confident as to whether I have done it in a sensible / elegant (well, I can dream ...) way. If anyone could offer some tips, I'd be very grateful.

Code: Select all

# rock.py - an example of OOP
import random

class Rock:
    # constructor
    def __init__(self):
        print("**** Welcome to Rock, Paper, Scissors!****\n")
        
    # the main run of the game.
    def run(self):
        self.winner = False
        self.Make_players()
        self.play_round()
        print("The winner was", self.winner.name + ".\n")
        temp = input("Enter 'y' to play again: ")
        if temp == 'y':
            print()
            self.run()

    # plays a round (best of 5)
    def play_round(self):
        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()
        
    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()
game.run()
print("Bye.")

BlackJack
Posts: 288
Joined: Sat Aug 04, 2012 8:28 am
Contact: Website

Re: Could someone who knows about OOP look at my code?

Sat Aug 11, 2012 3:19 pm

@antiloquax: The `Rock` class seems to be a program with globals just moved into a class. `__init__()` methods should create a fully usable instance with a consistent and complete state. After `__init__()` all instance attributes should be set, i.e. there should be no introduction of attributes outside of `__init__()` and other initialisation methods called from there.

`__init__()` is not a constructor, that's `__new__()`.

A better value for „nothing” would be `None` instead of `False`. `False` implicates that at some point `True` would be a sensible value for the name.

`Rock.run()` is an endless recursion which is a bad idea in languages that do not promise tail call optimisation. Every call creates another frame on the call stack and CPython has a recursion limit:

Code: Select all

In [155]: def f():
   .....:     f()
   .....: 

In [156]: f()

[looooong taceback...]

RuntimeError: maximum recursion depth exceeded
Don't use „unlimited” recursion if not writing inherently recursive algorithms. This is just a simple loop.

Comparing with literal boolean values is bad style. The result of the comparison is a boolean itself — either always the same as the variable part or always the opposite, so you can always use the value of the variable or its negation instead of the opposite. ``var == True`` is the same as ``var`` in a boolean context and ``var == False`` the same as ``not var``. The `play_round()` loop: ``while not done:``.

I would remove `done` here and write an endless loop that is left by returning the winner. Which also eliminates the need for a winner attribute on `Rock`.

`Player.go_player()` sounds a bit redundant. I would call the method just `go()` or maybe `make_move()`. The attribute `go` would be more understandable if named `choice`. The return value of the method is not used anywhere.

The `check()` method does not everything I would expect from such a method. Half of the job of checking who has won that move is done in `play_round()`. The `check()` should return the winner or `None` if it is a draw. And `winner` would be a better name than `temp` for the result. With such a `check()` method `play_round()` might look like this:

Code: Select all

    def play_round(self):
        """Plays a round (best of 5)."""
        print("Best of Five - Let's play!\n")
        players = [self.p1, self.p2]
        while True:
            print()
            for player in players:
                player.make_move()
            winner, message = self.check(*players)
            print(message, end=' ')
            if winner:
                winner.scored()
                print(winner.name, 'won this round.')
                for player in players:
                    print('{p.name}: {p.score}'.format(p=player))
                if max(p.score for p in players) == 3:
                    return winner
                input()
From here it is very easy to also eliminate the last two attributes from `Rock` — the players — and get rid of the class. The `main()` is so short then that `run()` should be `main()`.

Code: Select all

#!/usr/bin/env python
# -*- coding: utf-8 -*-
"""rock.py - an example of OOP"""
import random


class Player:
    CHOICES = ['rock', 'paper', 'scissors']
    
    def __init__(self, name):
        self.name = name
        self.score = 0
        self.choice = None
        print('Player:', self.name, 'created!\n')

    def make_move(self):
        self.choice = random.choice(self.CHOICES)
        print(self.name, 'chose', self.choice, end='. ')

    def scored(self):
        self.score += 1


def check(player_a, player_b):
    if player_a.choice == player_b.choice:
        return (None, 'No winner.')
    
    if player_a.choice == 'rock' and player_b.choice == 'scissors':
        message = 'Rock breaks scissors.'
    elif player_a.choice == 'paper' and player_b.choice == 'rock':
        message = 'Paper wraps stone.'
    elif player_a.choice == 'scissors' and player_b.choice == 'paper':
        message = 'Scissors cut paper.'
    else:
        return check(player_b, player_a)
    
    return (player_a, message)


def play_round(players):
    """Plays a round (best of 5)."""
    print("Best of Five - Let's play!\n")
    while True:
        print()
        for player in players:
            player.make_move()
        winner, message = check(*players)
        print(message, end=' ')
        if winner:
            winner.scored()
            print(winner.name, 'won this round.')
            for player in players:
                print('{p.name}: {p.score}'.format(p=player))
            if max(p.score for p in players) == 3:
                return winner
            input()


def main():
    print('**** Welcome to Rock, Paper, Scissors! ****\n')
    while True:
        players = [
            Player(input('What shall we call Player %d? ' % (i + 1)))
            for i in range(2)
        ]
        winner = play_round(players)
        print('The winner was', winner.name + '.\n')
        if input("Enter 'y' to play again: ") != 'y':
            break
        print()
    print('Bye.')


if __name__ == '__main__':
    main()

Code: Select all

while not self.asleep():
    sheep += 1

antiloquax
Posts: 406
Joined: Sun Nov 20, 2011 11:37 am
Contact: Website

Re: Could someone who knows about OOP look at my code?

Sat Aug 11, 2012 6:18 pm

Many thanks BlackJack, for the detailed reply. I will have a closer read through later and rewrite my code.
:mrgreen:

Return to “Python”