Could you look at my code for the completed training project of the Python Hangman game and tell me where you can improve the organization and code style?
import random
from string import ascii_lowercase
class Hangman:
@staticmethod
def print_welcome():
print('H A N G M A N')
def __init__(self):
self.__state = 'welcome'
self.__keyword = None
self.__keywords = ['python', 'java', 'kotlin', 'javascript']
self.__letters_number = None
self.placeholder = '-'
self.hint = []
self.keyword_set = set()
self.attempt = 8
self.victory = False
self.user_letters_set = set()
self.deleted_letters_set = set()
self.mistake_message = ''
self.user_choice = ''
@property
def state(self) -> str:
return self.__state
@state.setter
def state(self, state_string):
self.__state = state_string
@property
def keyword(self) -> str:
return self.__keyword
@keyword.setter
def keyword(self, new_word):
self.__keyword = new_word
@property
def keywords(self) -> list:
return self.__keywords
def print_query_message(self):
self.user_choice = input('Type "play" to play the game, "exit" to quit:')
def run(self):
self.print_welcome()
while True:
if self.state == 'welcome':
while self.user_choice not in ['play', 'exit']:
self.print_query_message()
if self.user_choice == 'exit':
return None
self.state = 'init'
if self.state == 'init':
self.init_func()
self.state = 'on'
if self.state == 'on':
if not self.process_player():
self.state = 'off'
if self.state == 'off':
print(f'{self.get_analyze_result()}\n')
self.user_choice = ''
self.state = 'welcome'
def process_player(self):
print('\n' + ''.join(self.hint))
answer = input('Input a letter: ')
self.check_answer(answer)
if len(self.keyword_set) == 0:
self.victory = True
return False
if self.attempt == 0:
return False
return True
def find_letter_pos(self, letter):
letter_positions = []
ind = 0
while ind < len(self.keyword):
pos = self.keyword.find(letter, ind)
if pos == -1:
break
letter_positions.append(pos)
ind = pos + 1
return letter_positions
def transform_hint(self, letter):
positions = self.find_letter_pos(letter)
temp = self.hint
for ind in positions:
temp[ind] = letter
self.hint = temp
def init_func(self):
self.keyword = random.choice(self.keywords)
self.keyword_set = set(self.keyword)
self.hint = [self.placeholder for _i in range(len(self.keyword))]
def check_answer(self, letter):
if self.is_input_mistake(letter):
print(self.mistake_message)
elif letter in self.keyword_set:
self.transform_hint(letter)
self.keyword_set.discard(letter)
self.deleted_letters_set.add(letter)
else:
if not self.victory:
self.for_reduce_attempt(letter)
print(self.mistake_message)
def is_input_mistake(self, letter):
if len(letter) != 1:
self.mistake_message = 'You should print a single letter'
return True
if letter not in ascii_lowercase:
self.mistake_message = 'It is not an ASCII lowercase letter'
return True
if any([letter in self.deleted_letters_set, letter in self.user_letters_set]):
self.mistake_message = 'You already typed this letter'
return True
return False
def get_analyze_result(self):
if self.victory:
return 'You guessed the word!\nYou survived!'
return 'You are hanged!'
def for_reduce_attempt(self, letter):
self.user_letters_set.add(letter)
self.mistake_message = 'No such letter in the word'
self.attempt -= 1
hangman = Hangman()
hangman.run()
2 Answers 2
UX
When I run the code, I see that you keep track of letters that were already guessed, and you notify the user if the user tries to guess the same letter again. That is a nice feature.
When I enter an incorrect guess, the program notifies me, as expected. However, a key feature of Hangman is to let the user know how many guesses remain before losing the game. Typically this is done with a stick figure representation, but it would be sufficient to simply report the number:
You have 5 incorrect guesses; you have 3 remaining incorrect guesses
Documentation
The PEP 8 style guide recommends adding docstrings for classes and functions.
The class docstring could summarize its purpose and perhaps show example usage code:
"""
Hangman game
The user has 8 chances to guess a word.
Accepts input from the user's command line
until the user correctly guesses the word
or runs out of guesses.
"""
Enum
Since the state
variable is limited to a certain set of values,
it would be worth considering using an enumerated type
(Enum).
This also clearly documents what states are available.
Naming
Many of the functions have meaningful names; however, here are some suggestions for improvement.
The name get_analyze_result
is a little confusing. It would be simpler as
get_result
. Alternately, since it is only called inside a print
statement,
you could just have it call print
, and change the name:
def print_result(self):
if self.victory:
print('You guessed the word!\nYou survived!')
print('You are hanged!')
The name init_func
would be better without the "func" since we know
it is a function. Perhaps init_game
would be more specific.
The name process_player
is too generic. Perhaps get_guess
would be more specific. Also, since the function returns a boolean value,
it is common to use the "is_" prefix in the name.
Unused
This variable is unused and can be deleted:
__letters_number
Suggestions for print_query_message
First, this method does more than just print a message; it prints a prompt soliciting from the user an input command. So I would suggest renaming this method to get_user_command
, for example.
Second, this method must know what the valid user commands are since they are presented to the user in the prompt. But it is the caller of this function that also must know what the valid commands for obvious reasons. I would prefer to see this knowledge in one place. So I would suggest the following changes to get_user_command
:
- Have
get_user_command
take two arguments: a prompt, and a list of valid commands the user can enter. This function can then perform validation against the user's input and re-display the prompt when the input is not valid. Thus the caller can be sure that a valid command is always returned. - The current logic relies on attribute
self.choice
initialized to an invalid value by the caller to ensure that an initial prompt is output. This makes the method and a client of the method too closely coupled. A slight change in the logic removes this coupling. - Currently, the method stores the entered command as an instance attribute. I see no reason why this method should not simply return the command to the caller.
Incorporating these changes, we have:
def get_user_command(self, prompt, valid_commands):
while True:
command = input(prompt)
if command in valid_commands:
return command
def run(self):
while True:
...
if self.state == 'welcome':
command = self.get_user_command(
'Type "play" to play the game, "exit" to quit: ',
['play', 'exit']
)
...
The Finite State Machine in run
Method run
implements a finite state machine where the program is in some initial state and as the game progresses the state changes determining what the next action the program will take. Here are a few suggestions:
- The current state of the finite state machine is only a concern of method
run
. I would therefore makestate
a local variable to the method instead of an instance attribute. - The method tests and sets a state and then falls through testing for a different state, which might have been set as the action performed for the prior state. This works but is unnecessary since the program is in an infinite loop (at least until the user quits the game) and the new state will be tested on the next iteration anyway. If you have Python 3.10 or greater, you could take advantage of the new
match
statement:
def run(self):
self.print_welcome()
state = 'welcome'
while True:
match state:
case 'welcome':
command = self.get_user_command(
'Type "play" to play the game, "exit" to quit: ',
['play', 'exit']
)
if command == 'exit':
return
state = 'init'
case 'init':
self.init_func()
state = 'on'
case 'on':
if not self.process_player():
state = 'off'
case 'off':
print(self.get_analyze_result(), end='\n\n')
state = 'welcome'
I believe this is cleaner. Note that in state off
your print statement did not gain anything by using an f-string.
Attribute Naming
You have, for example, attribute self.__keyword
that is used to implement the keyword
property. It is more normal for such attributes to begin with a single underscore instead of the two you use. Attributes that begin with double underscore characters cause name mangling meaning the name of the actual attribute is modified to include the class name to prevents naming conflicts in a subclass. I don't think that this is really something you are actually concerned with. So I would suggest that you just use a single underscore. In fact, all attributes that you consider "private" (i.e. not to be accessed by a client of the class) should be renamed to use a leading underscore.
Make the Game "Importable"
Use the following check so that the game can be imported by another module without automatically running the game:
if __name__ == '__main__':
hangman = Hangman()
hangman.run()