5
\$\begingroup\$

I'm very new to coding and decided to try a mini-project in python to create a 'guess the number game'. I was just hoping for some feedback on my code-writing to prevent bad habits before they develop. This program runs smoothly but I would like to know if there are improvements I could make. In particular:

  • How can I improve the efficiency and readability?
  • Is it okay to call a function within its own body?
  • Is there any ugly implementation that I should avoid?
import random

number = 0
counter = 0

def start_game():
    print("I'm thinking of a number between 1 and 10.")
    print("Can you guess it?")
    global number
    number = random.randint(1,10)
    global counter
    counter = 0
    check_valid()


def check_valid():
    global counter
    counter += 1
    guess = input()
    try:
        val = int(guess)
        if int(guess) not in range(0,11):
            print("Hmmm.. that number is not between 1 and 10! Try again!")
            check_valid()
        elif int(guess) > number:
            print("Too high, try a smaller number")
            check_valid()
        elif int(guess) < number:
            print("Too low, try a bigger number")
            check_valid()
        elif int(guess) == number:
            print("Congratulations, you guessed it! The number was " + 
                  str(number) + ".\nIt took you " + str(counter) + " tries!")
            print("Do you want to play again?")
            check_replay()
    except ValueError:
        print("That's not a number, try again!")
        check_valid()


def check_replay():
    answer = input()
    valid_yes = ["yes", "ye", "y"]
    valid_no = ["no", "n"]
    if answer.lower() in valid_yes:
        start_game()
    elif answer.lower() in valid_no:
        print("Thanks for playing!")
    else:
        print("Please enter yes or no")
        check_replay()


start_game()
\$\endgroup\$
3
\$\begingroup\$

First, check_replay has a few notable things.

  • Since you're only using valid_yes and valid_no to check for membership, they would be better as sets which have a much more efficient membership lookup. The performance difference won't be noticeable in this case, but it's a good thing to keep in mind.

  • The whole purpose of check_replay is to ask if they want to play again, yet you're printing the "Do you want to play again?" message outside of the function. I would just pass it into the initial input call.

  • Even as someone who loves recursion, I agree with @abendrot that recursion isn't the ideal tool for the job here. Using it here means your program could crash if the user enters bad input too many times. I'd just use a while loop.

  • I also think it should return the decision instead of calling start_game

Taking all that into consideration, I'd write it closer to:

def check_replay():
    valid_yes = {"yes", "ye", "y"} # Using sets instead of lists
    valid_no = {"no", "n"}

    while True:
        answer = input("Do you want to play again?: ") # Printing the message here instead

        if answer.lower() in valid_yes:
            return True
                                          # I added blank lines for readability
        elif answer.lower() in valid_no:
            print("Thanks for playing!")
            return False

        else:
            print("Please enter yes or no")

My other concern is that start_game and check_valid don't really make sense as two different functions. start_game is basically just being used to initialize the globals, but the globals aren't necessary in the first place. Normally I'm all for breaking up functions into smaller pieces, but I think here everything works better if you collapse them into one function. I also neatened up a lot of stuff. See the comments:

def play_game():
    # I wrapped the whole thing in a loop to avoid the recursive call
    while True: 
        print("I'm thinking of a number between 1 and 10.")
        print("Can you guess it?")

        # No more globals 
        number = random.randint(1, 10)
        counter = 0

        while True:
            counter += 1
            guess = input("Your guess: ")

            try:
                val = int(guess) # You forgot to use val and were instead writing int(guess) all over

                if val not in set(range(0, 11)): # Made into a set as well
                    print("Hmmm.. that number is not between 1 and 10! Try again!")
                                                               # Again, I added blank lines for readability
                elif val > number:
                    print("Too high, try a smaller number")

                elif val < number:
                    print("Too low, try a bigger number")

                else: # This should just be an else since if the other two checks failed, they must be equal 
                    print("Congratulations, you guessed it! The number was " + 
                          str(number) + ".\nIt took you " + str(counter) + " tries!")

                    if check_replay():
                        break # Break to the outer loop to play again

                    else:
                        return # Else exit

            except ValueError:
                print("That's not a number, try again!")

