2
\$\begingroup\$

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.

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

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()))
\$\endgroup\$
1
  • \$\begingroup\$ I see your points! Thank you for your detailed answer. \$\endgroup\$
    – adama
    May 28 at 11:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.