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()