21
\$\begingroup\$

I am a Java + Angular developer. My new job is as a Senior Python developer ... but I've never written a Python program in my life. In order to teach myself the language, I've started writing some simple projects.

This is a console-based Hangman game in Python 3.7. The game picks a 'secret word' from a list, and then enters a game loop prompting the user to guess letters. After 6 incorrect guesses, the game is over and the user loses. Otherwise if the user guesses all the letters, the user wins. After the game finishes, the user is prompted to start a new game, which reruns the method with a new secret word.

The word list and the hangman drawing I have omitted because I cribbed them from this gist on GitHub as a direct copy-paste (renaming the hangman array to HANGMAN_STAGES and the word list to WORDS). I can include it in the post if need be, but it seems like an extra 60 lines which really aren't needed.

import random
import sys
from typing import Tuple

# Omitted declarations: https://gist.github.com/chrishorton/8510732aa9a80a03c829b09f12e20d9c
# HANGMAN_STAGES = [...]
# WORDS = ...

def run_game() -> None:
    """The main game loop. Will prompt the user if they would like to start a new game at the end."""
    print("WELCOME TO HANGMAN.")

    secret_word = pick_secret_word()
    guessed_letters = []
    incorrect_guesses = 0
    won = False
    round = 1

    while won == False and incorrect_guesses < 6:
        print('\n\nROUND ' + str(round))
        incorrect_guesses, won = process_turn(incorrect_guesses, secret_word, guessed_letters)
        round += 1

    print("\n\n")

    if won == False:
        print("GAME OVER! You lost.")
        draw_hangman(6)
    else:
        print("Congratulations! You won!", end=" ")

    print("The secret word was: " + secret_word)

    if play_again():
        run_game()


def pick_secret_word() -> str:
    """
    Chooses a new secret word from the list of available secret words. The word is chosen psuedo-randomly.
    :return: the new secret word
    """
    index = random.randint(0, len(WORDS))
    return WORDS[index].upper()


def process_turn(incorrect_guess_count: int, secret_word: str, guessed_letters: list) -> Tuple[int, bool]:
    """
    Processes a user's turn. First draws the current state of the game: current hangman, partially-guessed word, and
    list of previously guessed letters. Then prompts the user for their next guess, evaluates that guess to see if it
    was correct, and then updates the game state.

    :param incorrect_guess_count: the number of previous incorrect guesses
    :param secret_word: the secret word
    :param guessed_letters: the list of previously guessed letters
    :return: (updated number of inccorect guesses, True/False indication of whether the user has won)
    """
    draw_hangman(incorrect_guess_count)
    draw_secret_word(secret_word, guessed_letters)
    print_guessed_letters(guessed_letters)
    next_letter = prompt_for_guess(guessed_letters)
    return apply_guess(next_letter, secret_word, incorrect_guess_count, guessed_letters)


def print_guessed_letters(guessed_letters: list) -> None:
    """
    Sorts the list of previously-guessed letters and prints it to screen.

    :param guessed_letters: the list of previously guessed letters
    :return: Nothing
    """
    guessed_letters.sort()
    print("Guesses: " + str(guessed_letters))


def apply_guess(next_letter: str, secret_word: str, incorrect_guess_count: int, guessed_letters: list) -> Tuple[int, bool]:
    """
    Checks the validity of the user's guess. If the guess was incorrect, increments the number of incorrect guesses by
    1. If the user has guessed all of the letters in the secret word, return an indication that the user has won the
    game.

    :param next_letter: the user's guess
    :param secret_word: the secret word
    :param incorrect_guess_count: the number of previously incorrect guesses
    :param guessed_letters: the list of previously guessed letters
    :return: (the updated number of incorrected guesses, True/False indicating if the user has won the game)
    """
    guessed_letters.append(next_letter)
    correct, letters_remaining = check_guess_against_secret(next_letter, secret_word, guessed_letters)

    if correct == False:
        incorrect_guess_count += 1

    if letters_remaining == 0:
        return incorrect_guess_count, True

    return incorrect_guess_count, False


