4
\$\begingroup\$

I am comfortable with the procedural style, and learning the object oriented, so I have done a small Rock Paper Scissors game in both styles.

This script is in procedural style and this one is in object oriented style. Did I implement this little game correctly object oriented wise? And if no, where did I mess things up? And how can I improve them?

This is the code in procedural style:

import random, sys

def computer_choice():
    computer_choice = random.choice("rps")
    return computer_choice

def user_choice():
    user_ch = raw_input(" Your Choice ?   ")
    if user_ch != "r" and user_ch != "p" and user_ch != "s":
        print " Wrong input, Please try again.\n"
        user_ch = user_choice()

    return user_ch

def compare_choices(user_choice, computer_choice):
   if user_choice == "r" and computer_choice == "p":
        return "computer"
   elif user_choice == "r" and computer_choice == "s":
       return "user"
   elif user_choice == "r" and computer_choice == "r":
       return "n" # n means nul
   elif user_choice == "p" and computer_choice == "r":
      return "user"
   elif user_choice == "p" and computer_choice == "s":
      return "computer"
   elif user_choice == "p" and computer_choice == "p":
      return "n"
   elif user_choice == "s" and computer_choice == "r":
      return "computer"
   elif user_choice == "s" and computer_choice == "p":
      return "user"
   elif user_choice == "s" and computer_choice == "s":
      return "n"

def play_again():
    print "Game Over"
    choice = raw_input(" Would you like to play again? (Y/N) ")
    if choice == "y" or choice == "Y":
        play()
    elif choice == "n" or choice == "N":
        sys.exit()
    else:
        print " Wrong input, Please try again.\n"
        play_again()

def play():
    global user_score, computer_score
    i = 1
    while i <= 3:
        user_c = user_choice()
        computer_c = computer_choice()
        result = compare_choices(user_c, computer_c)
        if result == "user":
            user_score += 1
            print " Your choice was " + user_c + ", the computer's choice was "+ computer_c
            print " You WON ! \n"
        elif result == "computer":
            computer_score += 1
            print " Your choice was " + user_c + ", the computer's choice was "+ computer_c
            print " The computer won. \n"
        else:
            print " Your choice was " + str(user_c) + ", the computer's choice was "+ str(computer_c)
            play_again()
        i += 1
    print "Your score was: "+str(user_score)+", The Computer's score was: "+str(computer_score)
    if user_score > computer_score:
        print "You are the WINNER, CONGRATULATIONS! \n\n"
    else:
        print "The Computer is the WINNER. \n\n"

play_again()

user_score = computer_score = 0
play()

And this is the code in object oriented style:

import random, sys

class Player:#two methods in comun between user player and computer player
    score = 0

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

    def get_score(self):
        return self.score
class Computerp(Player): # The computer player class, it has in addition to the two  Player method, it's own choose method.

    def choose(self):
        computer_choice = random.choice("rps")
        return computer_choice

class Userp(Player): # Same as computer player class

    def choose(self):
        user_ch = raw_input(" Your Choice ?   ")
        if user_ch != "r" and user_ch != "p" and user_ch != "s":
            print " Wrong input, Please try again.\n"
            user_ch = self.choose()

        return user_ch

class Game: # This is a wrapper class for the "Game logic", it has: compare_choices method, play, and play_again methods.

    def compare_choices(self, computer_choice, user_choice):
        if user_choice == "r" and computer_choice == "p":
            return "computer"
        elif user_choice == "r" and computer_choice == "s":
            return "user"
        elif user_choice == "r" and computer_choice == "r":
            return "n" # n means nul
        elif user_choice == "p" and computer_choice == "r":
            return "user"
        elif user_choice == "p" and computer_choice == "s":
            return "computer"
        elif user_choice == "p" and computer_choice == "p":
            return "n"
        elif user_choice == "s" and computer_choice == "r":
            return "computer"
        elif user_choice == "s" and computer_choice == "p":
            return "user"
        elif user_choice == "s" and computer_choice == "s":
            return "n"

    def play(self):
        user = Userp()
        computer = Computerp()
        i = 1
        while i <= 3:
            user_ch = user.choose()
            computer_ch = computer.choose()
            result = self.compare_choices(computer_ch, user_ch)
            if result == 'user':
                user.update_score()
                print " Your choice was " + user_ch + ", the computer's choice was "+ computer_ch
                print " You WON ! \n"
            elif result == 'computer':
                computer.update_score()
                print " Your choice was " + user_ch + ", the computer's choice was "+ computer_ch
                print " The computer won. \n"
            else:
               print " Your choice was " + str(user_ch) + ", the computer's choice was "+ str(computer_ch)
               self.play_again()
            i = i + 1
        print "Your score was: "+str(user.get_score())+", The Computer's score was: "+str(computer.get_score())
        if user.get_score() > computer.get_score():
            print "You are the WINNER, CONGRATULATIONS! \n\n"
        else:
            print "The Computer is the WINNER. \n\n"
        self.play_again()
