I made a smart Hangman game in Python and now want your opinion about the readability and code quality of this script. I want to improve my overall coding style. So I am happy to hear improvements for naming conventions, proper docstrings and logical/runtime enhancements.
So lets start with a quick explanation how my program works.
- The user chooses a word from a dictionary
- The program gets the possible words from the list based on the length
- The program calculates the character frequency before asking
- If the asked character is not in the secret word the possible word list gets updated again
- If the asked character is in the list the program asks for indices and based on that input the word list gets updated again
This concept works very good. The only problem is if several characters occurs with the same count the program asks in alphabetically order, but this can't be optimized I guess and is acceptable.
Let ́s get through the functions.
Here I load the vocabulary file:
def read_vocabuary_file(path):
"""
Function to read the vocabulary file
:param path: Path to file
:return: List with words
"""
words = []
with open(path) as file:
for line in file:
words.append(line.rstrip().lower())
return words
This function checks if the word is in the list
def check_word(word, words):
"""
Helper function to check if choosen word is in list
:param word: Word choosed by user
:param words: Word list
:return: True if word is in list - False if not
"""
if word in words:
return True
else:
return False
Get the character frequency
def get_char_frequency(words):
"""
Function to get the frequency of each character in the word list
:param words: Word list
:return: Frequency dict
"""
char_counts = Counter(letter for word in words for letter in word)
return char_counts
Function to get words with equal length as the hidden word
def get_equal_words(word_length, words):
"""
Function to get possible words with same length
:param word_length: The length of the user word
:param words: List of words
:return: Possible words with equal length
"""
possible_words = [word for word in words if len(word) == word_length]
return possible_words
Update function to remove words with wrong character
def filter_wrong_characters(character, words):
"""
Function to update the word list and remove words where asked character is not in
:param character: The character for that the program asked
:param words: Current word list
:return: Updated word list
"""
possible_words = [word for word in words if character not in word]
return possible_words
Function to get words where correct character is at a given index
def filter_correct_characters(character, indices, words):
"""
Function to update the word list with potential words
:param character: The character for that the program asked
:param indices: The indicies where the characters are
:param words: Current word list
:return: Updated word list
"""
possible_words = []
for idx in indices:
possible_words += [word for word in words if word[int(idx)] == character]
return list(set(possible_words))
Helper function to print the current game state
def print_screen(your_word, wrong_guess):
"""
Helper function to print the current game state
:param your_word: The masked user word
:return: None
"""
print(HANGMAN[wrong_guess])
print(' '.join(str(i) for i in your_word))
for i in range(len(your_word)):
print(i, end=" ")
print()
Helper function to clear the screen for better readability
def clear_screen():
"""
Helper function to clear terminal
:return: None
"""
os.system('cls' if os.name == 'nt' else 'clear')
Finally here is the full source code
from collections import Counter
import os
HANGMAN = ['''
+---+
| |
|
|
|
|
=========''', '''
+---+
| |
O |
|
|
|
=========''', '''
+---+
| |
O |
| |
|
|
=========''', '''
+---+
| |
O |
/| |
|
|
=========''', '''
+---+
| |
O |
/|\ |
|
|
=========''', '''
+---+
| |
O |
/|\ |
/ |
|
=========''', '''
+---+
| |
O |
/|\ |
/ \ |
|
=========''']
def read_vocabuary_file(path):
"""
Function to read the vocabulary file
:param path: Path to file
:return: List with words
"""
words = []
with open(path) as file:
for line in file:
words.append(line.rstrip().lower())
return words
def check_word(word, words):
"""
Helper function to check if choosen word is in list
:param word: Word choosed by user
:param words: Word list
:return: True if word is in list - False if not
"""
if word in words:
return True
else:
return False
def get_char_frequency(words):
"""
Function to get the frequency of each character in the word list
:param words: Word list
:return: Frequency dict
"""
char_counts = Counter(letter for word in words for letter in word)
return char_counts
def get_equal_words(word_length, words):
"""
Function to get possible words with same length
:param word_length: The length of the user word
:param words: List of words
:return: Possible words with equal length
"""
possible_words = [word for word in words if len(word) == word_length]
return possible_words
def filter_wrong_characters(character, words):
"""
Function to update the word list and remove words where asked character is not in
:param character: The character for that the program asked
:param words: Current word list
:return: Updated word list
"""
possible_words = [word for word in words if character not in word]
return possible_words
def filter_correct_characters(character, indices, words):
"""
Function to update the word list with potential words
:param character: The character for that the program asked
:param indices: The indicies where the characters are
:param words: Current word list
:return: Updated word list
"""
possible_words = []
for idx in indices:
possible_words += [word for word in words if word[int(idx)] == character]
return list(set(possible_words))
def print_screen(your_word, wrong_guess):
"""
Helper function to print the current game state
:param your_word: The masked user word
:return: None
"""
print(HANGMAN[wrong_guess])
print(' '.join(str(i) for i in your_word))
for i in range(len(your_word)):
print(i, end=" ")
print()
def clear_screen():
"""
Helper function to clear terminal
:return: None
"""
os.system('cls' if os.name == 'nt' else 'clear')
if __name__ == '__main__':
words = read_vocabuary_file('vocabulary.txt')
word = str(input('Please enter a word which is in the vocabulary: \t'))
word_in_list = False
while word_in_list == False:
if check_word(word, words):
word_in_list = True
else:
word = str(input('Word not in list, please choose another one: \t'))
words = get_equal_words(len(word), words)
playing = True
your_word = ["_"] * len(word)
wrong_guess = 0
clear_screen()
print_screen(your_word, wrong_guess)
while playing:
word_counts = get_char_frequency(words)
for char in ''.join(str(i) for i in your_word):
if char in word_counts:
del word_counts[char]
# try/except block if user messed up the input
# for example if he said no but the word contains the asked character
# breaks list of possible words
try:
most_common = word_counts.most_common()[0]
except IndexError:
print('Ops, something went wrong')
break
character_contained = str(input(f'Is there a [{most_common[0][0]}] ? - [Y]ES or [N]O :\t'))
while character_contained.lower() not in ['yes', 'y', 'no', 'n']:
print("Only yes/no is allowd")
character_contained = str(input(f'Is there a [{most_common[0][0]}] ? - [Y]ES or [N]O :\t'))
if character_contained.lower() in ['yes', 'y']:
valid_indices = False
while not valid_indices:
indices = list(input('At which indices? Please start with 0 and separate them with a space:').split())
for idx in indices:
try:
your_word[int(idx)] = most_common[0][0]
valid_indices = True
except:
print("Something went wrong. Please check your input again")
words = filter_correct_characters(most_common[0][0], indices, words)
else:
words = filter_wrong_characters(most_common[0][0], words)
wrong_guess += 1
clear_screen()
print_screen(your_word, wrong_guess)
if wrong_guess == 6:
clear_screen()
print('GAME OVER')
print_screen(your_word, wrong_guess)
playing = False
if len(words) == 1:
clear_screen()
print('I found your word:')
your_word = words[0]
print_screen(your_word, wrong_guess)
playing = False
I can't upload the vocabulary file, you can download it here
This can't ge a bit complicated to understand but I didn't found a better solution.
word_counts = get_char_frequency(words)
for char in ''.join(str(i) for i in your_word):
if char in word_counts:
del word_counts[char]
Since I always assign the first element of the frequency dict to the variable most_common
I first have to delete the characters which the program already guesses correctly. If not the program asks for a character it already founds over and over again.
-
\$\begingroup\$ Please do not edit the question, especially the code, after an answer has been posted. Changing the question may cause answer invalidation. Everyone needs to be able to see what the reviewer was referring to. What to do after the question has been answered. \$\endgroup\$pacmaninbw– pacmaninbw ♦2025年05月21日 11:41:15 +00:00Commented May 21 at 11:41
1 Answer 1
Organize programs around functions. The best parts of your current code are the small, well-focused functions and the worst part is the stuff after the main-guard where you deal with user inputs in an ineffective manner. Why is it ineffective? Because you abandoned functions. More details below.
Put all code in functions. Everything after the main-guard should
be in a function such as main()
or play_hangman()
.
When feasible, select names that agree with grammar. For example, a count
of the number wrong guesses should be named in a plural fashion or in a way
that conveys the idea of counting: for example, wrong_guesses
or n_wrong
are better than the singular wrong_guess
. Similarly, a list of hangman
ascii-images should be HANGMEN
, not HANGMAN
.
Avoid names based on ambiguous pronouns. Names like your_word
or my_foo
are often a bad idea. When writing/reading a computer program, who is the "you"
and who is the "me" or "we" or "they"? In the current code, the implication
is that "you" refers to the human player. But "we" (you and me) are the ones
writing the code and thinking about it: our perspective is that of a coder, not
a player. Rather than getting bogged down in referential mysteries like that,
aim for either simpler names where appropriate (eg, just word
) or for
clarifying names (eg, secret_word
or masked_word
).
If you can return directly from a simple function, do so. In a few functions your code becomes unnecessarily bureaucratic, either by creating an intervening variable that doesn't help with code clarity or by adding unnecessary if-else clauses. Two examples:
# Fine, but needlessly heavy.
def get_char_frequency(words):
char_counts = Counter(letter for word in words for letter in word)
return char_counts
# Better: return the Counter directly.
def get_char_frequency(words):
return Counter(letter for w in words for letter in w)
# The if-else structure isn't helping.
def check_word(word, words):
if word in words:
return True
else:
return False
# Better: return the bool directly.
# It turns out that we don't need this function, but that's a separate issue.
def check_word(word, words):
return word in words
Alternative names for the filtering functions. Your word-filtering
functions have reasonable implementations, but their names are not particularly
clear. None of these names convey an accurate sense of what they do:
filter_wrong_characters()
and filter_correct_characters()
sort of imply
that we are filtering characters, but we are actually filtering words; and
get_equal_words()
, in addition to being named differently than the other two
functions, sidesteps the main issue, which is not word-equality but word-size.
Here are some alternatives to consider. Some points worth noting: (1) the
functions establish a naming convention (filter by X); (2) they establish an
argument convention (words
first because that's what we are filtering, then
letter
if any, and then other stuff); and (3) the filter_by_known_letter()
implementation seems a bit more direct that your current approach because we
drive the logic via an iteration over words
, eliminating the need to filter
out duplicates.
def filter_by_word_size(words, size):
return [w for w in words if len(w) == size]
def filter_by_excluded_letter(words, letter):
return [w for w in words if letter not in w]
def filter_by_known_letter(words, letter, indices):
return [
w
for w in words
if all(w[i] == letter for i in indices)
]
Handle user input with reusable functions. You need to ask the user
for input several times, and each operation involves similar annoying details.
The way to avoid repetitive code like that is to think about collecting user
input in a general way: start a while-true loop; give the user a prompt; get
the user's reply; convert the reply to a value; validate the value; return if
OK, otherwise ask for input again. That sounds simple enough, but the slightly
tricky part is realizing that a general-purpose function for user input needs
to receive conversion and validation behavior as arguments -- in other words,
its arguments will be functions. Here's one way to do that kind of thing. [As
it turns out, your program can rely entirely on the choices
approach to
validation, but I've left the validate
parameter in place here to illustrate
the general approach.]
def get_input(prompt, convert = None, validate = None, choices = None):
# Prepare the conversion and validation functions.
convert = convert or (lambda x: x)
if choices:
validate = lambda x: x in choices
elif validate is None:
validate = lambda x: True
# Get user reply: convert and return if valid.
while True:
reply = input(prompt + ': ')
try:
value = convert(reply)
if validate(value):
return value
except Exception:
pass
prompt = 'Invalid input. Try again'
The user-interaction is too heavy and error-prone. Your program reverses
the typical hangman approach where the human guesses and the computer reveals.
That's perfectly fine both as a game and as a learning exercise, but it does
set up an odd dynamic where the computer is asking the human for answers that
the computer already knows -- specifically, whether the letter is in the word
and where the letter is located. And since humans can make mistakes, this
dynamic forces you to handle exceptions caused by data-entry errors related to
the indices. Two simplifications are possible. (1) Don't ask the user whether
and where (two questions); instead just ask where (interpret empty reply as
nowhere). (2) Compute the expected indices and pass them to get_input()
as
the only allowed choice, eliminating the possibility of data-entry errors.
Some adjustments to the printing function. It's good that you've
centralized your screen printing logic in one place. A few adjustments
can simplify the function and its usage. (1) You always clear before printing,
so just move the clear_screen()
call into print_screen()
. (2) Strings
are iterable, so you can print both the masked word and its indices via
simple print-join operations. (3) When the game is over, you have some awkward
code that ends up printing the screen twice, first in the ordinary way and
then with a game-over message. You can eliminate that repetition by
adding an optional message
parameter to the print_screen()
function.
def print_screen(masked_word, wrong_guesses, message = None):
clear_screen()
print(HANGMEN[wrong_guesses])
print(' '.join(masked_word))
print(' '.join(map(str, range(len(masked_word)))))
if message:
print(message)
Playing hangman. With those changes in place, the game-playing function can
be simplified a fair bit. A few other points worth noting: (1) where it makes
sense, outsource logical details to utility functions, such as
most_common_letter()
; (2) readability in a moderately large function like
this can be enhanced with fairly simple comments that organize the code into
smaller logical units; (3) to get indices from the user, we need a small
data-conversion function; and (4) rather than hardcoding six wrong guesses as a
game-over condition, compare the number of wrong guesses to the length of
HANGMEN
.
def play_hangman():
# Select word from vocabulary.
words = read_vocabuary_file('vocabulary.txt')
word = get_input(
'Please enter a word which is in the vocabulary',
choices = words,
)
# Filter down to words of that size.
size = len(word)
words = filter_by_word_size(words, size)
masked_word = ['_'] * size
# Play until solved or failed.
game_over = None
wrong_guesses = 0
print_screen(masked_word, wrong_guesses)
while not game_over:
# Guess the most common letter.
letter = most_common_letter(words, masked_word)
# Force the poor human to enter the letter locations, if any.
expected_indices = [i for i, c in enumerate(word) if c == letter]
indices = get_input(
f'If the word contains [{letter}], enter its indices',
convert = parse_ints,
choices = [expected_indices],
)
# Filter words based on reply.
if indices:
words = filter_by_known_letter(words, letter, indices)
for i in indices:
masked_word[i] = letter
else:
words = filter_by_excluded_letter(words, letter)
wrong_guesses += 1
# Check for game over.
if wrong_guesses >= len(HANGMEN):
game_over = 'GAME OVER!'
elif len(words) == 1:
game_over = 'I found your word!'
masked_word = words[0]
# Display.
print_screen(masked_word, wrong_guesses, message = game_over)
def most_common_letter(words, masked_word):
counts = get_char_frequency(words)
for c in masked_word:
counts.pop(c, None)
return counts.most_common()[0][0]
def parse_ints(reply):
return sorted(map(int, reply.split()))