7
\$\begingroup\$

I am still practising object oriented programming and I decided to do the rock-paper-scissors game as a project. I listen to constructive criticism to improve the program.

#!/usr/bin/env python3
# Rock, Paper, Scissors
from os import system
from random import randint
from sys import exit


class rock_paper_scissors:
    def __init__(self):
        self.choices = "rock", "paper", "scissors"
        self.player_wins = 0
        self.computer_wins = 0

    def _spacer_size(self, length=65):
        return '-' * length

    def _player_move(self):
        while True:
            try:
                option = int(input('Choose an option between Rock (1), Paper (2), Scissors (3): '))

                if 1 <= option <= 3:
                    break
                else:
                    print('You can only enter a number between 1 and 3.')    
            except ValueError:
                print('The value entered is invalid. You can only enter numeric values.')

        return option

    def _computer_move(self):
        return randint(1,3)

    def _check_winner(self):
        if self.player_wins == self.computer_wins:
            return 'Tie.'
        elif self.player_wins > self.computer_wins:
            return 'You won the set.'
        else:
            return 'Computer wins the set.'

    def _play(self):
        times = int(input("How many times do you wish to play?: "))

        for i in range(times):
            player = self._player_move()
            computer = self._computer_move()
            print(f"You chose {self.choices[player-1]}.")
            print(f"The computer chose {self.choices[computer-1]}.")

            if player == computer:
                print('Tie.\n')
                print(self._spacer_size(), '\n')
            elif (player-computer) % 3 == 1:
                print('You won.\n')
                print(self._spacer_size(), '\n')
                self.player_wins += 1
            else:
                print('You lost.\n')
                print(self._spacer_size(), '\n')
                self.computer_wins += 1

        print(self._check_winner())
        input("Press a key to return to the main menu...")
        system("CLS")
        self.main()
            
    def main(self, length=95):
        while True:
            try:
                print('-' * length, '\n')
                print('''
                █▀█ █▀█ █▀▀ █▄▀ ░   █▀█ ▄▀█ █▀█ █▀▀ █▀█ ░   █▀ █▀▀ █ █▀ █▀ █▀█ █▀█ █▀
                █▀▄ █▄█ █▄▄ █░█ █   █▀▀ █▀█ █▀▀ ██▄ █▀▄ █   ▄█ █▄▄ █ ▄█ ▄█ █▄█ █▀▄ ▄█                                                   
                '''.center(10))
                print('-' * length, '\n')
                print('1. Play'.center(length))
                print('2. Instructions'.center(length))
                print('3. Exit'.center(length))
                choice = int(input('\nEnter an option: '))
            except ValueError:
                print('The value entered is invalid. You can only enter numeric values.')

            if choice == 1:
                system("CLS")
                self._play()
                break
            elif choice == 2:
                system("CLS")
                print("  Instructions for Rock, Paper, Scissors: ")
                print("- Rock wins over scissors (because rock smashes scissors).")
                print("- Scissors wins over paper (because scissors cut paper).")
                print("- Paper wins over rock (because paper covers rock).")
                print("- If both players show the same sign, it's a tie.\n")
                input("Press a key to return to the main menu...")
                system("CLS")
            elif choice == 3:
                exit()
            else:
                print("You have entered a number that isn't in the list.")
                system("CLS")
            

if __name__ == '__main__':
    game = rock_paper_scissors()
    game.main()
\$\endgroup\$
5
  • \$\begingroup\$ 1st read a single character instead of input. This could allow omitting messages about wrong input (just get a character in a while loop until valid). 2nd I'd make public current score somewhere in _player_move or inside for loop in _play e.g. f"{self.player_wins}:{self.computer_wins}" Then you know that score isn't initialized properly to 0:0 for rerun Play inside while loop in game.main(). 3rd Allow r/p/s alongside 1/2/3. 4th Is system("CLS") valid in *nix? (IMHO, you clear screen too often…) \$\endgroup\$
    – JosefZ
    Feb 17 at 22:07
  • \$\begingroup\$ My previous comment is about design rather than pythonic approach… \$\endgroup\$
    – JosefZ
    Feb 17 at 22:09
  • \$\begingroup\$ *nix is linux?? \$\endgroup\$ Feb 17 at 22:11
  • \$\begingroup\$ stackoverflow.com/questions/4715374/what-is-the-meaning-of-nix \$\endgroup\$
    – JosefZ
    Feb 17 at 22:23
  • \$\begingroup\$ I think your _spacer_size is a misnomer, the name suggests its return value should be an int, but it returns a str, and it really shouldn't be a function, but an attribute named spacer instead, your _computer_move is really too short to be its own function, it should be a part of _play instead (function calls are expensive), and I fail to see the reason why all functions except main are private... \$\endgroup\$ Feb 20 at 14:49