def check_guess_against_secret(next_letter: str, secret_word: str, guessed_letters: list) -> Tuple[bool, int]:
    """
    Determines if the user has guessed correctly. Also evaluates the secret word to determine if there are more letters
    left for the user to guess.

    :param next_letter: the user's guessed letter
    :param secret_word: the secret word
    :param guessed_letters: the list of previously guessed letters
    :return: (True/False indicating if the guess was correct, 0 if no letters left and positive integer otherwise)
    """
    correct = next_letter in secret_word

    letters_remaining = 0
    for letter in secret_word:
        # Known issue: if a letter is present in the secret multiple times, and is not guessed,
        # letters_remaining incremented by more than one.
        if letter not in guessed_letters:
            letters_remaining += 1

    return correct, letters_remaining


def prompt_for_guess(guessed_letters: list) -> str:
    """
    Prompts the user for their next guess. Rejects guesses that are more than a single letter, and guesses which were
    already made previously. Returns the (validated) guess.

    :param guessed_letters: the list of previously guessed letters
    :return: the user's next guess
    """
    guess = input("Your guess? ").strip().upper()
    if len(guess) > 1:
        print("Sorry, you can only guess one letter at a time.")
        return prompt_for_guess(guessed_letters)
    elif guess in guessed_letters:
        print("Sorry, you already guessed that letter.")
        return prompt_for_guess(guessed_letters)
    return guess


def draw_hangman(number_of_incorrect_guesses: int) -> None:
    """
    Draws the appropriate hangman stage, given the number of incorrect guesses. 0 or fewer will draw the empty scaffold.
    6 or more will draw the fully hanged man.

    :param number_of_incorrect_guesses: the number of incorrect guesses the player has made in the current game
    :return: Nothing
    """
    if (number_of_guesses < 0):
        number_of_guesses = 0
    if (number_of_guesses > 6):
        number_of_guesses = 6
    print(HANGMAN_STAGES[number_of_guesses])


def draw_secret_word(secret_word: str, guessed_letters: list) -> None:
    """
    Prints the secret word, with underscores representing unknown letters and with any correctly-guessed leters printed
    in the appropriate location within the word.

    :param secret_word: The secret word
    :param guessed_letters: All previous guesses
    :return: Nothing
    """
    for letter in secret_word:
        to_print = letter if letter in guessed_letters else '_'
        print(to_print, end=' ')
    print("\n")


def play_again() -> bool:
    """
    Prompts the user if they would like to play again. If the user enters something other than Y/y/N/n, it will continue
    prompting until the use enters a valid value. If the user indicates Y or y, this method returns True; N or n will
    return False

    :return: True if the user would like to start a new game; False otherwise
    """
    choice = ''
    while choice != "Y" and choice != "N":
        choice = input("Play again? (Y/N)").strip().upper()
    return choice == "Y"

run_game()

Things I know are an issue:

  • No unit tests. I'm still trying to figure out how to test input() and print() statements
  • I attempted pydoc docstring comments on all of the methods, but couldn't figure out how to properly document a returned tuple.
  • I'm a bit inconsistent in my string quotation mark usage because of my dual background with Java (double quotes) and Angular/Typescript (single quotes)
  • The check_guess_against_secret method doesn't accurately count how many unique letters remain; duplicate unguessed letters in the secret are counted twice. But since we only evaluate whether that value is 0, it could be swapped over to a boolean flag on refactor

In addition to a general review, I would very much appreciate it if folks could point out places where I'm doing things in a more Java (or Typescript!) way rather than a Python way.

\$\endgroup\$
  • \$\begingroup\$ Using upper or lower for case insensitive string comparisons is as bad a practice in Python as it is in Java, although admittedly Python makes it unnecessarily hard to do it correctly. You want to use str.casefold(). \$\endgroup\$ – Voo Jan 6 at 13:12
  • \$\begingroup\$ To help with formatting issues like consistency in single or double quotes, I like to use black (other similar tools exist in Python as well). Combined with, e.g., precommit, this will handle a lot of little formatting things automatically whenever you commit. \$\endgroup\$ – Nathan Jan 6 at 16:49
  • \$\begingroup\$ @Voo thanks for the suggestion, I will look into it. I was intending to look into an equivalent to Java's equalsIgnoreCase() and this sounds like a step in the right direction \$\endgroup\$ – Roddy of the Frozen Peas Jan 6 at 19:53
