15
\$\begingroup\$

I finished Python Principles and this is my first program I've done (mostly) from scratch since, having to get some outside help on the loop to repeat the program, and I saw a nifty thing about using modular math to figure out the results so I did that to save a bunch of lines.

I'm just looking for any comments, improvements, bad habits, any comments at all!

import random
options = ['rock', 'paper', 'scissors']

def pickRando(): #set random choice for CPU
        global randoPick
        randoPick = random.choice(options)

def game(): #start up the game
    global player
    print('Rock, Paper, Scissors:')
    player = input("Choose wisely: ") #get player input
    player = player.lower()
    if player in options:
        check = True
        print("Okay, you picked " + player + ' and I picked ' + randoPick + '!')
        return check
    else:
        print('You have not chosen a viable option! Try again')
        check = False
        return check

def convertible(swap): #changing the word into a number
    if swap == 'rock':
        swap = 0
    elif swap == 'paper':
        swap = 1
    else:
        swap = 2
    return swap

def combatCheck(a, b): #determine the results of the choices
    a = convertible(a)
    b = convertible(b)
    result = (a - b) % 3 #modmath
    return result

def finish(z): # report the results
    global wins
    global losses
    global ties
    if z == 0:
        print('A tie! You are a most worthy opponent! Go again?')
        ties = ties + 1
    if z == 1:
        print('You win! My honor demands a rematch!')
        wins = wins + 1
    if z == 2:
        print('Haha, I am victorious! Dare you challenge me again?')
        losses = losses + 1
    print('You have ' + str(wins) + ' wins, ' + str(losses) + ' losses and, ' + str(ties) + ' ties!')

wins = 0
losses = 0
ties = 0

while True :
    pickRando()
    check = False
    while check == False:
        check = game()
    finish(combatCheck(player, randoPick))
    while True: #looping
        global answer
        answer = input('Run again? (y/n): ')
        if answer in ('y', 'n'):
            break
            print('Invalid input.')
    if answer == 'y':
        continue
    else:
        print('You are weak!')
        break
\$\endgroup\$
  • \$\begingroup\$ What exactly did "Python Principles" cover? Did it cover classes as well? If it did, I'll write my review/answer up because I kiiiinda rewrote your program with classes, which removes a lot of the global complexity and rewrites a good bit of your code so you don't need code repetition or the use of functions to determine numeric values. If it did not cover classes, then I'll not share my rewrite because you don't know the basic principles of it. \$\endgroup\$ – Thomas Ward Nov 1 at 20:31
  • \$\begingroup\$ @ChaosHat, do you think that between our three answers, there is anything else that you'd like explained more in depth so that you have some more context or so that it is easier to understand? Do you have any feedback on our answers? I hope everything thus far has helped you. \$\endgroup\$ – TrevorPaulMartin Nov 1 at 20:43
  • 1
    \$\begingroup\$ @ThomasWard it did not include classes, but please do that anyways! I could benefit from such a direct example to learn from. \$\endgroup\$ – ChaosHat Nov 1 at 20:57
  • 1
    \$\begingroup\$ @TrevorPaulMartin I am reading everything over now having got home from work. Everyone has been very helpful in pointing things out and a lot of comments reinforce similar points so at least it seems like the feedback is useful. At first I was thinking about rewriting the program after feedback but I feel like maybe it would be best to move on for now, and then revisit it again in a few months and just see how different it is. \$\endgroup\$ – ChaosHat Nov 1 at 20:58
  • 1
    \$\begingroup\$ @ChaosHat OK, I wanted to make sure I wasn't going to overload your understandings by rewriting it as a Python class to handle the actual game object/mechanics while only handing off 'replay' to the actual 'python' program outside of the class. gist.github.com/teward/d303af90949b39a1f94dce84da973ac8 contains all the code, but I also posted this as codereview.stackexchange.com/questions/231706/… to get input on how to improve my variant too :) \$\endgroup\$ – Thomas Ward Nov 1 at 21:03
12
\$\begingroup\$

This code reads like beginner code. Good beginner code, but there are a few habits you're getting into that are systemic among beginners and will screw you over later down the line.

First, your function names are somewhat confusing. I would rename pickRando to computerChoice, combatCheck to game, and finish to something like printRecord. Once you do that, you shouldn't need the comments describing them. Comments are good, but shouldn't be used in lieu of descriptive names.

