6
\$\begingroup\$

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?

Code on GitHub

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()
toolic
15.1k5 gold badges29 silver badges211 bronze badges
asked May 17, 2020 at 21:41
\$\endgroup\$
0

2 Answers 2

3
\$\begingroup\$

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
answered Jun 9 at 17:05
\$\endgroup\$
3
\$\begingroup\$

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:

  1. 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.
  2. 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.
  3. 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:

  1. The current state of the finite state machine is only a concern of method run. I would therefore make state a local variable to the method instead of an instance attribute.
  2. 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()
answered Jun 9 at 20:10
\$\endgroup\$

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.