This is just something I did in my free time. How can I improve my code as a beginner?
- Is there an alternative to using
isalpha()
- What are other efficient ways to implement this program
- Suggestions for improving code structure
# The guess game
def guess_game():
done = False
quit = False
type_check = False
while not done:
num = input("enter num b/w 1 and 10: ")
quit = False
if num.isalpha():
if num == "quit":
while not quit:
quit_conf = str.lower(input("are you sure you want to quit - Y/n ?: "))
if quit_conf == "y":
exit()
elif quit_conf == "n":
quit = True
else:
print("invalid input")
else:
print("Invalid input")
type_check = False
elif num.isnumeric():
type_check = True
int_num = int(num)
else:
print("Invalid value")
type_check = False
# this is the answer
realno = 3
if type_check:
# checking if in range
if int_num < 1 or int_num > 10:
print("number out of range")
# checking num
elif int_num > realno:
print("number is higher")
elif int_num < realno:
print("number is lower")
else:
print("you win!")
done = True
return 0
guess_game()
5 Answers 5
Instructions
You should provide explicit instructions to the user when the code is run. For example, the code allows the user to quit, but there is no way for the user to know that without reading the code. Add it to the initial prompt message:
num = input("guess a number between 1 and 10 (or 'quit' to quit):")
Status message
The status message is not clear to me when running the game. For example, I see:
number is higher
It is not clear to me if my last guess is higher than the number I am trying to guess
or if my last guess is lower. Also, using the word number
is ambiguous. Using the
word guess
is clearer:
if int_num < 1 or int_num > 10:
print("guess out of range")
elif int_num > realno:
print("guess is too high")
Comments
The following comments are unnecessary because it is obvious what the code is doing:
# checking if in range
if int_num < 1 or int_num > 10:
print("number out of range")
# checking num
The comments can be deleted.
Naming
The variable num
is not very descriptive. guess
would be a better name.
Similarly, realno
might be better as answer
.
Hard-coded
The game would be much more interesting if you let the code choose a random answer
instead of hard-coding the answer as 3
.
Documentation
You could add a doctring to the top of the code to describe its purpose:
"""
The guess game
Guess a number between 1 and 10.
"""
The other review is kind of OK but is focusing on the little things instead of the things that would make you a "real" programmer. It all depends on the perspective you take on that program :
- Since you program in Python you probably have an implicit sense that it should be a small, self-contained script. That's OK but I'm guessing that's not how you're trying to improve.
- However, if you imagine for a second that this is just a "practice run" for a "real" program -- a program which would, let's say, be accepted in the corporate world -- then it's missing two serious things :
1) Break it down
By doing so, you are doing two different things:
- For the READER: it makes the flow easy to follow. I'm not just saying that the program is easier to "understand". I'm saying that the FLOW is more straightforward, as the reader knows at all times what exactly comes in and out of subparts.
- For the PROGRAM : avoid side effects, which means you literally FORBID unwanted parts of the program to mess with other wanted parts and with the global state (which you're aiming at removing entirely, ideally).
It's not just about separating bits of code. It's to stay in control of the flow. Right now, as one big function, everything has access to everything. You need to break it down into smaller functions that can change only what they need to change, nothing more. That's what we call leaving no room for side effects.
The most obvious break down is to move the entire code inside of the
while
to its own separate function, which you could for example nameupdate
. Then your challenge is to decide how that function notifies theloop
that it needs to exit, as variabledone
is now inside the function. That's for you to solve.Another bit of code that you can move to a separate function is the loop asking "are you sure you want to exit".
etc.
2) Separate the infrastructure from the business logic
I used big words to make you think ;-)
You need to imagine that you can't know in advance what underlying subsystem will render the output. Maybe it will be text in a console, maybe it will have graphics, etc. Even when you're 100% sure that the output goes to the terminal, in effect you still don't know anything. Maybe it will be handled by a third-party library. Maybe there is some hidden mechanism to log stuff to a log file. Etc.
Therefore, you need to separate the input/output from the rest. To achieve that, maybe you need to have a tiny library that contains : a wrapper around input
, a wrapper around print
, a wrapper around isnumeric
and isalpha
. Then you need to throw in the parsing of the input : A function isQuit
, isY
, isN
. All of that is good practice because maybe in the future another dev will revisit those methods and make them handle upper case and lower case (that's just an example).
On the other hand, the "business logic" is everything that handles the state of the game in a "perfect world" where you don't have to worry about I/O or that kind of things. In your game the business logic is at least a function "has_won". And its siblings: is_bigger
, is_smaller
. Yes, as overkill as it sounds, if int_num == realno:
is the core of your business logic and must be put on some kind of pedestal, isolated from the rest, so that any reader can tell :"Aha, that's the important part that should never be broken by my changes".
3) Exit the application cleanly!
Remember what I said about staying in control of the flow? Well right now there are two entirely separate flows for exiting the application :
- by setting "done" to true, which exits the loop
- by calling "exit()" right in the middle of the loop.
That's not very good for the reader. Plus, if you want to add a goodbye message then you'll have to add it in both places.
A single flow!
Instead I would add quit
to the exit condition of the loop :
while not done and not quit:
I'll try to lay out this answer as a series of potential improvements, first showing the specific defects of code you show and then following with what I'd have done myself.
Initial
- Indentation: you use 4 spaces everywhere, but the main function is indented 8. Be consistent, and 4 spaces is the recommended option.
- Boolean flags: usually
break
andcontinue
are better for flow control than a bunch of boolean variables. You might have not learned about them yet - that's fine for first program. At least consider scoping them: yourquit
flag only belongs to the "should quit?" loop, don't define it at the very top - "isalpha"/"isnumeric"/... - logic bug? You are only interested in two cases: "number", "quit" and "anything else". Let's be explicit about that:
if guess == "quit": ... # Quit confirmation elif guess.isnumeric(): ... # Something with number else: ... # Unexpected input
- Needless return: python is not C. No need to return 0 from something like a
main
function. Just omitreturn
entirely. str.lower(input(...))
is something strange:input
is astr
, references to unbound methods are common with functional programming approach (passing functions as variables is sometimes convenient as well), but not for direct application. Considerinput(...).lower()
instead.__main__
guard. You may not be thinking about this yet, but at larger scales your code will be imported into other files. For example, for unit tests: you'll have another file that tests the behaviour of your code. You do not want to start a game in such case, right? Check out this Q&A for details.exit
- that only works from interactive console. Either useimport sys
andsys.exit()
or just exit the loop.
So, here's what a first iteration with some of these adjustments may look like (I kept names the same to make modifications easier to follow):
def guess_game():
# this is the answer
realno = 3
done = False
while not done:
num = input("enter num b/w 1 and 10: ")
if num == "quit":
quit = False
while not quit:
quit_conf = input("are you sure you want to quit - Y/n? ").lower()
if quit_conf == "y":
done = True
elif quit_conf == "n":
quit = True
else:
print("invalid input")
elif num.isnumeric():
int_num = int(num)
if int_num < 1 or int_num > 10:
print("number out of range")
elif int_num > realno:
print("number is higher")
elif int_num < realno:
print("number is lower")
else:
print("you win!")
done = True
else:
print("Invalid value")
if __name__ == "__main__":
guess_game()
Now, let's get rid of boolean flags. done
is a flag that controls the outer loop - let's use while True
and return
from the function when we're done. quit
works similarly for inner loop - we can exit it with break
.
def guess_game():
# this is the answer
realno = 3
while True:
num = input("enter num b/w 1 and 10: ")
if num == "quit":
while True:
quit_conf = input("are you sure you want to quit - Y/n? ").lower()
if quit_conf == "y":
return
elif quit_conf == "n":
break
else:
print("invalid input")
elif num.isnumeric():
int_num = int(num)
if int_num < 1 or int_num > 10:
print("number out of range")
elif int_num > realno:
print("number is higher")
elif int_num < realno:
print("number is lower")
else:
print("you win!")
return
else:
print("Invalid value")
Now, let's extract a helper function with a nested loop - exit confirmation.
def guess_game():
# this is the answer
realno = 3
while True:
num = input("enter num b/w 1 and 10: ")
if num == "quit" and confirm_quit():
return
elif num.isnumeric():
int_num = int(num)
if int_num < 1 or int_num > 10:
print("number out of range")
elif int_num > realno:
print("number is higher")
elif int_num < realno:
print("number is lower")
else:
print("you win!")
return
else:
print("Invalid value")
def confirm_quit():
while True:
quit_conf = input("are you sure you want to quit - Y/n? ").lower()
if quit_conf == "y":
return True
elif quit_conf == "n":
return False
else:
print("invalid input")
It may be also wise to extract the block in elif num.isnumeric()
- it contains important business logic which should be testable separately.
Usability
- As already pointed out, the outcome strings in your code are ambiguous, and the instruction could be improved.
- Confirmation: most CLI utilities will only ask for confirmation once, interpreting "y" as confirmation and anything else as rejection. That'd be the route of least surprise for your users familiar with UNIX CLI tools. Also, the upper-case option in choices list usually represents the default. "Should we quit?" choice is not safety-critical, so you can assume "yes" - just handle empty input the same way as you would "y".
- Hardcoded answer can be replaced with random choice to make the game more interesting:)
EAFP
EAFP (Easier to ask for forgiveness than permission) is a very common approach in Python. Instead of asking for .isnumeric
, you can just try to parse the string as an integer and report a failure.
Logic/presentation separation
It's a great idea to have presentation (what your users see) separated from the logic (how does the thing work) whenever possible.
Since your scenario is kind of trivial, you can use a Enum to denote message constants: this way your logic operates in terms of business role, and print
produces relevant user-friendly message strings.
Here's how I would approach this problem from scratch in a production setting:
from enum import StrEnum
from random import randint
RANGE_START = 1
RANGE_END = 10
ANSWER = randint(RANGE_START, RANGE_END)
class GuessOutcome(StrEnum):
CORRECT = "Correct!"
TOO_LOW = "Your guess is too low."
TOO_HIGH = "Your guess is too high."
OUT_OF_RANGE = "Number out of range."
QUIT = "Bye-bye!"
ABORT_QUIT = "Not quitting..."
UNRECOGNIZED = "Input not recognized, please try again."
def check_guess(guess: str) -> GuessOutcome:
if guess == "quit":
return maybe_quit()
try:
guessed_number = int(guess)
except ValueError:
return GuessOutcome.UNRECOGNIZED
if guessed_number == ANSWER:
return GuessOutcome.CORRECT
elif not RANGE_START <= guessed_number <= RANGE_END:
return GuessOutcome.OUT_OF_RANGE
elif guessed_number < ANSWER:
return GuessOutcome.TOO_LOW
else:
return GuessOutcome.TOO_HIGH
def maybe_quit() -> GuessOutcome:
quit_conf = input("Are you sure you want to quit - Y/n? ").lower()
if not quit_conf or quit_conf == "y":
return GuessOutcome.QUIT
elif quit_conf == "n":
return GuessOutcome.ABORT_QUIT
else:
return GuessOutcome.UNRECOGNIZED
def guess_game() -> None:
print(
"Welcome to the guessing game! Guess numbers between 1 and 10,"
" or type 'quit' to quit"
)
while True:
guess = input("Enter a number between 1 and 10, or 'quit': ")
result = check_guess(guess)
print(result)
if result in {GuessOutcome.CORRECT, GuessOutcome.QUIT}:
break
if __name__ == "__main__":
guess_game()
The bulk of your code deals with validating the input. My suggestion is to factor them out to separate functions, making the code cleaner. Also, it helps to write down what kind of input is acceptable, which is not:
Input | Valid? | Return | Error Message |
---|---|---|---|
1-10 | Yes | The number | None |
quit | Yes | 'quit' | None |
Out-of-range int | No | No return, keep looping | Out of range |
Other | No | No return, keep looping | Not an integer |
So, if you have a function which only accept the first two rows of input and reject the rest, your code will be much simpler. Here is that function:
def ask() -> str | int:
"""Ask and accept only number 1..10 and 'quit'."""
while True:
guess = input("Enter a number between 1 and 10, or type quit: ")
if guess == "quit":
return guess
try:
guess = int(guess)
if 1 <= guess <= 10:
return guess
print("Out of range")
except ValueError:
print("Not an integer")
Not only this function makes your code easier, it help eliminating the type_check
variable.
Cyclomatic code complexity
You code contains one function and within that lots of while, if/else
, and with flags that control code logic.
This is not really conducive to maintainability and understanding. If you use the Python package radon
on your code it will give you a rank of "C" which is a "slightly complex block". This is not bad and I, personally, have had worse but it is nice to avoid it here, for such a simple program.
$ radon cc your_program.py
F 3:0 guess_game - C
I think this is a nice example where recursion might be useful.
Restructring with recursion
When I generally see infinite while
statements with exit clauses I generally prefer to restructure it with recursion. This might not always be practical but I think it suits your situation. The 'game' will simply restart with a new call and does not require any of the flags you use.
ANSWER = 3
def guess_game():
guess = input("Enter a number between 1 and 10 (inclusive): ")
maybe_quit(guess)
valid, out = validate_guess(guess)
if not valid:
print(out)
return guess_game()
if out == ANSWER:
print("You win")
return 0
elif out > ANSWER:
print("Guess is too high, try again..")
return guess_game()
else:
print("Guess is too low, try again..")
def maybe_quit(guess):
if guess.isalpha() and guess == "quit":
quit_conf = str.lower(input("are you sure you want to quit - Y/n ?: "))
if quit_conf == "y":
exit()
elif quit_conf == "n":
print("Exiting.")
else:
print("Invalid input")
def validate_guess(guess):
if guess.isalpha():
return False, "Invalid input: guess is not a number"
if guess.isnumeric():
guess = int(guess)
if guess < 1 or guess > 10:
return False, "Invalid input: number out of range"
return True, guess
return False, "Invalid input: is not recognised."
guess_game()
If you want to change your validation principles or your exiting procedure you now have simple functions within which to do so, and not change the body of the core guess_game
function.
And note that the CC of my functions all achieve an A grade (low risk -simple blocks).
$ radon cc my_program.py
F 21:0 maybe_quit - A
F 31:0 validate_guess - A
F 4:0 guess_game - A
-
2\$\begingroup\$ My personal opinion is that recursion is a terrible idea here. You have a simple game with conceptually an infinite number of trials, loop is the way to go. Not to mention, it's simpler to read and maintain. \$\endgroup\$jeancallisti– jeancallisti2024年07月23日 09:44:17 +00:00Commented Jul 23, 2024 at 9:44
-
\$\begingroup\$ I don't really share the view anyone that wants to play a game guessing a number between 1 and 10 with an infinite number of trials. And simplicity to read and maintain is obviously subjective. Whilst I don't share your perspective, I do appreciate it because it is helpful to highlight these differences. Thank you @jeancallisti \$\endgroup\$Attack68– Attack682024年07月23日 10:35:34 +00:00Commented Jul 23, 2024 at 10:35
-
\$\begingroup\$ I should add that I find the other suggestions of @Attack68 perfectly valid (reduce code complexity + smaller methods such as maybe_quit and validate_guess). He showed you a good implementation of what I was suggesting in my own answer. \$\endgroup\$jeancallisti– jeancallisti2024年07月23日 11:52:42 +00:00Commented Jul 23, 2024 at 11:52
-
\$\begingroup\$ My professional opinion is that recursion is a terrible idea here. Recursion instead of looping is a common beginner mistake. Beginners see it as a cheat way of using
GOTO
instead of restructuring their code more cleanly and using loops and break/return. You should not be teaching this strategy to beginners. It has implications that beginners do not understand, and it is simply the wrong tool for the job. \$\endgroup\$Blorgbeard– Blorgbeard2024年07月23日 17:36:45 +00:00Commented Jul 23, 2024 at 17:36 -
1\$\begingroup\$ @STerliakov this was a restructure attempting to capture the same data model/logic as used by the OP's question. While valid points, these concerns were out of scope in this answer, but I am sure the OP would benefit with your own answer contribution addressing these issues in his original code. Thanks for the comment. \$\endgroup\$Attack68– Attack682024年07月24日 15:28:41 +00:00Commented Jul 24, 2024 at 15:28
Explore related questions
See similar questions with these tags.
realno = random.randint(1, 10)
\$\endgroup\$