2 Answers 2

5
\$\begingroup\$

One suggestion would be to use an enum instead of a tuple.

#!/usr/bin/env python3
# Rock, Paper, Scissors
from enum import Enum
from os import system
from random import randint
from sys import exit


class Hand(Enum):
    ROCK = 1
    PAPER = 2
    SCISSORS = 3
   
    def __str__(self):
       return self.name.title()

 class rock_paper_scissors:
    def __init__(self):
       # self.choices = "rock", "paper", "scissors"
       self.player_wins = 0
       self.computer_wins = 0

    ...

       for i in range(times):
          player = self._player_move()
          computer = self._computer_move()
          print(f'{player} {computer} ')
          # print(f"You chose {self.choices[player-1]}.")
          print(f'You chose {Hand(player)}')
          # print(f"The computer chose {self.choices[computer-1]}.")
          print(f'The computer chose {Hand(computer)}')

In your use case, it doesn't do much, besides dump the use of math related to the tuple position, but its cleaner and makes more sense this way.

Other than that enums are easy to use and offer a lot of functionality.

for instance

x = Hand.ROCK
print(x)
print(x.name)
print(x.value)
print(x is Hand.ROCK)
print(x is Hand.PAPER)

you have the ability to assign a value, string, easy comparisons. enums are easy to iterate through as well.

for hand, value in Hand.__members__.items():
    print(f'{hand} => {value}')

There's a lot more and would recommend looking into them, if you were't already familiar.

suggestion #2:

another thing you could consider, and honestly this is a personal preference, is to use a single print statement for multiple lines. In my opinion, it looks cleaner and is easier to read.

print(f"""
    Instructions for Rock, Paper, Scissors:
    - Rock wins over scissors (because rock smashes scissors).
    - Scissors wins over paper (because scissors cut paper).
    - Paper wins over rock (because paper covers rock).
    - If both players show the same sign, it's a tie.\n
    Press a key to return to the main menu...                        
    """.center(length))

suggestion #3:

your system clear is nice, it keeps the terminal, game screen decluttered. However, system('cls') only works on windows. I am using Linux. So if you want to make your game cross-platform you need to be on the lookout for those issues.

create a new function that will check the system platform and then execute the appropriate command.

def clearScreen(self):
    if platform == "linux" or platform == "linux2":
        system('clear')
        # linu
    elif platform == "darwin":
        pass
        # OS X
    elif platform == "win32":
        system('cls')
        # Windows...

and just call this function each time you want to clear the screen.

Actually, what was I thinking. Just check once as to what your system is in your constructor. This way you set a constant that can be used for your system call, without needing to check every time.

class rock_paper_scissors:
    def __init__(self):
           

        if platform == "linux" or platform == "linux2":
            self.clear = 'clear'
            # linu
        elif platform == "darwin":
            pass
            # OS X
        elif platform == "win32":
            self.clear = 'cls'
            # Windows...

        self.player_wins = 0
        self.computer_wins = 0
\$\endgroup\$
1
2
\$\begingroup\$

PEP-8

The Style Guide for Python Code has many recommendations you should follow. Perhaps the most important is naming. snake_case is used for variables, functions and method names. Class names should be BumpyWords.

class RockPaperScissors:
    ...

Clear screen

os.system(...) launches a new process. This is slow, difficult for cross-platform compatibility, and a security risk.

Instead, use a terminal module like colorama to clear the screen, eg)

import colorama

def clear_screen() -> None:
    print(colorama.ansi.clear_screen(), end='')

if __name__ == '__main__':
    try:
        colorama.init()
        clear_screen()
    finally:
        colorama.deinit()
\$\endgroup\$
2
  • \$\begingroup\$ The colorama module does not work for me, the screen does not clear. The code: github.com/xSyrax123/rock_paper_scissors \$\endgroup\$ Feb 18 at 19:27
  • \$\begingroup\$ @LucioMazzini Apologies. I forgot the colorama.init(), recommended for *nix platforms and absolutely required for Windows. \$\endgroup\$
    – AJNeufeld
    Feb 18 at 19:44

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.