If you wanted to break that large function up (which is understandable), I'd factor out the turn-taking loop aspect of it. Something like:

def make_guess(computer_number): # Pass in the target number
    while True:
        guess = input("Your guess: ")

        try:
            val = int(guess)

            if val not in set(range(0, 11)):
                print("Hmmm.. that number is not between 1 and 10! Try again!")

            elif val > computer_number:
                print("Too high, try a smaller number")

            elif val < computer_number:
                print("Too low, try a bigger number")

            else:
                return True # Tell the caller that the player won

            return False # Else return that they haven't won yet

        except ValueError:
            print("That's not a number, try again!")

def play_game():
    while True: 
        print("I'm thinking of a number between 1 and 10.")
        print("Can you guess it?")

        number = random.randint(1, 10)
        counter = 0

        while True:
            counter += 1

            has_won = make_guess(number)

            if has_won:
                print("Congratulations, you guessed it! The number was " + 
                      str(number) + ".\nIt took you " + str(counter) + " tries!")

                if check_replay():
                    break

                else:
                    return

Normally I don't like using while True, but to avoid it here you'd need to use flags all over instead which I don't think would help readability.

Altogether, I have:

import random

def make_guess(computer_number): # Pass in the target number
    while True:
        guess = input("Your guess: ")

        try:
            val = int(guess)

            if val not in set(range(0, 11)):
                print("Hmmm.. that number is not between 1 and 10! Try again!")

            elif val > computer_number:
                print("Too high, try a smaller number")

            elif val < computer_number:
                print("Too low, try a bigger number")

            else:
                return True # Tell the caller that the player won

            return False # Else return that they haven't won yet

        except ValueError:
            print("That's not a number, try again!")

def play_game():
    while True: 
        print("I'm thinking of a number between 1 and 10.")
        print("Can you guess it?")

        number = random.randint(1, 10)
        counter = 0

        while True:
            counter += 1

            has_won = make_guess(number)

            if has_won:
                print("Congratulations, you guessed it! The number was " + 
                      str(number) + ".\nIt took you " + str(counter) + " tries!")

                if check_replay():
                    break

                else:
                    return

def check_replay():
    valid_yes = {"yes", "ye", "y"} # Using sets instead of lists
    valid_no = {"no", "n"}

    while True:
        answer = input("Do you want to play again?: ") # Printing the message here instead

        if answer.lower() in valid_yes:
            return True
                                          # I added blank lines for readability
        elif answer.lower() in valid_no:
            print("Thanks for playing!")
            return False

        else:
            print("Please enter yes or no")

play_game()
\$\endgroup\$
  • \$\begingroup\$ Wow I wasn't expecting so much detail that's awesome thanks so much! I think a big problem I had was not really understanding how the while True loop would work, but you've helped a lot! Also some really good advice about code structure!! Thanks a million for the help! \$\endgroup\$ – Powdie May 6 '19 at 14:46
  • \$\begingroup\$ @Powdie Ya, no problem. I enjoy writing up breakdowns like this. \$\endgroup\$ – Carcigenicate May 6 '19 at 14:53
4
\$\begingroup\$

One thing I notice is that, you use global variables. While they are not necessarily bad and work fine in this particular case, it might be beneficial to read these answers. It would be better to define these variables in main function and pass them as parameters for other functions.

def start_game(number, counter):
  #Your code here

def main():
  number = 0
  counter = 0
  start_game(number, counter)

if __name__ == "__main__":
    main()

Calling the function within its body is called a recursion and as long as you assure that the calling sequence won't become an infinite loop, it is widely used technique in programming.

One more minor thing: you define val = int(guess) but never use it, maybe delete it?

Other than that, everything looks fine and the code is readable.

\$\endgroup\$
  • \$\begingroup\$ Thanks so much for your help! I think I had intended to use val instead of int(guess) but forgot to change it! I did try to pass those variables as parameters but couldn't get the hang of it! Thanks for the link there's a lot of helpful info there! Thanks for taking the time to help me out! Much appreciated! \$\endgroup\$ – Powdie May 6 '19 at 14:34

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.