4
\$\begingroup\$

This is the first time I made a quick program, which is a game "paper, rock, scissors'. I am looking for any advice, and information if it is done in proper way or it is not.

I did my code by myself, and I would like to know is it a good part of code or I have to focus on something.

Here is a code I did:

import random
import time
#get user's name
while True:
        user = input("What is your name? (Type 'q' if You want to end the game): ")
        if user == 'q':
                print("Game over.")
                continue
        answer = f"Hello {user}. Welcome to the game!"
        reject = ('You have to type your name!')
        if user.strip() != '':
                time.sleep(.5)
                print(answer)
                break
        else:
                print(reject)
#get user's choice
def get_player_input():
        items = ['paper', 'rock', 'scissors']
        while True:
                user_choice = input('Choose your item (paper, rock, scissors): ')
                answer = f"You have chosen {user_choice}. Good luck!"
                reject = ('You have chosen wrong item. Try again!')
                if user_choice.strip() in items:
                        time.sleep(.5)
                        print(answer + " The game begins!")
                        break
                else:
                        print(reject)

#generate computer's choice
        computer_choice = random.randint(0,2)
        computer_move = items[computer_choice]
        time.sleep(1.5)
        print('Computer has chosen ' + computer_move)
#determine the winner
        if user_choice == computer_move:
                time.sleep(1.5)
                print("It is a draw!")
        elif (
                (user_choice == "rock" and computer_move == "scissors")
                or (user_choice == "paper" and computer_move == "rock")
                or (user_choice == "scissors" and computer_move == "paper")
        ):
                time.sleep(1.5)
                print(str(user_choice) + ' beats '+ str(computer_move) +'. ' + 'You won!')
        else:
                time.sleep(1.5)
                print('Oooops! ' + str(user_choice) + ' is beaten by ' + str(computer_move) + '. ' + 'You lost!')
#start the game
get_player_input()




\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

Don't lie to users. The instructions claim that entering a q will quit the game. But that's not what happens. Related to quitting, I would suggest a simpler approach: just interpret an empty response as a desire to quit.

Put all code in functions. This is a time-tested approach, passed down by the computing giants. The list of benefits is long. Here's a template for small scripts like your. The only things at the top level are imports, constants, and functions.

import sys
...

# Constants, if needed.

def main(args):
    # Program starts here.
    ...

def get_reply(...):
    ...

# Other functions, if needed.

if __name__ == '__main__':
    main(sys.argv[1:])

Define some constants. Don't litter the code with magical values like 'paper', 'rock', etc. Again, there are many benefits.

ROCK = 'rock'
PAPER = 'paper'
SCISSORS = 'scissors'
MOVES = (ROCK, PAPER, SCISSORS)

Parameterize the snoozing. Speaking of constants, it's annoying to work on a program that sleeps all the time. Each time you make a change and run the script you have to slog through the delays. For your own sanity, put that delay in a constant so you can set it to zero while you're working on the code.

DRAMA_LEVEL = 0

def main(args):
    ...
    time.sleep(DRAMA_LEVEL)
    ...

If you see repetition, generalize the needed behavior in a function. You need to get user input twice. In the first case, you'll accept almost any reply (user name) and in the second case, the user must select from a small set of choices (the moves). Writing a function to handle such a use case is very doable for beginners. Here is a preliminary sketch to get you started:

def get_reply(prompt, choices = None, quit_if_empty = True):
    while True:
        reply = input(prompt + ': ').strip().lower()
        if quit_if_empty and ...:     # Check for empty.
            ...
        elif choices and ...:         # Check for invalid reply.
            prompt = 'Invalid reply'
        else:
            return reply

Use random.choice(). It will do exactly what you need:

computer_move = random.choice(MOVES)

Smarter data, dumber code. Your current approach to determine the winner is heavy on algorithm -- lots of conditional logic. That's understandable and common when first starting programming. But it's often more effective to invest time in figuring out a way to express a problem in a data structure that will make the necessary algorithm much simpler. For example, you could put the player move and computer move in a tuple, such as ('rock', 'scissors'). There are only three ways for the player to win. Put the three winning tuples in a set. If the actual pairs of moves are in that set, then the player won; otherwise, the player lost. With the right data, the need for clever algorithm nearly fades away.

WINNERS = {
    (PAPER, ROCK),
    (ROCK, SCISSORS),
    (SCISSORS, PAPER),
}

