The previous day, I helped someone build a really simple console "Lucky Number" game. It's really simple, the user has to choose a number, the program will also choose one, and if there's a match between the two, the user wins.
To made the game a bit more visual, the numbers are initially blue and displayed in a nice way.
I'm looking for the following things in a review, but any advice is welcome:
- is the code structured ok?
- did I use the correct data structures along the program? (e.g.: is it okay for
Colors
to be anEnum
, shouldnumbers
be anumber
:color
map and so on) - are type annotations used correctly?
- are the function / variable names suggestive enough?
- is there anything in terms of performance that can be improved?
Here's the code:
import random
import sys
import time
from enum import Enum
from typing import Dict
LINE_WIDTH = 5
EXTRA_PADDING = 6
# seconds to wait until the next number
# is picked up
SECONDS_TO_WAIT = 5
MIN_NUMBER = 1
MAX_NUMBER = 30
class Colors(Enum):
red = '033円[91m'
green = '033円[92m'
blue = '033円[94m'
end = '033円[0m'
def assign_color_to_number(number: int, default_color: str) -> str:
"""
Show ANSI colors in terminal for nicer UX.
Arguments:
number (int): A number between `MIN_NUMBER` and `MAX_NUMBER`
default_color (str): The color of a number
Returns:
str: Each number with it's own color
"""
for color in Colors:
if color.name == default_color:
return f'{color.value}{number}{Colors.end.value}'
else:
return f'{number}'
def print_table(numbers_color_map: Dict[int, str]) -> None:
"""
Print a nice table with colorful numbers wrapped after
`LINE_WIDTH` numbers.
Arguments:
numbers_color_map (dict): A map between numbers and their color.
"""
print('\n')
padding = max([int(len(color.name)) for color in Colors]) + len(str(MAX_NUMBER)) + EXTRA_PADDING
for number, default_color in numbers_color_map.items():
colored_number = assign_color_to_number(number, default_color).ljust(padding)
print(colored_number, end=' ') if number % LINE_WIDTH != 0 else print(colored_number)
print('\n')
def countdown(seconds: int) -> None:
"""
Let user know how much until the next game.
Arguments:
seconds (int): How many seconds until next game.
"""
for second in range(seconds, -1, -1):
sys.stdout.write(f"\r{second} seconds remaining until next "
f"number will be picked.")
time.sleep(1)
sys.stdout.flush()
print('\n')
def get_user_number() -> int:
"""
Sanitize user's input.
Returns:
int: User's chosen number.
"""
while True:
try:
value = int(input(f'Please chose a number between '
f'{MIN_NUMBER} and {MAX_NUMBER}: '))
if MAX_NUMBER < value < MIN_NUMBER:
print(f"Please insert a number between {MIN_NUMBER} and "
f"{MAX_NUMBER}!\n")
continue
return value
except ValueError:
print(f"Please insert a number between {MIN_NUMBER} and "
f"{MAX_NUMBER}!\n")
continue
def play():
numbers_color_map = {
number: 'blue' for number in range(MIN_NUMBER, MAX_NUMBER + 1)
}
print_table(numbers_color_map)
user_number = get_user_number()
print(f"You chose {user_number}. GOOD LUCK!\n\n")
countdown(SECONDS_TO_WAIT)
random_number = random.randint(MIN_NUMBER, MAX_NUMBER)
if user_number != random_number:
numbers_color_map.update({
random_number: 'green',
user_number: 'red'
})
print_table(numbers_color_map)
print(f' Your number: {user_number}\n'
f'Computer number: {random_number}\n\n'
f'You did not win. GOOD LUCK NEXT TIME!')
else:
print('You guessed it! Congrats! Please fill in the information '
'below in order to receive your prize.\n\n')
# reset our numbers to their initial color
numbers_color_map.update({
random_number: 'blue',
user_number: 'blue'
})
def main():
while True:
os.system('cls' if os.name == 'nt' else 'clear')
print('\nWELCOME TO THE LUCKY NUMBER!')
play()
answer = input('\nPlay again? [Y]es/[N]o: ')
if answer.lower() == 'n':
break
print('\nTHANKS FOR PLAYING!')
if __name__ == '__main__':
main()
You can either c/p the code and run it locally or play it here.
2 Answers 2
Firstly, I really enjoyed reviewing this code, it is very readable and understandable. The code looks nice. I would just highlight few things that might be improve
Reduce the use of Globals
You want to always reduce your use of global variables. In your program, you have a lot of global variables, you can eliminate all of them by encapsulating them in a datatclass
. I would not expect MIN_NUMBER
and MAX_NUMBER
to be constant as they might change.
Here I created a data class named LuckyGameConfig
and hide some configuration data in it
from dataclasses import dataclass
@dataclass
class LuckyGameConfig:
"""Lucky number guessing game configurations"""
line_width: int = 5
padding: int = 6
seconds_to_wait: int = 5
min_number: int = 1
max_number: int = 30
This looks better now, we know where to look when we need to modify how we want our game to look.
Prefer colorama
Module to ANSI Color
I see you created a nice Enum
class to hold your colors, colorama
module shines here, it makes what you are doing really easy.
from colorama import init, Fore
We can simply create a green color
green_color = Fore.GREEN
Fore.RESET # reset our color
The module also have styles like BOLD
, ITALIC
, you might want to check the module out.
Reduce print_table
Complexity
A lot of stuff are going on in print_table
. The name is also misleading, what table are we printing here. It would not take much to get confused on what is really going on in the function.
Why smashed this into one line?
print(colored_number, end=' ') if number % LINE_WIDTH != 0 else print(colored_number)
This looks smart but confuses the reader, he/she would need to pause a little to understand this piece of code.
This following can also be improved.
padding = max([int(len(color.name)) for color in Colors]) + len(str(MAX_NUMBER)) + EXTRA_PADDING
Let's call our function display_numbers
def display_numbers(numbers: dict) -> None:
"""Display a nice table with colorful numbers wrapped.
Arguments:
numbers (dict): A dict of numbers that maps to its color
"""
for number, color in numbers.items():
if number % LuckyGameConfig.line_width != 0:
format = f'{number:<{LuckyGameConfig.padding}}'
print(color, format, end=' ')
else:
print(color, number)
print(Fore.RESET)
This looks more readable.
Fix the Bug
The little bug in your program. Choosing a number greater than MAX_NUMBER
or less than MIN_NUMBER
seems to work. On inspecting the code, I found that you wrote this test
if MAX_NUMBER < value < MIN_NUMBER:
Let's see a simple example, say the user chooses 90, the condition evaluates to 30 < 90 < 1
which is False
, the condition statement never evaluates and the error passes silently and value
is initialized.
The bug can be fixed like this
if value < MIN_NUMBER or value > MAX_NUMBER:
The full code becomes
"""A random lucky number game"""
import sys
import time
import os
from dataclasses import dataclass
from enum import Enum
from colorama import init, Fore
from random import randint
from typing import Dict
@dataclass
class LuckyGameConfig:
"""Lucky number guessing game configurations"""
line_width: int = 5
padding: int = 6
seconds_to_wait: int = 5
min_number: int = 1
max_number: int = 30
def display_numbers(numbers: dict) -> None:
"""Display a nice table with colorful numbers wrapped.
Arguments:
numbers (dict): A dict of numbers that maps to its color
"""
for number, color in numbers.items():
if number % LuckyGameConfig.line_width != 0:
format = f'{number:<{LuckyGameConfig.padding}}'
print(color, format, end=' ')
else:
print(color, number)
print(Fore.RESET)
def get_user_number() -> int:
"""
Sanitize user's input.
Returns:
int: User's chosen number.
"""
while True:
try:
value = int(input(f'Please chose a number between '
f'{LuckyGameConfig.min_number} and {LuckyGameConfig.max_number}: '))
if value < LuckyGameConfig.min_number or value > LuckyGameConfig.max_number:
print(f"Please insert a number between {LuckyGameConfig.min_number} and "
f"{LuckyGameConfig.max_number}!\n")
continue
return value
except ValueError:
print(f"Please insert integer values!\n")
continue
def countdown(seconds: int) -> None:
"""
Let user know how much until the next game.
Arguments:
seconds (int): How many seconds until next game.
"""
for second in range(seconds, -1, -1):
sys.stdout.write(f"\r{second} seconds remaining until next "
f"number will be picked.")
time.sleep(1)
sys.stdout.flush()
print('\n')
def play():
"""Play a lucky number guessing game"""
init() # initialize colorama to get our basic colors
numbers = {number: Fore.BLUE for number in range(LuckyGameConfig.min_number, LuckyGameConfig.max_number + 1)}
display_numbers(numbers)
user_number = get_user_number()
print(f"You chose {user_number}. GOOD LUCK!\n\n")
countdown(LuckyGameConfig.seconds_to_wait)
random_number = randint(LuckyGameConfig.min_number, LuckyGameConfig.max_number)
if user_number != random_number:
numbers.update({
random_number: Fore.GREEN,
user_number: Fore.RED
})
display_numbers(numbers)
print(f' Your number: {user_number}\n'
f'Computer number: {random_number}\n\n'
f'You did not win. GOOD LUCK NEXT TIME!')
else:
print('You guessed it! Congrats! Please fill in the information '
'below in order to receive your prize.\n\n')
# we do not need to reset the colors, the while loop in main does the job
def main():
while True:
os.system('cls' if os.name == 'nt' else 'clear')
print('\nWELCOME TO THE LUCKY NUMBER!')
play()
answer = input('\nPlay again? [Y]es/[N]o: ')
if answer.lower() == 'n':
break
print('\nTHANKS FOR PLAYING!')
if __name__ == '__main__':
main()
-
1\$\begingroup\$ Nice review. Regarding Fix the Bug I find
if not MIN_NUMBER <= value <= MAX_NUMBER:
nice to read. \$\endgroup\$AcK– AcK2020年12月29日 11:32:32 +00:00Commented Dec 29, 2020 at 11:32
Nice implementation, the code is well structured and easy to understand. My suggestions:
- Printing the table: there are two variables to configure how to display the numbers:
LINE_WIDTH = 5 EXTRA_PADDING = 6
LINE_WIDTH
seems to be how many numbers can be displayed for each line, so a better name could beNUMBERS_PER_LINE
. The variableEXTRA_PADDING
is ok but I would document it in theprint_table
function. - Bug: this condition doesn't catch the wrong input:
Assuming thatif MAX_NUMBER < value < MIN_NUMBER: print(f"Please insert a number between {MIN_NUMBER} and {MAX_NUMBER}!\n")
MIN_NUMBER
andMAX_NUMBER
are valid options, you can change it to:if value < MIN_NUMBER or value > MAX_NUMBER: print(f"Please insert a number between {MIN_NUMBER} and {MAX_NUMBER}!\n")
- Colors:
The colorclass Colors(Enum): red = '033円[91m' green = '033円[92m' blue = '033円[94m' end = '033円[0m'
end
is a bit confusing for an enum calledColors
. ConsiderANSIColorCode
as a name, from here. - Design: the logic to display the numbers is interesting but not very easy to reuse. Suppose I want to reuse it, I need to copy the functions
assign_color_to_number
,print_table
,Colors
, and the two constants to configure how to display the numbers. Plus understanding how to create and use the mapnumbers_color_map
. Consider creating a class for it, likeTablePrinter
orRangePrinter
, to make it easier to reuse and test. For example:class RangePrinter: NUMBERS_PER_LINE = 5 EXTRA_PADDING = 6 def __init__(self, start, end, default_color='blue'): # Initialize the map setting all numbers to default_color def print(self): # Print numbers to console def set_number_to_color(self, number, color): # Set color for a single number def set_all_numbers_to_color(self, color): # Set color for all numbers