Second, avoid global variables. It will cause confusing issues later down the line, so please get into the habit of not using them. In order to avoid the global variables wins, losses, and ties, you would need to restructure a lot of code and it may not be worth it for this small of a code base. Exercise to the reader, I guess. But the other globals can easily be removed. I would change pickRando to

def computerChoice():
    return random.choice(options)

The concept behind the input check is good, but I would heavily modify the structure. Here's a better way to do that

while True:
    [...]
    print("Rock, Paper, Scissors:")

    playerChoice = input('choose wisely')
    while  not playerChoice in options:
        print "invalid input"
        playerChoice == input('choose wisely')
    print("Okay, you picked " + playerChoice + ' and I picked ' + computerChoice + '!')

This restructuring should make what it's actually doing clearer.

Next: the convertible method is a very good solution if you don't know about the built-in method index. You can change the combatCheck method to something like this.

a = options.index(a)
b = options.index(b)
return (a-b) % 3

I would also comment the last line with something more descriptive than modmath. I might say something like uses modular arithmetic to calculate results.

I like that you put combatCheck in its own function from the perspective of the teacher, because it shows that you use functions, but I think it's unnecessary here, because you only use that bit of code once and it's only a few lines.

You definitely have some really great foundations. Most of your mistakes are mistakes I made in my early days of Python, and I like to think I'm a pretty OK programmer. I think with practice and time, you'll turn into an amazing programmer.

\$\endgroup\$
  • 1
    \$\begingroup\$ I think a lot of my bad variable or method names (like finish) came from my iterative process. Some did different things and as I added more stuff (replays/score tally/final clean up) the methods added or changed. Should have touched on it in clean up. Index is useful for this so thanks for that. I thought about a dictionary but I couldn't find a simple way to look a thing up and swap it for the value in the dictionary. Combat check originally did more, and got pared down. I'm looking for the sweet spot of "should this be a function, part of another, or just not at all." \$\endgroup\$ – ChaosHat Nov 1 at 21:09
  • 1
    \$\begingroup\$ To add to the content of this post, please see PEP8 for function naming guidelines. \$\endgroup\$ – Modelmat Nov 2 at 11:37
  • \$\begingroup\$ @ChaosHat unfortunately, the only way to find that sweet spot is lots of practice. \$\endgroup\$ – Levi Willrich yesterday
7
\$\begingroup\$

You are abusing globals here. Ideally, functions should take in data using parameters, and return data they produce. Reassigning globals like you are makes it much more difficult to tell what a function does when reading code.

For example, instead of

def pickRando():
    global randoPick
    randoPick = random.choice(options)

. . .

finish(combatCheck(player, randoPick))

You should get rid of the global randoPick and just do:

def pickRando():
    # The data is simply returned instead of altering a global
    return random.choice(options)  

. . .

randoPick = pickRando()  # local instead of global
finish(combatCheck(player, randoPick))

The less "behind-the-scenes" data manipulation you do, the better. Code is much easier to reason about when function calls are simply an in/out flow of information.


There's multiple odd things going on with game.

  • It alters the global check, then returns check, then you do check = game() again when calling game.

  • You're returning whether or not the input failed, then dealing with a bad result externally when calling game.

I would make a helper to take input in a loop, and eliminate the global check. We just need a function that will loop for us while the input is invalid. Something like:

from typing import Callable

def get_valid_input(ask_message: str, error_message: str, validator: Callable[[str], bool]):
    while True:
        user_input = input(ask_message)

        if validator(user_input):
            return user_input

        else:
            print(error_message)

It loops for you until validator returns True. validator is a function that we supply that tells it if an input is valid or not.

I'd also re-jig things a bit and alter the responsibility of game. Once you make the changes I suggested, you're basically just left with:

def game(): #start up the game
    print('Rock, Paper, Scissors:')
    player = get_valid_input("Choose wisely: ",
                             'You have not chosen a viable option! Try again',
                             lambda move: move.lower() in options)

    print("Okay, you picked " + player + ' and I picked ' + randoPick + '!')

It doesn't seem to have much point. I'd change this to something like a play_round function that handles the entirety of one round:

def play_round():
    computer_move = pickRando()

    print('Rock, Paper, Scissors:')
    player_move = get_valid_input("Choose wisely: ",
                             'You have not chosen a viable option! Try again',
                             lambda move: move.lower() in options)

    print("Okay, you picked " + player_move + ' and I picked ' + computer_move + '!')

    finish(combatCheck(player_move, computer_move))

This eliminates multiple globals, and makes the calling code make a lot more sense:

while True:
    play_round()

    answer = get_valid_input("Run again? (y/n): ",
                             "Invalid input.",
                             lambda a: a in {'y', 'n'})

    if answer == 'y':
        continue

    else:
        print('You are weak!')
        break

Now you aren't needing to manually validate input, which gets rid of a lot of messy looping.



This still has a lot that can be commented on:

  • There's still some globals in charge of keeping track of the scores. I would bundle those scores into a class or tuple or something, and pass them into and out of play_round.

  • convertible can be simply made into a dictionary:

    {'rock': 0,
     'paper', 1,
     'scissors', 2}
    

    Then you can do a = convertible[a]. Note though that this will raise an error instead of defaulting to 2 if somehow bad input makes its way through. This is arguably a good thing though.

  • A lot of your comments are useless. Comments like in (a - b) % 3 #modmath and input("Choose wisely: ") #get player input are just repeating what the code says. Comments should explain why code is as it is; if that's necessary. Ideally, your code should be "fluent" enough that you don't need to comment what a code does because it's obvious already.

I hate to rush reviews, but FedEx just got here with my new laptop :D

\$\endgroup\$
  • \$\begingroup\$ Thanks! I know I'm using too many globals. I had more, and then I felt removing the last couple meant rewriting all of it. My prior experience is couple 200 level Java classes a few years ago so I think the way I think about variables stem from that. As far as your randoPick fix, I did think of that but I wasn't sure whether or not it would be as bad to nest a ton of functions as far as readability. Any solution beyond renesting it felt like a rewrite. I also thought of a dictionary, I wasn't sure if it was better, worse, or just different. It seemed like an overall same amount of effort. \$\endgroup\$ – ChaosHat Nov 1 at 20:53
  • \$\begingroup\$ @ChaosHat Calling functions inside of other functions is fine. If each function has a well-defined purpose, "nesting" functions will be a lot more readable than trying to do everything manually. Unless you don't define your own functions (which isn't a good idea), you'll always be calling functions from within other functions. And in this case, whether or not you use ifs or a dictionary is a matter of taste since performance isn't a concern. You could have also used an enum. \$\endgroup\$ – Carcigenicate Nov 1 at 21:02
5
\$\begingroup\$

Just for the record, when I saw the abuse of global arguments above and a ton of Python program level looping just to handle a lot of the game functionality, etc., I immediately thought this could be completely redesigned around a class to handle the actual game itself, track score, etc. for the entire game, and pretty much eliminate the reliance on globals and passing variables around outside the class.

As your Python Principles course didn't touch upon class objects, W3Schools has a half-decent rough explanation of a class and how it works with internal variables and methods/functions here, though you're bound to learn classes in-depth if you take more advanced Python courses.

This said, using a class to instantiate the game itself actually alleviates much of your global abuse and much of the passing of score objects around as 'program level' objects, and keeps everything as part of a singular game object when you use a class to define the game object, its mechanics, and its variables internal to itself, so it's available to the game object as you go. Also makes having to pass data between the various functions and methods a lot easier, since everything's referred to as an object within the game instance of RockPaperScissors itself! Cool, huh?


Anyways, I rewrote your game mechanics and functionality as a class, and kept all the base functionality you do for your game mechanics as part of the class (choosing randomly, checking if a win/loss/tie and keeping track of scores, actually running the game, and actually handling 'wrong' inputs of choices), but made a much simpler mechanism to handle the mapping of string-to-numeric-value mapping of choices using a dictionary instead within the class.

I moved the checking for continuing playing, however, to outside the class as part of the actual Python 'program' execution.

The game is initially created as an instance of the RockPaperScissors class, and we just straight refer to the game object outside the class for running a round of RPS and outputting the current scores; everything in terms of score, game mechanics, etc. is all kept within the game object as variables or methods within the class itself.

I also rewrite your functions for the mechanics to be snake_case instead of camelCase, but keep most of the stuff the same, just slightly more Pythonic (with if/elif instead of more than one if statement, etc.)

import random