21
\$\begingroup\$

Good points

  • Your code follows a lot of the suggested styles when using Python.
  • Typed Python. This is fairly new, but I've found it to be very helpful. Given that it supports gradual typing it also allows my dodgy metaprogramming to work fine too.
  • Docstrings, these seem good. You seem to have elected to use a Sphinx format. If you've not chosen a documentation generator then Sphinx looks like a good fit for you.
  • You seem to know about single and multi-line docstring styles.

Code Review

  • When comparing to singletons you should use is. Because the equality operator isn't guaranteed to perform the check you want it to.

    >>> False == 0
    True
    
  • It's more Pythonic to perform truthy and falsy checks.

    # if foo == True:
    if foo:
        ...
    
  • I'm unsure if you're a Java or JavaScript developer, but in either case you should know about the limitations of recursion. Recursion creates a stack frame for each and every call. If you call the function in itself then when you make the second frame, the first still exists. Once the second exits then the first resumes.

    This means that run_game can only run a finite amount of times. Implementing a main loop with recursion is pretty insensible.

  • run_game should be split into two functions, one that is in charge of the main loop and one for a hangman's main loop.
  • I would prefer a class to encapsulate state. I don't know much about Java but I've heard that it loves OOP, maybe wholly dedicated to OOP would better describe the relationship. But Python is different. If something can be better described as a class use a class. If it's better as a function use a function.

    Than again I read online that Python's butchered OOP and it's impossible to follow OOP in Python. Which is a pedantic misinterpretation at best, so maybe you're used to functional Java?

  • Your choice of random function, random.randint, is susceptible to cause the following indexing to IndexError. You should use random.randrange which doesn't include the end value.

  • It's better if you use random.choice rather than random.randint or random.randrange.
  • It doesn't matter if you use ' or " but stick to one. Using the other should only be used when the string contains your preferred delimiter, solely as a form of syntactic sugar.
  • You can use sorted to sort guessed_letters.
  • I would prefer to see a list without it wrapped in []. Just use str.join.
  • You can simplify your buggy check_guess_against_secret check by using sets. Given that you always display a sorted guessed list, there's not much point in having the guessed_letters as a list.
  • It's unPythonoic to use brackets around if statements. Unless you need them.
  • Don't fail silently in draw_hangman if the input is not between 0 and 6 then you should fix your broken code, not monkey patch the problem and pray to god it never reappears.
  • Calling print by default flushes the stream, and so is fairly expensive. The simple solution to this is print(..., flush=False). However, why not just build a string and print once? You can use a comprehension of sorts to make the code look nice too.
  • Rather than a do-while loop I would prefer a while True loop in play_again. You can just return to exit the function cleanly.
  • You should use a if __name__ == '__main__': guard to protect your code from accidentally running main.

Additional comments

Your naming convention seems poor to me.

  • You use both draw and print to mean the same thing. They're different words with different meanings.
  • You have play_again and prompt_for_guess. Both of the functions prompt for user input, but play_agin doesn't tell us that.
  • You have a function play_again that doesn't sound like a function. That sounds like it should just be a plain old Boolean variable.
  • Your names to me seem needlessly long, or cryptic. What are the benefits to:

    • secret_word over word or secret,
    • guessed_letters over guesses, or
    • run_game over main.
  • Furthermore I can't think of a short name for _check_guess_against_secret because it's doing two things. This breaks SRP.

Overall I think your code looks ok at a glance. You've got documentation, static typing and you've linted your code to be PEP 8 compliant. But I don't think your code is amazing when you actually look deeper into it.

import random
import sys
from typing import Tuple

# Omitted declarations: https://gist.github.com/chrishorton/8510732aa9a80a03c829b09f12e20d9c
# HANGMAN_STAGES = [...]
# WORDS = ...


