13
\$\begingroup\$

I think I finally finished a small coin flip project I found online. However, I'm pretty confident this is far from the optimal solution and would really appreciate any feedback on how this could be improved. Since I'm studying by myself using Head First and Code Academy any feedback would be very very helpful!

The goals of the challenge are:

  • As a user I want to be able to guess the outcome of a random coin flip (heads/tails).
  • As a user I want to clearly see the result of the coin flip.
  • As a user I want to clearly see whether or not I guessed correctly.
  • As a user I want to clearly see the updated guess history (correct count/total count).
  • As a user I want to be able to quit the game or go again after each cycle.

This is my first time really using GitHub, but I included the .py file which hopefully you can access here.

import random


def random_flip(): #function to generate random Heads or Tails output

    random_choice = random.choice(["Heads","Tails"])
    return random_choice

def user_choice(): #function to ensure a valid input from the user, and returns that input.

    user_input = ""
    valid_guesses = ["Heads", "Tails"]

    while user_input not in valid_guesses:
        user_input = input("Heads or Tails?")

    return user_input


def coin_flip_game(): #function that plays the coin flip game.

    guess_count = 1  #guesses set to 1 so that program considers 1st guess.
    guesses = []     #list of guesses made by user
    winner = False
    first_try = True      #Variable to determine whether first try of user or not
    cancel_game = False   #Variable to determine whether game was cancelled

    print("Lets start! Begin by choosing:")

    while winner == False:
        if first_try == True:
            user_input = user_choice()

            if user_input != random_flip():
                guess_count += 1
                guesses.append(user_input)
                first_try = False
            else:
                winner = True
        else:
            decision = input("You guessed wrong, do you want to continue? Yes or No ")

            if decision == "Yes":
                user_input = user_choice()

                if user_input != random_flip():
                    guess_count += 1
                    guesses.append(user_input)
                    first_try = False
                else:
                    winner = True
            else:
                winner = True
                cancel_game = True
                break

    if cancel_game == True:
        print("Game over")
    elif guess_count == 1:
        print("Congratulations, you guessed correctly on your first try!")
    else:
        print("Congratulations! After ", guess_count, "you guessed correctly!")
        print("Your previous guesses were: ", guesses)

coin_flip_game()
\$\endgroup\$

2 Answers 2

11
\$\begingroup\$

After getting user input, you check it by doing decision == "Yes". This is pretty exact text to expect from the user. I'd at least upper-case their input, and unless you really needed them to be specific, only keep the first letter:

decision = input("You guessed wrong, do you want to continue? Yes or No ")
std_decision = decision[:1].upper()  # [:1] discards all but the first letter

if std_decision == "Y":
   . . .

In coin_flip_game you have

guess_count = 1  #guesses set to 1 so that program considers 1st guess

Then later:

if user_input != random_flip():
    guess_count += 1
    guesses.append(user_input)
    first_try = False
else:
    winner = True

You're only increasing guess_count if the guess was wrong. Regardless of if they were right or wrong though, they still made a guess. I think it makes more sense to add to the guess_count regardless, and start it at 0.

But then you're also only adding to guesses on wrong answers too. If you change it to append to guesses on both right and wrong answers, you'll have this:

guess_count = 0

. . .

guess_count += 1
guesses.append(user_input)

if user_input != random_flip():
    first_try = False

else:
    winner = True

Notice now though that guess_count will always be equal to the length of guesses, so it isn't needed. If you need to find out how many guesses were made, you can just do len(guesses). first_try also isn't necessary. To check if it was their first guess or not, you can do:

if len(guesses) == 0:
    . . .

Or, more idiomatically:

if not guesses:  # "If no(t) guesses (have been made)"
    . . .

I think that whole game loop part should be taken out into it's own function though; beyond what you've already done. I'd have a function that returns a list of guesses made until a correct guess was made. With how you have it set up now, I'm going to disregard cancelling since that will require some broader changes to accommodate, and it's highly unlikely that the user will have to guess more than a few times.

def guess_loop() -> List[str]:
    guesses = []

    while True:  # We're just going to return, so we don't need a condition
        user_input = user_choice()
        guesses.append(user_input)

        if user_input == random_flip():
            return guesses  # Returning guesses means we won

        else:
            print("You guessed wrong. Try again.")

def coin_flip_game():
    print("Lets start! Begin by choosing:")
    guesses = guess_loop()

    if len(guesses) == 1:
        print("Congratulations, you guessed correctly on your first try!")

    else:
        print("Congratulations! After", len(guesses), "you guessed correctly!")
        print("Your guesses were: ", guesses)

