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()
2 Answers 2
First, check_replay
has a few notable things.
Since you're only using
valid_yes
andvalid_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 initialinput
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()
-
\$\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– Powdie2019年05月06日 14:46:51 +00:00Commented May 6, 2019 at 14:46
-
\$\begingroup\$ @Powdie Ya, no problem. I enjoy writing up breakdowns like this. \$\endgroup\$Carcigenicate– Carcigenicate2019年05月06日 14:53:56 +00:00Commented May 6, 2019 at 14:53
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.
-
\$\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– Powdie2019年05月06日 14:34:14 +00:00Commented May 6, 2019 at 14:34
Explore related questions
See similar questions with these tags.