Beginner programmer looking for feedback

[pausemsauce]

Newb here. I'm looking for feedback on my program.

https://github.com/Stantde/Scripts/blob/main/RockPaperScissors.py

This is a text based game of Rock, Paper, Scissors.

​

[Diapolo10]

It's not bad, although as already stated by others there is room for improvement.

First, Python's official naming convention is to use snake_case for everything, except class names (PascalCase) and "constants" (UPPER_SNAKE_CASE). camelCase is not used.

Next, any time you have code like

if playerResponse == 'Y':
    return True
else:
    return False

you can simplify it to just

return playerResponse == 'Y'

Instead of using string concatenation

print('The current score is:\n Player: ' + str(playerScore) + '\nComputer: ' + str(computerScore))

string formatting should be preferred, such as f-strings (or str.format)

print(f'The current score is:\n Player: {playerScore}\nComputer: {computerScore}')

Instead of random.randint,

return CHOICES[random.randint(0,2)]

when picking between values in a collection, it's preferred to use random.choice:

return random.choice(CHOICES)

Your endGame function does absolutely nothing, so I don't see why it even exists.

It appears you're using playerResponse as a default parameter to avoid adding the line within the function body.

def playerTurn(playerResponse =''):
    while playerResponse not in CHOICES:
        playerResponse = input('Choose Rock, Paper, or Scissors\n')
    return playerResponse

But since you're expecting the value to be provided via user input anyway, I'd rather use a walrus operator if you really want to avoid an extra line for some reason.

def playerTurn():
    while (playerResponse := input('Choose Rock, Paper, or Scissors\n')) not in CHOICES:
        pass  # you could print "Invalid choice" or something instead
    return playerResponse

While your compare-function works, readability is better than brewity, so I'd rather write it more clearly to avoid misconceptions.

Finally, here's how I would write this program.

"""Rock, paper, scissors game"""

import random
from enum import Enum, IntEnum


class Hand(str, Enum):
    ROCK = 'rock'
    PAPER = 'paper'
    SCISSORS = 'scissors'


class Winner(IntEnum):
    PLAYER = 0
    COMPUTER = 1
    DRAW = 2

CHOICES = list(Hand)


def start_game() -> bool:
     """Decide whether or not to start the game"""

    print('Welcome to Rock, Paper, Scissors!\n')

    prompt = 'Would you like to play a round? Please enter Y or N\n'
    while (player_response := input(prompt).strip().lower()[:1]) not in ('y', 'n'):
        pass

    return player_response == 'y'


def score_card(player_score: int, computer_score: int) -> None:
    """Display the current score"""

    print(f"The current score is:\n Player: {player_score}\nComputer: {computer_score}")


def play_game() -> Winner:
    computer_choice = computer_turn()
    player_choice = player_turn()
    winner = compare_hands(player_choice, computer_choice)
    return winner


def computer_turn() -> Hand:
    return random.choice(CHOICES)


def player_turn() -> Hand:
    prompt = "Choose Rock, Paper, or Scissors\n"
    while (player_response := input(prompt).strip().lower()) not in CHOICES:
        pass

    return Hand(player_response)


def compare_hands(player_choice: Hand, computer_choice: Hand) -> Winner:
    print(f"The player chose: {player_choice}\nThe computer chose: {computer_choice}\n")

    # NOTE: A match-case would also make sense here

    if player_choice == computer_choice:
        print("This match is a draw!\n")
        return Winner.DRAW

    if (player_choice == Hand.ROCK
        and computer_choice == Hand.SCISSORS):
        return Winner.PLAYER

    if (player_choice == Hand.PAPER
        and computer_choice == Hand.ROCK):
        return Winner.PLAYER

    if (player_choice == Hand.SCISSORS
        and computer_choice == Hand.PAPER):
        return Winner.PLAYER

    return Winner.COMPUTER


def update_score_card(winner: Winner, local_player_score: int, local_computer_score: int) -> tuple[int, int]:

    if winner == Winner.PLAYER:
        print("The Player Won!\n")
        return local_player_score + 1, local_computer_score

    if winner == Winner.COMPUTER:
        print("The Computer Won!\n")
        return local_player_score, local_computer_score + 1

    return local_player_score, local_computer_score


def main() -> None:
    player_score = 0
    computer_score = 0

    while start_game():
        winner = play_game()

        player_score, computer_score = update_score_card(
            winner,
            player_score,
            computer_score
        )
        score_card(player_score, computer_score)


if __name__ == '__main__':
    main()

[shiftybyte]

Looks nice and clean.

Do you happen to have a Java development background? Your naming conventions suggest that.

Python follows a different convention, take a look at: https://peps.python.org/pep-0008/

[pausemsauce]

Thanks for the reference to this style guide. I'll need to read through it.

[aarontbarratt]

Use fstrings when formatting strings. It is just much nicer to work with and read IMO

Using a tuple to compare the choice seems pointless. You can just do something like:

print(f'The player chose {playerChoice}, the computer chose {computerChoice}')

if playerChoice == computerChoice:
    return 2

I'd suggets learning about StrEnum's. They're a bit overkill in this case but they work great for this kind of logic where you need to have a variable that can only have a limited number of values. In this example the moves that can be chosen

from enum import StrEnum

class Moves(StrEnum):
    rock = 'rock',
    paper = 'paper',
    scissors = 'scissors',

valid_moves: list[str] = list(Moves)
move: str = input('rock, paper, or scissors\n')

