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()
200_success
145k22 gold badges190 silver badges478 bronze badges
asked May 11, 2019 at 13:56
\$\endgroup\$
3
  • \$\begingroup\$ Take a look at this - codereview.stackexchange.com/questions/220080/… and see if you find anything that helps you. \$\endgroup\$ Commented May 11, 2019 at 18:32
  • \$\begingroup\$ Removing the info about a bug won't fix the code... it's still broken. \$\endgroup\$ Commented May 13, 2019 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\$ Commented May 13, 2019 at 10:42

1 Answer 1

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.

answered May 17, 2019 at 15:11
\$\endgroup\$
1
  • \$\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\$ Commented May 24, 2019 at 21:03

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.