def main(args):
    ...

    if user_move == computer_move:
        # Draw
    elif (user_move, computer_move) in WINNERS:
        # Player won.
    else:
        # Player lost.

Next steps: reconsider interactivity. Does your program really need to play-act a human-to-computer dialogue? The entire game could operate purely from command-line arguments (the args that main() receives in the sketch above). For example, a user could run the script like the example below. And you could easily add other behaviors to the program via command-line options (see argparse). Most real-world Python scripts are driven by arguments, not interactivity, so you might want to shift your learning in that direction.

python rock_paper_scissor.py Edward scissors
\$\endgroup\$
2
\$\begingroup\$

Thanks for posting your code and asking for feedback. You implemented the code and it works. You probably tested it. That's already a good step ahead. I see student's code which was not tested.

Now for the hard part: feedback time. I see the following areas of improvement:

  • separation of concerns: get things sorted.
  • object orientation: if you can identify nouns in a verbal description of your game (write it down!), maybe those are a good choice for classes. If you can identify verbs in that description, maybe those are a good choice for methods or functions.
  • Python details

The overview

Imagine a stranger like me getting an overview of your code. I'm going to look at the most left-aligned code, because that's what will be at the greatest abstraction level. I'm hoping to find classes (class X:) and functions (def f():) and a main entry point (if __name__ == "__main__":).

Here's what's top-level in your code:

  1. imports. That's fine. It gives me an idea about what the code will use (timing and randomness) and not use (OpenCV, NumPy, Networking, ...)
  2. #get user's name. Hmm, why isn't that a method get_users_name() then?
  3. while True: An endless loop right at the beginning. Will that end well?
  4. #get user's choice
  5. def get_player_input():. Why is it named different to what the comment says?
  6. #generate computer's choice. Why isn't that a method either? And what happened to the indentation?
  7. #determine the winner. Dito.
  8. get_player_input()
  9. No main entry point.

At this point, it's clear to me that I need to read all the code in order to understand it. There is no game class which would control the games flow, no player class, no rules class, nothing.

What could I find instead?

class RockPaperScissors:
    ...

class HumanPlayer:
    ...

class ComputerPlayer:
    ...

if __name__ == "__main__":
    ...

That would tell me:

  1. It's a game that I know.
  2. It has two kinds of players.
  3. There's a main method which runs the thing.

Setting up the human player

At the beginning, you want to set up a human player. That's what #get user's name tells me and what the while True: loop does. That code has aspects of two things: it controls the flow of the game (because the game can be over) and it acts like a builder for a human object.

Let me try to separate that loop into a player class and a game class:

import sys
class HumanPlayer:
    def __init__(self):
        self.username: str = ""

    def is_invalid(self) -> bool:
        return self.username.strip() == ""

    def wants_exit(self) -> bool:
        return self.username == "q"

class RockPaperScissors:
    def __init__(self):
        self.human = None

    def play(self):
        self.human = self.set_up_human()


    def set_up_human(self) -> HumanPlayer:
        human = HumanPlayer()
        while human.is_invalid() and not human.wants_exit():
            human.username = input("What is your name? (Type 'q' if You want to end the game): ")
            if human.is_invalid():
                print('You have to type your name!')
        if human.wants_exit():
            print("Game over.")
            sys.exit()
        print(f"Hello {human.username}. Welcome to the game!")
        return human

if __name__ == "__main__":
    game = RockPaperScissors()
    game.play()

Note how the while True: has disappeared and it's immediately clear that this is not an endless loop?

Player input

def get_player_input() sounds like a method for getting the player input, but actually it does everything: Human input, computer input and checking the outcome. Let's move the first part into the HumanPlayer class. For the computer, let's create a ComputerPlayer class with exactly the same function signature for duck typing reasons. And let's create a method for evaluating the match.

For the human:

class HumanPlayer:
    def choose_from(self, items) -> str:
        user_choice = ""
        while user_choice not in items:
            user_choice = input('Choose your item (paper, rock, scissors): ').strip()
            if user_choice not in items:
                print('You have chosen wrong item. Try again!')
        time.sleep(.5)
        print(f"You have chosen {user_choice}. Good luck!" + " The game begins!")
        return user_choice   

Also no endless loop here.

For the computer:

class ComputerPlayer:
    def __init__(self):
        self.username = "Computer"

    def choose_from(self, items) -> str:
        computer_choice = random.randint(0, len(items))
        computer_move = items[computer_choice]
        time.sleep(1.5)
        print('Computer has chosen ' + computer_move)
        return computer_move