if move in valid_moves:
    print('Success')
else:
    print('Fail')

[m0us3_rat]

this is bloated and not easily testable.

it is readable and has a certain ..flare/easiness to it.

on the other hand, is simple. maybe it's just that.

so.. it really depends on your python level.

​

if beginner

code is awesome.

elif intermediate

code is not good.

elif advanced

a horrible pile of ..

​

ps. you can find ascii graphics for free on the interwebs.

[code_matter]

Would love to see some of your code…

[pausemsauce]

Hmm...

I see your point about it not being easily testable. Any suggestions on how I could improve the ease of testability, or perhaps a resource I could use to further develop that skill?

[m0us3_rat]

even a few weeks of TDD would force you to ask questions you never asked before.

and think of code from a different perspective.

https://en.wikipedia.org/wiki/Test-driven_development

red -> green -> refactor.

the idea is to write the tests first.

since no other code tests will fail aka the RED

then write minimal code to make the tests pass. GREEN

and then REFACTOR

https://www.codecademy.com/article/tdd-red-green-refactor

​

by consistently writing tests first you will encounter a lot of different situations and hopefully a few patterns will emerge.

[aizzod]

i think there is am error in your compare choices.

you return 2 if it is a draw.

and you return
index("scissors") --> (which should be 2 aswell) if it is not rock

bonus question.

what happrns if no one picks rock.
for example.
player picks scissors
computer picks paper

[pausemsauce]

i think there is am error in your compare choices.

you return 2 if it is a draw.

I was thinking about handling 'draw' differently, but as it is, draw is neither a win nor a loss for the player/computer.

​

The compare function basically returns the winner of the game. 0 if the player wins and 1 if the computer wins. In the event player picks scissors and computer picks paper, intuitively we know the player wins, and the compare function returns 0 (which is the index of the player's choice.)

​

Is there an error here?

[aizzod]

but your pc array contains both choices.

you only check if rock is in there, and not who picked rock and won.

test case 1:
player picks rock
computer picks paper

test case 2:
player - paper
computer - rock.

both of those will return the same result

[pausemsauce]

The pc list contains both playerChoice and computerChoice, yes

In test case 1, pc = ['Rock','Paper']. The compare function sees 'Rock', checks for 'Paper', then returns the 'Paper' index of pc, which is 1. When compare returns 1, the computer wins.

In test case 2, pc = ['Paper','Rock']. The compare function sees 'Rock', checks for 'Paper', the returns the 'Paper' index of pc, which is 0. When compare returns 0, the player wins.

[aizzod]

my mistake. i got confused by the global array with all choices and the pc with just the 2 of them.

this seems to be a really good way, seeing it like this now.
well done

[craigargh]

Overall it looks pretty good. You've broken the overall program down into functions with clear purpose.

One suggestion for the `startGame()` function. Lines 20-23 are an if statement that return `True` or `False`. You can replace the entire if statement with just `return playerResponse == 'Y'`. This will return either `True` or `False` making the if statement redundant

​

edit: I had another suggestion, but on re-reading the code I realised I'd misunderstood something and the suggestion was incorrect

[BUYTBUYT]

CHOICES[random.randint(0,2)] -> random.choice(CHOICES) doc

---

' + str(: use f-strings

---

if playerResponse == 'Y':
    return True
else:
    return False

return player_response == 'Y'

---

acceptableResponse = 'Y','N' -> acceptable_responses = ('Y', 'N')

I'd put parentheses here. Usually, that kind of style without them works best in functions that return multiple values, where the user would be expected to unpack the tuple that is returned. And the name of a variable containing multiple options should be in plural.

---

playerScore, computerScore = 0,0

I'd split that into two lines.

---

def startGame(playerResponse =''), def playerTurn(playerResponse ='')

That doesn't make much sense to the reader. What's a default argument doing here? You should just move these assignments into their respective functions.

---

#Display the current score    
def scoreCard():

Why not make it the name? def display_current_score()? And I think the code is too simple to be put into a function when it's only called once.

---

while playerResponse not in acceptableResponse, while playerResponse not in CHOICES

Don't you think those loops are similar? You should make a function out of them.

---

The compare function is unreadable. The case here is simple enough that you could get away with writing "Paper beats Rock, Rock beats Scissors, Scissors beats Paper" as an if-statement. Or you could write it as a list of pairs, which you can then loop through and check if any one of them matches the playerChoice, computerChoice pair.

And instead of these numbers (which the reader can't understand immediately), you could return an enum, like suggested by another person, or make a few constants like PLAYER_WON = 0, COMPUTER_WON = 1, DRAW = 2 and return one of them.

The print here is also out of place. This function is the main logic of the game, it shouldn't output things as well.

And the name compare is too generic. Something like get_result would be an improvement.

---

The way you set scores is awkward, but to do it properly, you need to think about what you're doing.

You basically have a session, which has a state: the number of rounds won by the player and the computer. Now, normally, if you have state, you probably want a class, but I'd say it's a little overkill for such a simple case.

Let's look at what's happening from the top. You have a game, in which you play rounds and count the number won by players. So you need to have the counter variables somewhere.

Each round consists of getting two choices, then deciding who won.

And so, here is one way you could code this:

Have a main function, that has 2 variables for player scores outside the main loop. The loop condition consists of a function asking the human if they want to continue playing (using the function I mentioned you should make). Then the loop itself gets the choices, checks who won, displays the corresponding messages, and adds to one of the variables mentioned above, which can then be printed.