class Hangman:
    _secret_word: str
    _guessed_letters: set
    _incorrect_guesses: int
    _round: int
    _won: bool

    def __init__(self, secret_word: str) -> None:
        self._secret_word = secret_word
        self._guessed_letters = set()
        self._incorrect_guesses = 0
        self._round = 1
        self._won = False

    def _turn(self) -> None:
        """
        Processes a user's turn. First draws the current state of the game: current hangman, partially-guessed word, and
        list of previously guessed letters. Then prompts the user for their next guess, evaluates that guess to see if it
        was correct, and then updates the game state.

        :param guessed_letters: the list of previously guessed letters
        """
        draw_hangman(incorrect_guess_count)
        self._draw_secret_word(secret_word, guessed_letters)
        self._print_guessed_letters()
        next_letter = self._prompt_for_guess()
        return self._apply_guess(next_letter)

    def _print_guessed_letters(self) -> None:
        """Print the guessed letters to the screen."""
        print('Guesses: ' + ', '.join(sorted(self.guessed_letters)))

    def _apply_guess(self, next_letter: str) -> None:
        """
        Checks the validity of the user's guess. If the guess was incorrect, increments the number of incorrect guesses by
        1. If the user has guessed all of the letters in the secret word, return an indication that the user has won the
        game.

        :param next_letter: the user's guess
        """
        self._guessed_letters.add(next_letter)
        correct, letters_remaining = self._check_guess_against_secret(next_letter, secret_word, guessed_letters)

        if not correct:
            self._incorrect_guess_count += 1

        if letters_remaining == 0:
            self._won = True

    def _check_guess_against_secret(self, next_letter: str) -> Tuple[bool, int]:
        """
        Determines if the user has guessed correctly. Also evaluates the secret word to determine if there are more letters
        left for the user to guess.

        :param next_letter: the user's guessed letter
        :return: (True/False indicating if the guess was correct, 0 if no letters left and positive integer otherwise)
        """
        return (
            next_letter in secret_word,
            len(set(self._secret_word) - self._guessed_letters)
        )

    def _prompt_for_guess(self) -> str:
        """
        Prompts the user for their next guess. Rejects guesses that are more than a single letter, and guesses which were
        already made previously. Returns the (validated) guess.

        :return: the user's next guess
        """
        while True:
            guess = input('Your guess? ').strip().upper()
            if len(guess) > 1:
                print('Sorry, you can only guess one letter at a time.')
                continue
            elif guess in guessed_letters:
                print('Sorry, you already guessed that letter.')
                continue
            return guess

    def _draw_secret_word(self) -> None:
        """
        Prints the secret word, with underscores representing unknown letters and with any correctly-guessed leters printed
        in the appropriate location within the word.

        :param secret_word: The secret word
        :param guessed_letters: All previous guesses
        :return: Nothing
        """
        print(
            ' '.join(
                letter if letter in self._guessed_letters else '_'
                for letter is self._secret_word
            )
            + '\n'
        )

    def run(self) -> None:
        while not self._won and self._incorrect_guesses < 6:
            print('\n\nROUND ' + str(round))
            self._turn()
            self._round += 1

        print('\n\n')

        if self._won:
            print('Congratulations! You won!', end=' ')
        else:
            print('GAME OVER! You lost.')
            draw_hangman(6)


def main() -> None:
    """The main game loop. Will prompt the user if they would like to start a new game at the end."""
    print('WELCOME TO HANGMAN.')
    while True:
        Hangman(pick_secret_word()).run()
        if not play_again():
            return


def pick_secret_word() -> str:
    """
    Chooses a new secret word from the list of available secret words. The word is chosen psuedo-randomly.
    :return: the new secret word
    """
    return random.choice(WORDS).upper()


def draw_hangman(number_of_incorrect_guesses: int) -> None:
    """
    Draws the appropriate hangman stage, given the number of incorrect guesses. 0 or fewer will draw the empty scaffold.
    6 or more will draw the fully hanged man.

    :param number_of_incorrect_guesses: the number of incorrect guesses the player has made in the current game
    :return: Nothing
    """
    if (number_of_guesses < 0
        or 6 < number_of_guesses
    ):
        raise ValueError('Hangman can only support upto 6 incorrect guesses.')
    print(HANGMAN_STAGES[number_of_guesses])