game = Game()

game.play()
\$\endgroup\$
5
\$\begingroup\$

Both versions

Imports should be on separate lines:

import random
import sys

Your compare_choices code in both versions could be simplified with a dictionary - note that tuples, which are hashable and immutable, can be used as dictionary keys:

def compare_choices(user_choice, computer_choice):
    result = {('r', 'p'): 'computer',
              ('r', 's'): 'user',
              ('r', 'r'): None, # None is a more conventional null
              ...}
   return result[user_choice, computer_choice]

In both cases, you should "guard" the code that runs at the top level of the script:

if __name__ == "__main__":
    ...

this makes it easier to import their functionality into other scripts later.


Use a string formatting method, rather than concatenating strings:

" Your choice was {0}, the computer's choice was {1}".format(user_ch, computer_ch)

this is more readable, and also more efficient.


 i = 1
 while i <= 3:
     ...
     i += 1

would be much neater as:

for i in range(1, 4):

or, as you never actually use the value of i:

for _ in range(3):

OOP Version

All classes in new Python code should really be "new-style" classes, i.e. inherit from object (this is done automatically in 3.x code):

class Player(object):

Note that Player.score is a class attribute, not an instance attribute, in your current code. This is a common "gotcha" - it's not a big problem here, as the attribute is immutable, but should be avoided:

class Player(object):        
    def __init__(self):
        self.score = 0

There's also no need for the "get" and "set" methods of your Players, just access the score attribute directly:

if user.score > computer.score:

You can easily convert the attribute to an @property later if necessary.


You can signal that choose is not implemented by the parent class but should by implemented by all child classes with the following convention:

class Parent(object):

    def some_method(self, *args, **kwargs):
        """This method must be implemented by classes inheriting from Parent."""
        raise NotImplementedError

ComputerPlayer and UserPlayer, or just Computer and User, would be better names for your sub-classes - the lower-case p suffix is awkward and unnecessary.


Procedural version

global user_score, computer_score

is always a bad sign - instead of globals, consider explicitly passing around the two scores, e.g. as separate variables or in a single dictionary {'user': 0, 'computer': 0}.


The recursive implementation of your procedural version means that a very long game will eventually hit the recursion limit - have a go at writing an iterative version instead.

\$\endgroup\$
  • \$\begingroup\$ your answer helped me, thank you @jonrsharpe \$\endgroup\$ – the-sizixe Sep 13 '14 at 21:20
2
\$\begingroup\$

One nitpick in user_choice() and choice()

def choose(self):
    user_ch = raw_input(" Your Choice ?   ")
    if user_ch != "r" and user_ch != "p" and user_ch != "s":
        print " Wrong input, Please try again.\n"
        user_ch = self.choose()

    return user_ch

This function uses recursion for evaluating a choice which could never be correct. This leaves a hole in your program where a user can enter enough wrong values and crash the program due to reaching the recursion limit.

A safer way of making the method would be to use a while loop instead.

def choose(self):
    user_ch = raw_input(" Your Choice ?   ")
    while user_ch != "r" and user_ch != "p" and user_ch != "s":
        print " Wrong input, Please try again.\n"
        user_ch = raw_input(" Your Choice ?   ")

    return user_ch
\$\endgroup\$

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.