class RockPaperScissors:
    """
    Class to handle an instance of a Rock-Paper-Scissors game
    with unlimited rounds.
    """

    def __init__(self):
        """
        Initialize the variables for the class
        """
        self.wins = 0
        self.losses = 0
        self.ties = 0
        self.options = {'rock': 0, 'paper': 1, 'scissors': 2}

    def random_choice(self):
        """
        Chooses a choice randomly from the keys in self.options.
        :returns: String containing the choice of the computer.
        """

        return random.choice(list(self.options.keys()))

    def check_win(self, player, opponent):
        """
        Check if the player wins or loses.
        :param player: Numeric representation of player choice from self.options
        :param opponent: Numeric representation of computer choice from self.options
        :return: Nothing, but will print whether win or lose.
        """

        result = (player - opponent) % 3
        if result == 0:
            self.ties += 1
            print("The game is a tie! You are a most worthy opponent!")
        elif result == 1:
            self.wins += 1
            print("You win! My honor demands a rematch!")
        elif result == 2:
            self.losses += 1
            print("Haha, I am victorious! Dare you challenge me again?")

    def print_score(self):
        """
        Prints a string reflecting the current player score.
        :return: Nothing, just prints current score.
        """
        print(f"You have {self.wins} wins, {self.losses} losses, and "
              f"{self.ties} ties.")

    def run_game(self):
        """
        Plays a round of Rock-Paper-Scissors with the computer.
        :return: Nothing
        """
        while True:
            userchoice = input("Choices are 'rock', 'paper', or 'scissors'.\n"
                               "Which do you choose? ").lower()
            if userchoice not in self.options.keys():
                print("Invalid input, try again!")
            else:
                break
        opponent_choice = self.random_choice()
        print(f"You've picked {userchoice}, and I picked {opponent_choice}.")
        self.check_win(self.options[userchoice], self.options[opponent_choice])


if __name__ == "__main__":
    # Initialize an instance of RockPaperScissors for us to refer to
    game = RockPaperScissors()
    # Keep playing the came repeatedly, stop playing by just exiting
    # the entire program directly.
    while True:
        game.run_game()  # Run a round of RPS
        game.print_score()  # Print the score(s) after the round

        # Find out if we want to continue playing or not.
        while True:
            continue_prompt = input('\nDo you wish to play again? (y/n): ').lower()
            if continue_prompt == 'n':
                # Exit the game directly after printing a response.
                print("You are weak!")
                exit()
            elif continue_prompt == 'y':
                # Break the continue prompt loop and keep playing.
                break
            else:
                # Bad input was given, re-request if we want to play again.
                print("Invalid input!\n")
                continue

Now, this code has absolutely no explanation of what each function does, etc. per line of code within the class (though I provide docstrings to explain things!), even though I comment what we do in the outer block that actually runs the code.

THIS BEING SAID, I have a version of this that has much more thorough comments throughout the entire codebase (including docstrings)

A complete explanation of the code and what each bit does is detailed in a GitHub GIST located here as the rps.py file in the Gist because the number of lines doubles when you include all my comments.

(This also has a request for a review of this rewrite at Python Rock-Paper-Scissors via a class to handle the game if you want to see people review it! I'll also provide the polished version later in a separate gist!)

\$\endgroup\$
4
\$\begingroup\$

Minor improvements to documentation and structure but otherwise no significant improvements. I am sure there are optimization suggestions to be made but this seems small enough a program so as to not really benefit too much from these suggestions. If you want to be pedantic you could use type checking by importing the typing module.

Suggestion 1

Keep two lines between dependencies (import) and the rest of the code like this:

import random


options = ['rock', 'paper', 'scissors']

as well as between methods.

    # ...
    result = (a - b) % 3 #modmath
    return result
# space 1
# space 2
def finish(z): # report the results
    global wins
    # ...

For reference on stylist things such as this check out https://www.python.org/dev/peps/pep-0008/#imports

Suggestion 2

You can use docs strings to describe a function or method instead of lone comments. Doc strings are for "documentation" and help people better understand your methods or blocks of code. The # comments are for commenting on a single or several lines of code and understanding each's specific functionality, say in context of the method as a whole.

Example:

def combatCheck(a, b):
'''Determines combat choice and returns the result'''
    a = convertible(a)
    b = convertible(b)
    result = (a - b) % 3 #modmath # <-- this comment "modmath" is not too helpful 
    return result

Suggestion 3

You can use type checking to show people what type of data goes into your methods.

For this include:

import typing

Examples:

# the line below tells people the param and return types, and doesn't
# affect how the code runs
def combatCheck(a: str, b: str) -> int: 
    a = convertible(a)
    b = convertible(b)
    result = (a - b) % 3 #modmath
    return result

Since finish just prints a statement we type check it as such

def finish(z: int) -> None: 
'''Method to print results to user'''
    global wins
    global losses
    global ties
# etc...
\$\endgroup\$
  • \$\begingroup\$ I'll check out typing. I'm not familiar with it but it seems helpful. \$\endgroup\$ – ChaosHat Nov 1 at 21:00

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.