def play_again() -> bool:
    """
    Prompts the user if they would like to play again. If the user enters something other than Y/y/N/n, it will continue
    prompting until the use enters a valid value. If the user indicates Y or y, this method returns True; N or n will
    return False

    :return: True if the user would like to start a new game; False otherwise
    """
    while True:
        choice = input('Play again? (Y/N)').strip().upper()
        if choice in 'YN':
            return choice


if __name__ == '__main__':
    main()
\$\endgroup\$
  • \$\begingroup\$ Can you link me to something that elaborates on why one should prefer random.choice over randint and randrange? I've tried googling but I'm not getting anything definitive. When I Googled how to get a random integer when writing the original, both posts I looked at (one on StackOverflow, the other elsewhere) recommended randint, which is why I used it. \$\endgroup\$ – Roddy of the Frozen Peas Jan 6 at 3:59
  • \$\begingroup\$ With regards to using classes and sets ... I'm actually using a book to teach myself the Python concepts and haven't gotten to that part yet. I'll be sure to keep reading and do a refactor with your suggestions in mind. :) \$\endgroup\$ – Roddy of the Frozen Peas Jan 6 at 4:11
  • 4
    \$\begingroup\$ @RoddyoftheFrozenPeas In terms of why random.choice() over alternatives: it's mainly because of simplicity. Using rand.randint() and then choosing an index is two steps, and could lead to an IndexError depending on the circumstances/synchronicity. random.choice() is one step, is very straightforward (chooses one element from the list), and has no chance of IndexError unless the list is empty (again, only one step). "Take a random element from this list" is a cleaner and simpler expression than "Find a random index less than the size of the list, and take the element at that index". \$\endgroup\$ – Green Cloak Guy Jan 6 at 6:28
  • \$\begingroup\$ @RoddyoftheFrozenPeas: It is also very easy to introduce bias when rolling your own sampling functions; I would generally trust the standard library implementations more than my own. (Although of course, standard library implementations of PRNGs and sampling have been known to be wrong, and sometimes quite spectactularly so.) \$\endgroup\$ – Jörg W Mittag Jan 6 at 10:48
5
\$\begingroup\$

Handling input can be done with try to handle exceptions. For instance:

