To learn Python, I've been working through Learn Python the Hard Way, and to exercise my Python skills I wrote a little Python Hangman game (PyHangman - creative, right?):
#! /usr/bin/env python2.7
import sys, os, random
if sys.version_info.major != 2:
raw_input('This program requires Python 2.x to run.')
sys.exit(0)
class Gallows(object):
def __init__(self):
'''Visual of the game.'''
self.state = [
[
'\t _______ ',
'\t | | ',
'\t | ',
'\t | ',
'\t | ',
'\t | ',
'\t________|_',
],
[
'\t _______ ',
'\t | | ',
'\t O | ',
'\t | ',
'\t | ',
'\t | ',
'\t________|_',
],
[
'\t _______ ',
'\t | | ',
'\t O | ',
'\t | | ',
'\t | | ',
'\t | ',
'\t________|_',
],
[
'\t _______ ',
'\t | | ',
'\t O | ',
'\t \| | ',
'\t | | ',
'\t | ',
'\t________|_',
],
[
'\t _______ ',
'\t | | ',
'\t O | ',
'\t \|/ | ',
'\t | | ',
'\t | ',
'\t________|_',
],
[
'\t _______ ',
'\t | | ',
'\t O | ',
'\t \|/ | ',
'\t | | ',
'\t / | ',
'\t________|_',
],
[
'\t _______ ',
'\t | | ',
'\t O | ',
'\t \|/ | ',
'\t | | ',
'\t / \\ | ',
'\t________|_',
]
]
def set_state(self, misses):
'''Sets the current visual being used.'''
image = ''
state = self.state[misses] # set state to the list of desired gallows image
# construct gallows image into str from list
for piece in state:
image += piece + '\n'
return image
class Wordlist(object):
def __init__(self):
'''Set the length of the wordlist.'''
self.numLines = sum(1 for line in open('test.txt'))
def new_word(self):
'''Choose a new word to be guessed.'''
stopNum = random.randint(0, self.numLines-1) # establish random number to be picked from list
# extract word from file
with open('test.txt') as file:
for x, line in enumerate(file):
if x == stopNum:
word = line.lower().strip() # remove endline characters
return word
def set_blanks(word):
'''Create blanks for each letter in the word.'''
blanks = []
for letter in word:
# Don't hide hyphens
if letter == '-':
blanks += '-'
else:
blanks += '_'
return blanks
def check_letter(word, guess, blanks, used, missed):
'''Check if guessed letter is in the word.'''
newWord = word
# If the user presses enter without entering a letter
if guess.isalpha() == False:
raw_input("You have to guess a letter, silly!")
# If the user inputs multiple letters at once
elif len(list(guess)) > 1:
raw_input("You can't guess more than one letter at a time, silly!")
# If the user inputs a letter they've already used
elif guess in used:
raw_input("You already tried that letter, silly!")
# replace the corresponding blank for each instance of guess in the word
elif guess in word:
for x in range(0, word.count(guess)):
blanks[newWord.find(guess)] = guess
newWord = newWord.replace(guess, '-', 1) # replace already checked letters with dashes
used += guess # add the guess to the used letter list
#If the guess is wrong
else:
missed = True
used += guess
return blanks, used, missed
def new_page():
'''Clears the window.'''
os.system('cls' if os.name == 'nt' else 'clear')
def reset(image, wordlist):
wrongGuesses = 0 # number of incorrect guesses
currentImg = image.set_state(wrongGuesses) # current state of the gallows
word = wordlist.new_word() # word to be guessed
blanks = set_blanks(word) # blanks which hide each letter of the word until guessed
used = [] # list of used letters
return wrongGuesses, currentImg, word, blanks, used
def play(image, currentImg, wrongGuesses, word, blanks, used):
missed = False
new_page()
print currentImg
for x in blanks:
print x,
print '\n'
for x in used:
print x,
print '\n'
guess = raw_input("Guess a letter: ")
blanks, used, missed = check_letter(word, guess.lower(), blanks, used, missed)
if missed == True and wrongGuesses < 6:
wrongGuesses += 1
currentImg = image.set_state(wrongGuesses)
play(image, currentImg, wrongGuesses, word, blanks, used)
elif missed == False and blanks != list(word) and wrongGuesses != 7:
play(image, currentImg, wrongGuesses, word, blanks, used)
elif blanks == list(word):
endgame('win', word)
else:
endgame('lose', word)
def endgame(result, word):
new_page()
if result != 'lose':
print "Congratulations, you win!"
print "You correctly guessed the word '%s'!" % word
else:
print "Nice try! Your word was '%s'." % word
while True:
play_again = raw_input("Play again? [y/n]")
if 'y' in play_again.lower():
return
elif 'n' in play_again.lower():
sys.exit(0)
else:
print "Huh?"
def main():
image = Gallows()
wordlist = Wordlist()
new_page()
print("\nWelcome to Hangman!")
print("Guess the word before the man is hung and you win!")
raw_input("\n\t---Enter to Continue---\n")
new_page()
while True:
misses, currentImg, word, blanks, used = reset(image, wordlist)
play(image, currentImg, misses, word, blanks, used)
if __name__ == '__main__':
main()
So what do you all think? This is the second iteration of the program, as the first one was a mess, but I think I've got it all worked out a bit better now.
One of the things that I still think is messy is how I had the game loop after you guessed a letter, as it simply calls itself again. Is there a better way to do that?
Any tips on making my code more pythonic is appreciated!
-
\$\begingroup\$ A bit of a naive question why is '\' escaped in the right leg but not in the left arm ? \$\endgroup\$SylvainD– SylvainD2014年03月03日 17:14:40 +00:00Commented Mar 3, 2014 at 17:14
-
\$\begingroup\$ Hm, good question. I'm not sure why I originally did that, but removing it doesn't seem to break anything. \$\endgroup\$ananaso– ananaso2014年03月03日 17:45:15 +00:00Commented Mar 3, 2014 at 17:45
-
\$\begingroup\$ Here's a followup question. \$\endgroup\$ananaso– ananaso2014年04月15日 16:56:44 +00:00Commented Apr 15, 2014 at 16:56
2 Answers 2
I'll get a few trivial comments out of the way before addressing some serious concerns about the flow of control in your program.
User experience
Overall, the application feels like it's nicely put together. I enjoyed playing it while writing this review.
- The past participle of the verb "to hang" is "hung", but when talking about executions, it should be "hanged". ("Hung" isn't completely unacceptable usage, though.)
endgame()
should not clear the screen. For me, part of the fun of playing Hangman is to see the final state of the game, and clearing the screen denies me that pleasure.
Python 3 compatibility
You explicitly check that the game is running on Python 2. With a few changes, all related to input()
and print()
, you can also get it to run on Python 3.
Put the following shim at the top, then replace all
raw_input()
calls withinput()
.if sys.version_info.major < 3: # Compatibility shim input = raw_input
Replace all
print "string"
withprint("string")
However, the following printing code, which uses a trailing comma to suppress the newline in Python 2, requires special treatment:
for x in blanks: print x, print '\n' for x in used: print x, print '\n'
I suggest the following replacement code, which takes a better approach even if you weren't interested in Python 3:
print(' '.join(blanks)) print(' '.join(used))
Wordlist
- Buried in the middle of a long program, you hard-coded
"test.txt"
. That's bad for maintainability; hard-coding the same filename twice is even worse. Instead, theWordList
constructor should take a filename parameter. - You might have a file descriptor leak in the constructor, since you
open()
the file not using awith
block or callingclose()
. Therefore, you rely on the garbage collector to trigger the closing, which won't necessarily happen in all Python interpreters. - No need to assign the variable
word
. Justreturn line.lower().strip()
.
Gallows
Gallows.set_state()
is a misnomer. TheGallows
object doesn't actually keep any state! A more accurate name would beGallows.get_image()
.- I would prefer it if
Gallows
did keep track of the number of wrong guesses, and had methods.increment_count()
and.is_hanged()
. Currently, you hard-code 6 as a limit inplay()
, a number that you hope is consistent with the number of images available inGallows
(minus one for the initial image).
Interaction between reset()
, check_letter()
, and play()
- You pass clusters of variables around a lot. Those are actually state variables, so these three functions would be better as members of a class.
check_letter()
should not takemissed
as a parameter. It's an output, not an input.The way
check_letter()
handles correct guesses is clumsy. I would write# replace the corresponding blank for each instance of guess in the word elif guess in word: for index, char in enumerate(word): if char == guess: blanks[index] = guess used += guess # add the guess to the used letter list
Flow of control
All the critiques above are minor details. The most important problem with the code, in my opinion, is that you are misusing recursive function calls as a kind of goto.
To see a symptom of the problem, play a few rounds, then hit CtrlC. Notice that the stack trace is very deep — it contains one call to play()
for every guess ever made in the history of the program, including previous rounds. It is inappropriate to keep such state around in the call stack.
The remedy is to use loops for looping.
Here's how I would write it (without making too many of the recommended changes listed above).
def play(word):
""" Play one game of Hangman for the given word. Returns True if the
player wins, False if the player loses. """
gallows = Gallows()
wrong_guesses = 0 # number of incorrect guesses
blanks = set_blanks(word) # blanks which hide each letter of the word until guessed
used = [] # list of used letters
while True:
new_page()
print(gallows.get_image(wrong_guesses))
print(' '.join(blanks))
print(' '.join(used))
guess = input("Guess a letter: ")
blanks, used, missed = check_letter(word, guess.lower(), blanks, used)
if blanks == list(word):
return endgame(True, word)
elif missed and wrong_guesses >= 6:
return endgame(False, word)
elif missed:
wrong_guesses += 1
def endgame(won, word):
print('')
if won:
print("Congratulations, you win!")
print("You correctly guessed the word '%s'!" % word)
else:
print("Nice try! Your word was '%s'." % word)
return won
def play_again():
while True:
play_again = input("Play again? [y/n] ")
if 'y' in play_again.lower():
return True
elif 'n' in play_again.lower():
return False
else:
print("Huh?")
def main(words_file='test.txt'):
wordlist = Wordlist(words_file)
new_page()
print("\nWelcome to Hangman!")
print("Guess the word before the man is hanged and you win!")
input("\n\t---Enter to Continue---\n")
new_page()
while True:
play(wordlist.new_word())
if not play_again():
break
Python supports multi-line string literals. You could use those instead of using a list to encode your state image.
a = r""" 'hello' \n
more """
print a
You have multiple methods called set_XXX()
that return values instead of storing a value. The names of these methods could be changed to better describe what they are doing.
Wordlist.new_word()
will continue to iterate over the entire file even after the selected word is found. Putting the return
where you create the word
variable will break the loop once you have the value you want.
Wordlist
uses a hard coded string for the file name, that is specified in two different places. If you want to change to a different file, you have to remember to change both cases. This is a simple example, that may seem trivial, but this type of error is hard to track down when the code becomes larger and more complicated.