Rock-Paper-Scissors in procedural and object-oriented

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 computer_score:
print "You are the WINNER, CONGRATULATIONS! \n\n"
else:
print "The Computer is the WINNER. \n\n"

play_aga

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.

StackExchange Code Review Q#62312, answer score: 5