Skip to main content
Fix spelling
Source Link
Peilonrayz
  • 43.5k
  • 7
  • 76
  • 155
  • 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 inchargein 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 preferedpreferred delimiter, solely as a form of syntaticsyntactic 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 statmentsstatements. 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 accedentallyaccidentally running main.

  • 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 booleanBoolean variable.

  • Your names to me seem needlessly long, or cryptic. What are the benifitsbenefits 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.

  • 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 incharge 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 prefered delimiter, solely as a form of syntatic 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 statments. 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 accedentally running main.

  • 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 benifits 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.

  • 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.

  • 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.

Source Link
Peilonrayz
  • 43.5k
  • 7
  • 76
  • 155

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 incharge 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 prefered delimiter, solely as a form of syntatic 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 statments. 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 accedentally 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 benifits 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()