try:
    user_in = int(input('Enter number: ')
    ...
except ValueError:
    print('Integers only!')

Using quotations are your choice really but it is better to be consistent. If you use ' or " throughout your code make it one or the other. The benefits of using something like triple quote allow for simple data placement and calculations. In example:

print('''
data_1 > {}
data_2 > {}
data_3 > {}
'''.format(data_1, sum(data_1), len(data_1))

In regards to the secret word and what is left. Python is good for comprehension although it can be heavy on process time with more complex functions.

I saw a similar post not too long ago and it used classes to keep closely related functions together, which is what classes ideally are used for. I don't see the point in using them for a simple game, which would be something along the lines of:

import random

words = ['cheese', 'biscuits', 'hammer', 'evidence'] 
word_choice = [i.lower() for i in random.choice(words)] # List comprehension
lives = 3
cor_guess = []
while True:
  print('Letters found: {}\nWord length: {}'.format(cor_guess, len(word_choice)))
  if lives == 0:
      print('Your dead homie!\nThe word was: ', word_choice)
      break
  guess = str(input('Guess a letter: '))
  if guess in word_choice:
      print(guess, 'is in the word!\n')
      cor_guess.append(guess)
  else:
      print('Your dying slowly!\n')
      lives -= 1
  ...
  ...

Using list comprehension I have selected a object from a variable and converted it all to lowercase for simplicity if I need to open a file with capital letters in etc and then got every object of that object. This populates the list with one word separated in to separate letters. The data can then be compared to the user input. This way the data to begin with is very flexible and can be manipulated easily and it is much easier to follow the code.

\$\endgroup\$
  • \$\begingroup\$ In your try-catch example, the exception is raised if the input is not an integer. However all of my inputs are strings, so I don't really understand how I could go about using try-catch in this fashion unless I start raising the exceptions myself. Does Python really recommend using exception handling for control flow in this way? I've always understood this to be an anti-pattern and bad practice. \$\endgroup\$ – Roddy of the Frozen Peas Jan 6 at 4:10
  • 1
    \$\begingroup\$ docs.python.org/3/tutorial/errors.html Here is some documentation on try and except clauses. In the code above I have basically forced the user to input integers only and stay in the loop while checking that the conditions are met. Generally try/exceptions are used for specific conditional errors. It just saves time from creating a conditional type function. For instance not matter what you enter into str(input('')) it will be treated as a string. You then have to check if the input .isdigit() or convert it, handling it accordingly. Read some of the documentation its very helpful \$\endgroup\$ – Barb Jan 6 at 14:56
3
\$\begingroup\$

When presenting code, you should take the width of the code into consideration. The comment "Processes a user's turn. First draws the current state of the game: current hangman, partially-guessed word, and" is 116 characters long. The line of code "def process_turn(incorrect_guess_count: int, secret_word: str, guessed_letters: list) -> Tuple[int, bool]:" is 106 characters long. You can break function declarations into several lines:

def process_turn(
                     incorrect_guess_count: int, 
                     secret_word: str, 
                     guessed_letters: list
                 ) -> Tuple[int, bool]:

While Python allows you to have a function call another function that is defined later, it's easier to read if functions call functions that are previously defined.

You should use == to compare Boolean variables to constants. A == True just returns A and A == False returns not A.

Your pick_secret_word() function can be replaced by the single line 'secret_word = random.choice(WORDS).

Maybe it's personal taste, but I find the use of functions excessive. You spend a lot of your time passing parameters back and forth, and documenting what each parameter is.

If you use a for-loop rather than a while-loop for your rounds, you don't have to increment round. Also, round is a builtin function in Python, so you should use another name, such as turn. I'm using Spyder, which color codes builtins and keywords. If you're not using an IDE that does so, you might consider doing so.

6 is a "magic number". You can put it in as a parameter with a default value instead.

You should give the user more feedback about what's wrong if they don't give a valid response to whether they want to play again, or you could just take anything other than Y as a "no".

WORDS and HANGMAN_STAGES aren't defined anywhere.

def run_game(WORDS, HANGMAN_STAGES, max_turns = 26, max_guesses = 6) -> None:
    while True:
        print("WELCOME TO HANGMAN.")
        secret_word = random.choice(WORDS)
        guessed_letters = []
        incorrect_guesses = 0
        max_turns = 26
        max_guesses = 6
        letters_remaining = len(secret_word)
        for turn in max_turns:
            print('\n\nROUND ' + str(turn))    
            print(HANGMAN_STAGES[number_of_guesses])
            print(''.join([letter for letter in secret_word
                            if letter in guessed_letters 
                            else '_'])+'/n')
            guessed_letters.sort()
            print("Guesses: " + str(guessed_letters))
            while True:
                guess = input("Your guess? ").strip().upper()
                if len(guess) > 1:
                    print("Sorry, you can only guess one letter at a time.")
                    continue
                elif guess in guessed_letters:
                    print("Sorry, you already guessed that letter.")
                    continue
                break
            guessed_letters.append(guess)
            if guess in secret_word:
                if letters_remaining == 0:
                    print("\n\n")
                    print("Congratulations! You won!", end=" ")
                    break
                else:
                    letter_remaining -= 
                        sum([letter == guess for letter in secret_word])
            else:
                incorrect_guess +=1
                if incorrect_guesses == max_guesses:
                    print("\n\n")
                    print("GAME OVER! You lost.")
                    print(HANGMAN_STAGES[6]
                    break
        print("The secret word was: " + secret_word)
        choice = input("Press Y to play again").strip().upper()
        if choice != 'Y':
            break
\$\endgroup\$
  • \$\begingroup\$ I was using 120 characters as my line-length limit, since that's what I'm used to and what my IDE has set as a default. My understanding of PEP-8 is that the 80-character limit is a recommendation not a requirement. \$\endgroup\$ – Roddy of the Frozen Peas Jan 6 at 3:41
  • \$\begingroup\$ re: WORDS and HANGMAN_STAGES -- I mentioned in the original question that they're cribbed from a public Gist on GitHub and I omitted them intentionally because I had made no changes but renaming the variables, and didn't feel it was useful copy-pasting 60 lines of someone else's code (of indeterminate licensing). There's a link to the Gist in the post. \$\endgroup\$ – Roddy of the Frozen Peas Jan 6 at 4:13

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.