The bulky bit was taken out so it can be dealt with as an isolated bit of code. To know the outcome of the game, all I care about (besides cancelling, which could be handled by returning None) is the list of guesses they made until they won. I can get all the information I need from just that. Needing to mix in winner, and cancel flags can suggest that that code should be moved out. By the time you're managing two or three flags to control execution in one block of code, it can become hard to follow.


user_choice can be cleaned up too. Again, you're using a condition to control a loop when I think returning directly from the loop leads to cleaner looking code:

def user_choice():
    while True:
        user_input = input("Heads or Tails?").capitalize()  # Capitalize to give the user some leeway

        if user_input in {"Heads", "Tails"}:
            return user_input

        else:
            print("Invalid input.")

After the above and some small formatting touchups, I'm left with:

import random
from typing import List


def random_flip() -> str:
    return random.choice(["Heads", "Tails"])


def user_choice() -> str:
    while True:
        user_input = input("Heads or Tails?").capitalize()  # Capitalize to give the user some leeway

        if user_input in {"Heads", "Tails"}:
            return user_input

        else:
            print("Invalid input.")


def guess_loop() -> List[str]:
    guesses = []

    while True:  # We're just going to return, so we don't need a condition
        user_input = user_choice()
        guesses.append(user_input)

        if user_input == random_flip():
            return guesses  # Returning guesses means we won

        else:
            print("You guessed wrong. Try again.")


def coin_flip_game() -> None:
    print("Lets start! Begin by choosing:")
    guesses = guess_loop()

    if len(guesses) == 1:
        print("Congratulations, you guessed correctly on your first try!")

    else:
        print("Congratulations! After", len(guesses), "you guessed correctly!")
        print("Your guesses were: ", guesses)

coin_flip_game()

The -> bits are type hints for the return type of the function. I think they're handy to have to make it clearer what type of data is being dealt with.

\$\endgroup\$
11
  • 2
    \$\begingroup\$ I would just let the User Enter "Head, Tails or Cancel" and exit the game on cancel. The user should be able to exit the game at any time and not keep guessing in the hope the game will then (maybe?) end. \$\endgroup\$
    – Falco
    Dec 30, 2019 at 9:42
  • \$\begingroup\$ @Falco That was my original idea. But then I would have probably scraped/completely rewritten user_choice because "cancel" is probably a long enough word that I'd want the user to be able to enter an abbreviated version of the input, and at the time, for whatever reason, that seemed to be beyond the scope that I had arbitrarily decided on. \$\endgroup\$ Dec 30, 2019 at 12:15
  • 1
    \$\begingroup\$ @DanielCastro And regarding while True, it isn't testing for anything. It will run forever if I allow it to. Note though how I'm returning in the loop. You're right that that loop would run forever if I wasn't returning. return though will cause the function to exit regardless of where it is in the function. As soon as you reach a return, the function stops and returns; even if it's in a infinite loop. You may be used to only exiting loops via the condition. If that's the case, I'd practice using break and return in loops to get a better feel for what you can do with them. \$\endgroup\$ Dec 30, 2019 at 19:40
  • 2
    \$\begingroup\$ Great review as always. But is there a rationale behind detaching elses from their corresponding if blocks? I think this has a substantial negative impact on readability. \$\endgroup\$
    – ggorlen
    Dec 30, 2019 at 22:35
  • 1
    \$\begingroup\$ @ggorlen Hmm, I can see the want to have if more tightly associated with else. Maybe I'll play around with that style and see how I find it. Really, this style is a carry over from Clojure, so it's pretty engrained and I've just kind of accepted it. Maybe I just need play around and question if it could be improved. That's for letting me know. I never thought that it would be considered less readable than the alternative. \$\endgroup\$ Dec 30, 2019 at 22:47
2
\$\begingroup\$

User input shouldn't require the full case-sensitive text.

If there isn't already one in a library somewhere, write a function that compares two strings to see if one is a valid truncation of the other. Then use that function rather than != for comparing the response to the flip.

E.g. any of these should match "True": "t", "tr", "tru", and "true", regardless of case.

And users shouldn't have to respond so many times. It gets annoying. As "Falco" suggested, the input choices should be "Heads", "Tails", and "Quit".

\$\endgroup\$
1
  • \$\begingroup\$ Thank you! I was wondering if there was a method available already for truncating text but writing one out makes sense. I'll definitely give this a try! \$\endgroup\$ Dec 30, 2019 at 19:34

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.