9
\$\begingroup\$

I am looking to improve the Python Number Guessing Game program:

  1. How is the overall structure of the code?

  2. Have I used functions to the best of their ability?

  3. Can the code be simplified? If yes, please feel free to show how.

  4. The replay_game() function especially seems un-pythonic and overkill.

# Guess My Number Game
# MacOSX (Latest), Python 3.4.3, 27/08/2017
# Guess The Computers number between 1-100

from random import randint
from time import sleep

def gen_number():
    random_number = randint(0, 101)
    return random_number

def intro():
    print("Welcome to, Guess My Number!")
    print("Simply guess the number between 1-100 that the Computer is 'thinking' of!")
    print()

def ask_number(question, random_number):
    response = None
    while response != random_number:
        try:
            response = int(input(question))
            if response > random_number:
                print("Lower... ")
            elif response < random_number:
                print("Higher... ")
            else:
                correct = response
                congrat_winner(correct, random_number)
        except ValueError:
                print("Invalid. Enter a number between 1-100 ")
    return response

def human_guess():
    print("Ok Human! Let's begin... ")
    sleep(1)
    random_number = gen_number() # Named the variable random_number, the same as the variable in gen_number(). Is this good practise?
    guess = ask_number("Guess the number: ", random_number)

def congrat_winner(correct, random_number):
    if correct == random_number:
        print()
        print("Calculating results...")
        sleep(1)
        print()
        print("WELL DONE!")
        print("The answer was indeed,", str(correct) + "!")

def replay_game():
    replay_question = None
    while replay_question != 'y' or 'n':
        replay_question = input("Would you like to play again (y/n)? ").lower()
        if replay_question == 'y':
            print()
            print("Rebuilding game... ")
            main()
        elif replay_question == 'n':
            print("Goodbye!")
            exit()
        else:
            print("please enter either 'y' or 'n'... ")

def main():
    intro()
    human_guess()
    replay_game()

main()
\$\endgroup\$
9
\$\begingroup\$

There is a classic problem in this condition:

while replay_question != 'y' or 'n':

It always evaluates to True since string n is always truthy.

And, here is a set of code style violations I would work on:

  • guess variable inside the human_guess() function is unused
  • keep 2 blank lines between the function definition (PEP 8 - Style Guide)
  • put the main() function call into under the if __name__ == '__main__':
  • the print("Invalid. Enter a number between 1-100 ") is overly indented with 8 spaces, use 4 spaces
  • you can use a multi-line string for your intro message:

    print("""
    Welcome to, Guess My Number!
    Simply guess the number between 1-100 that the Computer is 'thinking' of!
    """)
    
\$\endgroup\$
  • 3
    \$\begingroup\$ while replay_question not in ('y', 'n'): \$\endgroup\$ – Aemyl Aug 27 '17 at 19:54
  • \$\begingroup\$ Hi there, many thanks for your answer. Was the while loop in replay_question just comparing two string types instead of the content of the strings? \$\endgroup\$ – Greg Aug 27 '17 at 20:34
  • 1
    \$\begingroup\$ @Greg If you split up the conditional, you get replay_question != 'y' and 'n'. Since non-empty strings in Python are always truthy, this condition always evaluates to true. \$\endgroup\$ – LyricLy Aug 27 '17 at 21:26
  • \$\begingroup\$ @lyricLy Brilliant, thank you for confirming. Greg. \$\endgroup\$ – Greg Aug 27 '17 at 22:51
7
\$\begingroup\$

Just some notes:


The bug:

random_number = randint(0, 101)

returns integers from 0 to 101, including both 0 and 101. The fix:

random_number = randint(1, 100)

(but see the next note).


The magic numbers 1 and 100, moreover repeatedly used. You may define them as constants near the top of your code, e. g.

LOWER_LIMIT = 1
UPPER_LIMIT = 100

and then use them in these (inconsecutive) lines of your code:

random_number = randint(0, 101)

print("Simply guess the number between 1-100 that the Computer is 'thinking' of!")

print("Invalid. Enter a number between 1-100 ")

changing them to

random_number = randint(LOWER_LIMIT, UPPER_LIMIT)     # see fix in the first note

print("Simply guess the number between {}-{} that the Computer is 'thinking' of!"
      .format(LOWER_LIMIT, UPPER_LIMIT))

print("Invalid. Enter a number between {}-{} "
      .format(LOWER_LIMIT, UPPER_LIMIT))

( {} in strings are replacement fields for parameters in the .format() method.)

\$\endgroup\$
  • \$\begingroup\$ Wow! I haven't been exposed to this sort of formatting before in Python. I really like this. Thank you for pointing it out. In addition, it is far easier to understand when you used the constant variables. Out of interest, do you think I could have put the main() function to better use? Thank you. Greg \$\endgroup\$ – Greg Aug 27 '17 at 22:50
  • \$\begingroup\$ We still learn something new. In Python 3.6+ is even possible to write it more straighforward, without .format() method, with the f string's prefix and expressions directly in the replacement fields {}- print(f"Invalid. Enter a number between {LOWER_LIMIT}-{UPPER_LIMIT} ") \$\endgroup\$ – MarianD Aug 27 '17 at 23:54
  • \$\begingroup\$ It's definitely better to use the main() function - see the last (3rd) part of my another answer with the links for reasoning such practice. \$\endgroup\$ – MarianD Aug 28 '17 at 0:00
4
\$\begingroup\$

I would replace random_number = gen_number() with randint(0, 101).

randint is already a function. What is the use in wrapping it in another function which does nothing extra?