Do you recognize that computer_choice and computer_move are hard to distinguish? I'll fix that later.

For the gameplay:

class RockPaperScissors:
    def beats(self, item_a, item_b):
        higher = {"rock": "scissors", "paper": "rock", "scissors": "paper"}
        return higher[item_a] == item_b

    def evaluate(self):
        time.sleep(1.5)
        if self.human.choice == self.computer.choice:
            print("It is a draw!")
            return

        if self.beats(self.human.choice, self.computer.choice):
            print(self.human.choice + ' beats ' + self.computer.choice + '. ' + 'You won!')
            return

        print('Oooops! ' + self.human.choice + ' is beaten by ' + self.computer.choice + '. ' + 'You lost!')

Note that I made a short function that can tell, whether one choice beat the other. It uses a dictionary. Read it and find out how it works.

Fine-tuning

  • You know f-Strings. Let's use them everywhere.
  • The random class has a built-in choice() method. No need to calculate a number
  • See the names in the Computer.choose_from() method. They don't match. Let's fix that.

Final result

Look at this method:

def play(self):
    self.human = self.set_up_human()
    self.computer = ComputerPlayer()
    items = ['paper', 'rock', 'scissors']
    self.human.choose_from(items)
    self.computer.choose_from(items)
    self.evaluate()

Does that make sense for a Rock, Paper, Scissors game?

import random
import sys
import time


class HumanPlayer:
    def __init__(self):
        self.username: str = ""
        self.choice = ""

    def is_invalid(self) -> bool:
        return self.username.strip() == ""

    def wants_exit(self) -> bool:
        return self.username == "q"

    def choose_from(self, items):
        user_choice = ""
        while user_choice not in items:
            user_choice = input("Choose your item (paper, rock, scissors): ").strip()
            if user_choice not in items:
                print("You have chosen wrong item. Try again!")
        time.sleep(.5)
        print(f"You have chosen {user_choice}. Good luck! The game begins!")
        self.choice = user_choice


class ComputerPlayer:
    def __init__(self):
        self.username = "Computer"
        self.choice = ""

    def choose_from(self, items):
        self.choice = random.choice(items)
        time.sleep(1.5)
        print(f"Computer has chosen {self.choice}")


class RockPaperScissors:
    def __init__(self):
        self.human = None
        self.computer = None

    def play(self):
        self.human = self.set_up_human()
        self.computer = ComputerPlayer()
        items = ['paper', 'rock', 'scissors']
        self.human.choose_from(items)
        self.computer.choose_from(items)
        self.evaluate()

    def set_up_human(self) -> HumanPlayer:
        human = HumanPlayer()
        while human.is_invalid() and not human.wants_exit():
            human.username = input("What is your name? (Type 'q' if You want to end the game): ")
            if human.is_invalid():
                print('You have to type your name!')
        if human.wants_exit():
            print("Game over.")
            sys.exit()
        time.sleep(.5)
        print(f"Hello {human.username}. Welcome to the game!")
        return human

    def beats(self, item_a, item_b):
        higher = {"rock": "scissors", "paper": "rock", "scissors": "paper"}
        return higher[item_a] == item_b

    def evaluate(self):
        time.sleep(1.5)
        if self.human.choice == self.computer.choice:
            print("It is a draw!")
            return

        if self.beats(self.human.choice, self.computer.choice):
            print(f"{self.human.choice} beats {self.computer.choice}. You won!")
            return

        print(f"Oooops! {self.human.choice} is beaten by {self.computer.choice}. You lost!")


if __name__ == "__main__":
    game = RockPaperScissors()
    game.play()

Is it perfect now?

Is that a perfect solution? Still not. There's room for improvement. E.g. I dislike:

  • both, the human player and the computer player still have impact on the timing of the game, because they sleep themselves
  • both players print messages to the console. That makes it hard to have a different UI for the game.
  • the game class does not only know the rules of the game, it also has the UI built-in
  • there's repetition for the items ("rock", "paper", "scissors") for which a typo could easily break the game.

However, the answer is already quite long and I'll leave it for now. Please come back with your next implementation and let us know how you improve from here.

\$\endgroup\$
1
  • 2
    \$\begingroup\$ This is a good answer, but I'm not sure if it's the best approach considering OP's experience level and the simple requirements of the project. A more straightforward, functional approach might be more fitting in this case. \$\endgroup\$ Jun 7, 2023 at 7:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.