2
\$\begingroup\$

I'm learning python and created a number guessing game on which I would like to get your feedback on the structure and style of the program.

import random


LOWER_NUMBER = 1
HIGHEST_NUMBER = 10


def print_header():
    print("-" * 40)
    print("Welcome to the number guessing game")
    print("-" * 40)


def user_input():
    """Collect and validate the entries made by the users

    Returns
    -------
    int
        Gives back the validated number entered by the user.
    """
    while True:
        try:
            guess = int(
                input(f'Enter your Guess of a number between '
                      f'{LOWER_NUMBER} - {HIGHEST_NUMBER}: '))
            if guess < LOWER_NUMBER or guess > HIGHEST_NUMBER:
                print("The entered number is out of range, try again.")
                continue
        except ValueError:
            print('you did not enter a number, please try again.')
            continue
        else:
            break
    return guess


def generate_number_to_guess(lower_number, higher_number):
    """Generates a random number between the given lowest and highest number

    Parameters
    ----------
    lower_number : int
        Lowest number for the generator

    higher_number : int
        highest number for the generator

    Returns
    -------
    int
        returns the generated random number that is in the given range.
    """
    return random.randint(lower_number, higher_number)


def play_again():
    """Perform and validate entry for a next game

    Returns
    -------
    string
        returns the validated user entry for a next game.
    """
    while True:
        new_game = input(
            "Would you like to play again? [y]es/[n]o: ")
        if new_game.lower() not in ["n", "y"]:
            print('Wrong entry please use y or n')
            continue
        else:
            break

    return new_game.lower()


def start_game():
    """This is the main loop that runs the app.
    """
    highscore = 0

    while True:
        print_header()
        number_to_guess = generate_number_to_guess(LOWER_NUMBER,
                                                   HIGHEST_NUMBER)
        guess = 0
        count = 0

        while guess != number_to_guess:
            guess = user_input()
            count += 1
            if guess < number_to_guess:
                print("It's higher")
            elif guess > number_to_guess:
                print("It's lower")
        else:
            print(
                f'\nYou geussed the right number and needed {count} tries')
            if count < highscore or highscore == 0:
                highscore = count

            # validate the input for a new game
            another_game = play_again()
            if another_game == 'y':
                print(f"\n\nThe HIGHSCORE is {highscore}")
                continue
            elif another_game == 'n':
                print("\n Thanks for playing, see you next time!")
                break


if __name__ == "__main__":
    start_game()
\$\endgroup\$
  • \$\begingroup\$ Take a look at this - codereview.stackexchange.com/questions/220080/… and see if you find anything that helps you. \$\endgroup\$ – Justin May 11 '19 at 18:32
  • \$\begingroup\$ Removing the info about a bug won't fix the code... it's still broken. \$\endgroup\$ – t3chb0t May 13 '19 at 7:57
  • \$\begingroup\$ @t3chb0t I did not remove info about a bug. The additional info that has been removed was related to an optimization that did not work as I expected, but this seems to be out of scope of the questions that should be put forward here, as this is about code review, not exploring suggestions. \$\endgroup\$ – Peter Pallen May 13 '19 at 10:42
2
\$\begingroup\$

There is a lot of things done right in your program. The code is structured into functions, there are docstrings, you validate user input and you even have a main guard. Still there is room for improvement.

code duplication

This is a minor issue in your code but I do not want to skip it. In

def print_header():
    print("-" * 40)
    print("Welcome to the number guessing game")
    print("-" * 40)

which could read

def print_header():
    dashed_line = "-" * 40
    print(dashed_line)
    print("Welcome to the number guessing game")
    print(dashed_line)

The reason for not duplicate code (even if it is less code) is maintenance. If you ever chose to increase the line length there is a single place to edit. In your case the chance to miss the other number is low. But keep that in mind.

naming of functions and variables

user_input() is not the best name as there is many user interaction. A better name would be get_user_guess(). Also play_again() does not play but getting user input again. Might be get_user_play_again().

overdone design

Your function

def generate_number_to_guess(lower_number, higher_number):
    """Generates a random number between the given lowest and highest number

    Parameters
    ----------
    lower_number : int
        Lowest number for the generator

    higher_number : int
        highest number for the generator

    Returns
    -------
    int
        returns the generated random number that is in the given range.
    """
    return random.randint(lower_number, higher_number)

does nothing but calling a standard(!) function with the same parameter signature. Instead call the standard function directly. Such detors are not good in terms of maintainability. Every reasonably experienced python programmer does know randint(). If reading that in a code he exactly knows what's happening. If he stumbles onto a call of your function he must immediately look upon your implementation to get to know what it does. He must not trust your docstring (between?!) but read the code. He is even forced to write unittests if they are not existing and there are some lines of implementation. In your case it is a pure delegation to a standard function, so the reviewer is happy to find a reliable implementation. But he lost time for no reason. Do not do this.

Beside that issues your code is very maintainable! It is structured and readable. The complexity of your functions is reasonable. Be very careful with docstrigs. Your docstring starts with

"""Generates a random number between the given lowest and highest number

while randint.__doc__ returns

'Return random integer in range [a, b], including both end points.\n        '

This is 'between' vs 'including both endpoints'.

global constants

Simply try to avoid them, especially if they are not naturally constant. You nearly did it right in the superfluous function generate_number_to_guess where you decided to pass tarameters instead of accessing the globals directly. However you stopped early on the process of passing. If you also pass the variables to start_game()

def start_game(lower_number, higher_number):

and call the generator

    number_to_guess = generate_number_to_guess(lower_number,
                                               higher_number)

you are done with that path. Now you also pass these numbers to user_input()

def user_input(lower_number, higher_number):

and eliminate the globals in there.

You end up with shouting variable names (no global constants)

LOWER_NUMBER = 1
HIGHEST_NUMBER = 10

# [...]

if __name__ == "__main__":
    start_game()

which you change to

if __name__ == "__main__":
    start_game(1, 10)

If a value is not globally constant for a function - pass it as a parameter. The function is then loosly coupled to the rest and is perfectly testable.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for the time you took to give me such detailed feedback, it's very much appreciated. I'm always doubting if I'm not going too granular in breaking down things. So thanks for the insight into the mistakes that I made. I also take your comments to heart on constants and will keep this in mind for future programs. \$\endgroup\$ – Peter Pallen May 24 '19 at 21:03

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.