\$\endgroup\$
  • \$\begingroup\$ Thank you for your answer. I have edited my question to show improved code. \$\endgroup\$ – Greg Aug 28 '17 at 20:40
3
\$\begingroup\$

Your answers (Lower... or Higher... ) are ambiguous - is the answer (guess) lower or is such the unknown number?

Maybe something as "Try lower ..." would be more appropriate.


In your congrat_winner() function

def congrat_winner(correct, random_number):
    if correct == random_number:
        print()
        print("Calculating results...")
        sleep(1)
        print()
        print("WELL DONE!")
        print("The answer was indeed,", str(correct) + "!")

is the introductory testing superfluous (and - consequently - the second parameter is needless) as you call that function only after passing the similar test.

So your function may be shortened to

def congrat_winner(correct):
    print()
    print("Calculating results...")
    sleep(1)
    print()
    print("WELL DONE!")
    print("The answer was indeed,", str(correct) + "!")

Subsequently, the part

        else:
            correct = response
            congrat_winner(correct, random_number)

where you call that function have to be changed to

        else:
            congrat_winner(response)      # directly used 'response' instead of 'correct'

(There is no more reason for the correct = response statement.)

\$\endgroup\$
  • \$\begingroup\$ Excellent. Thank you for your feedback. Yes, the if statement in congrat_winner() was useless. Would it be simpler to call the congrat_winner function through human_guess by using the guess variable? Thank you. \$\endgroup\$ – Greg Aug 27 '17 at 22:46
  • \$\begingroup\$ No, it isn't possible as guess is only a local variable of the human_guess, btw. never used so you may simply omit it. To be clear I am going to write another answer about it. \$\endgroup\$ – MarianD Aug 27 '17 at 23:23
3
\$\begingroup\$

In the loop where you take input you have the following statement:

    try:
        response = int(input(question))
        if response > random_number:
            print("Lower... ")
        elif response < random_number:
            print("Higher... ")
        else:
            correct = response
            congrat_winner(correct, random_number)
    except ValueError:
            print("Invalid. Enter a number between 1-100 ")

If the input is not numeric, you return Invalid. Enter a number between 1-100, but not when your input is outside of the 1-100 range. You might want to add a check for that as well:

    try:
        response = int(input(question))
        if response not in range(1, 101):
            print("Invalid. Enter a number between 1-100 ")
        elif response > random_number:
            print("Lower... ")
        elif response < random_number:
            print("Higher... ")
        else:
            correct = response
            congrat_winner(correct, random_number)
    except ValueError:
            print("Invalid. Enter a number between 1-100 ")
\$\endgroup\$
  • \$\begingroup\$ Hi - thank you, I just spotted this - I have added the new code in the question section above. Greg. \$\endgroup\$ – Greg Aug 28 '17 at 20:42
1
\$\begingroup\$

In the last line of your human_guess() function

    guess = ask_number("Guess the number: ", random_number)

you assign the return value of the function ask_number() to a local variable guess which is not accessible from outside of the human_guess() function, so it may be omitted.

So that statement will become just:

ask_number("Guess the number: ", random_number)

Consequently your ask_number() function need not return anything so the last line in its definition

    return response

may be safely deleted, too.

\$\endgroup\$
  • \$\begingroup\$ Great - thank you. I have taken your advice onboard and have updated the code accordingly in the question section. \$\endgroup\$ – Greg Aug 28 '17 at 20:40
0
\$\begingroup\$

I have collated and considered all the fantastic answers above and greated the following program. I hope this is of significant use.

# Guess My Number Game
# MacOSX (Latest), Python 3.4.3, 28/08/2017
# Guess the Computers number between 1-100

from random import randint
from time import sleep
import sys

LOWER_LIMIT = 1
UPPER_LIMIT = 100

def intro():
    print("""
    Welcome to, Guess My Number!
    Simply guess the number between {}-{} that the Computer is 'thinking' of!
    You will have the option of replaying the game. First, guess the number!

    Ok Human, let's begin... """
    .format(LOWER_LIMIT, UPPER_LIMIT
            ))
    sleep(1)


def ask_number(question, random_number):
    response = None
    while response != random_number:
        try:
            response = int(input(question))
            if response not in range(LOWER_LIMIT, UPPER_LIMIT):
                print("Invalid. Enter a number between {}-{} "
                      .format(LOWER_LIMIT, UPPER_LIMIT))
            elif response < random_number:
                print("Try Higher... ")
            elif response > random_number:
                print("Try Lower... ")
            else:
                congrat_winner(response)
        except ValueError:
            print("Invalid. Enter a number between {}-{} "
                  .format(LOWER_LIMIT, UPPER_LIMIT))


def human_guess():
    random_number = randint(LOWER_LIMIT, UPPER_LIMIT)
    ask_number("Guess the number: ", random_number)


def congrat_winner(winning_guess):
    print()
    print("Calculating results...")
    sleep(1)
    print("CONGRATULATIONS")
    print("The answer was indeed,", str(winning_guess) + "!")


def replay_game():
    replay_question = None
    while replay_question not in ('y', 'n'):
        replay_question = input("Would you like to play again (y/n)? ").lower()
        if replay_question == 'y':
            print("Rebuilding game... ")
            sleep(1)
            main()
        elif replay_question == 'n':
            print("Goodbye... ")
            break
        else:
            print("please enter either 'y' or 'n'... ")


def main():
    intro()
    print()
    human_guess()
    print()
    replay_game()
    print()
    return 0 

if __name__ == '__main__':
     sys.exit(main())
\$\endgroup\$

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.