10
\$\begingroup\$

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()
\$\endgroup\$
2
  • 4
    \$\begingroup\$ "number is higher" and "number is lower" are ambiguous. I interpreted them to mean that the real number is higher/lower. Since you're actually referring to the guessed number, I'd use something like like "your guess is too high" and "your guess is too low" \$\endgroup\$
    – tdy
    Commented Jul 22 at 19:22
  • 1
    \$\begingroup\$ Also instead of hardcoding the real answer, consider generating a random one, e.g. realno = random.randint(1, 10) \$\endgroup\$
    – tdy
    Commented Jul 22 at 19:27

5 Answers 5

8
\$\begingroup\$

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.

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

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 name update. Then your challenge is to decide how that function notifies the loop that it needs to exit, as variable done 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:
\$\endgroup\$
3
\$\begingroup\$

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 and continue 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: your quit 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 omit return entirely.
  • str.lower(input(...)) is something strange: input is a str, references to unbound methods are common with functional programming approach (passing functions as variables is sometimes convenient as well), but not for direct application. Consider input(...).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 use import sys and sys.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()
\$\endgroup\$
3
\$\begingroup\$

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.

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

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 
\$\endgroup\$
6
  • 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\$ Commented Jul 23 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
    Commented Jul 23 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\$ Commented Jul 23 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
    Commented Jul 23 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
    Commented Jul 24 at 15:28

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.