While I'm not new to Python, this is my first 'bigger' Python program I created, also my first game in Python.
import random
DEBUG = False; # If set to true, displays chosen word before each game. For debugging and cheating only :)
DICT_FILE = "english.txt";
GUESSES = 10;
def prompt(s):
while True:
ans = raw_input(s + " (Y/n) ").lower();
if ans in ["y", "n"]:
return ans == "y";
def loadWords():
with open(DICT_FILE, "r") as f:
return f.read().strip().split("\n");
# Strip in case of newline at end of file
def play(word):
word = word.lower();
if DEBUG:
print "[DEBUG] word is %r" % (word);
guesses = GUESSES;
toGuess = set(l for l in word.upper() if l.isalpha()); # Handles apostrophes, etc.
letters = set();
while guesses > 0:
if len(toGuess) == 0:
print "You win with %d guess%s left!" % (guesses, "" if guesses == 1 else "es");
print "The word is %r" % (word);
return True;
print "=" * 78;
print "You have %d guess%s left." % (guesses, "" if guesses == 1 else "es");
print "Word: " + " ".join("_" if c in toGuess else c for c in word.upper());
print "Letters: " + " ".join(sorted(letters));
while True:
l = raw_input("Choose a letter: ").upper();
if l.isalpha():
if l not in letters:
letters.add(l);
if l in toGuess:
toGuess.remove(l);
print "Correct!";
else:
guesses -= 1;
print "Sorry, not in the word.";
break;
else:
print "You already chose that letter!";
else:
print "That's not a letter!";
print "=" * 78;
print "Sorry, you lose.";
print "The word was: %r" % (word);
return False;
if __name__ == "__main__":
words = loadWords();
random.shuffle(words);
while True:
play(words.pop()); # Remove a random word so it's not chosen again
if not prompt("Would you like to play again?"):
break;
I'd like to know what you think and what I should change.
By the way, english.txt
placed in the same folder as the game script contains phrases/words separated by newlines that should appear in the hangman game.
6 Answers 6
I'm not the first person to wonder about those semicolons, but I might be the first to explain what they do!
They are used to teminate a line early, so that you can put two logical lines on the same physical one, such as in:
a = 1; b = 2
This is normally frowned upon (this in particular should be written a, b = 1, 2
) and use where not needed is a very dangerous sight to the untrained Pythonista. Use with care.
From the the prompt
function:
The parameter's name should be something meaningful like
question
.Writiting
(Y/n)
with theY
in capitals has the standard meaning of "defaults to yes". If this is not wanted you should use(y/n)
.Stylistically, you should either use a tuple or a set for
if ans in ["y", "n"]
.
I would do:
def prompt(question):
while True:
ans = raw_input(question + " (Y/n) ") or "y"
if ans.lower() in ("y", "n"):
return ans == "y"
For loadWords
:
The name breaks convention; it should probably be
load_words
.It should not use the global directly, or it should use it as a default parameter.
You do too much work inside the
with
after you finish reading it. It's a good habit to minimize the are under contexts, whether they betry
s orwith
s.Don't read everything and then split it; utilize files being iterable.
What's up with the positioning of that comment?
You don't need the
"r"
parameter toopen
; it's the default.
I would do:
def loadWords(filename="english.txt"): with open(filename) as f: return [line.strip() for line in f if line]
and ditch the global.
For play
The function is too long; a standard function should be a little smaller than this. It's OK to have longer functions, but they need to be justified (for example, implementing a non-trivial algorithm).
The function does not have a single responsibility; it does both output and dealing with minor concerns like
if l.isalpha()
."[DEBUG] word is %r" % (word)
strangely hasword
in brackets. You should remove the brackets; they do nothing. Sometimes it makes sense to wrap it in a tuple ((word,)
), but this is not what you did. IMHO it's even better to justprint "[DEBUG] word is", repr(word)
.You should probably avoid using an overarching
while guesses > 0
loop; awhile True
with appropriate exits would be clearer since it would not imply that there is only one standard exit method.toGuess
should beto_guess
, or IMHO justunguessed
.You can use a set comprehension for
unguessed
:{l for l in word.upper() if l.isalpha()}
.You're not checking for the length of the guess.
letters
should be named something likeguessed
."{}".format
is IMHO better than the old-style%s
formatting.
I would thus choose something like:
import random
# If set to true, displays chosen word before each game.
# For debugging and cheating only :)
DEBUG = False
GUESSES = 10
def loadWords(filename="english.txt"):
with open(filename) as file:
return [line.strip() for line in file if line]
def prompt(question):
while True:
ans = raw_input(question + " (Y/n) ") or "y"
if ans.lower() in {"y", "n"}:
return ans == "y"
def get_guess(guessed):
while True:
guess = raw_input("Choose a letter: ").upper()
if not guess.isalpha() or len(guess) != 1:
print "That's not a letter!"
continue
if guess in guessed:
print "You have already chosen that letter!"
continue
return guess
def alert_turn(word, guesses_left, guessed, unguessed):
print "=" * 78
print "You have {} guess{} left.".format(guesses_left, "" if guesses_left == 1 else "es")
print "Word: ", " ".join("_" if c in unguessed else c for c in word.upper())
print "Letters:", " ".join(sorted(guessed))
def alert_correct():
print "Correct!"
def alert_incorrect():
print "Sorry, not in the word."
def alert_won(word, guesses_left):
print "You win with {} guess{} left!".format(guesses_left, "" if guesses_left == 1 else "es")
print "The word is", repr(word)
def alert_lost(word):
print "=" * 78
print "Sorry, you lose."
print "The word was:", repr(word)
def play(word):
word = word.lower()
guessed = set()
unguessed = {l for l in word.upper() if l.isalpha()}
guesses_left = GUESSES
if DEBUG:
print "[DEBUG] word is", repr(word)
while True:
alert_turn(word, guesses_left, guessed, unguessed)
guess = get_guess(guessed)
guessed.add(guess)
if guess in unguessed:
alert_correct()
unguessed.remove(guess)
if not unguessed:
alert_won(word, guesses_left)
return True
else:
alert_incorrect()
guesses_left -= 1
if not guesses_left:
alert_lost(word)
return False
if __name__ == "__main__":
words = loadWords()
random.shuffle(words)
while True:
# Remove a random word so it's not chosen again
play(words.pop())
if not prompt("Would you like to play again?"):
break
This is Python and you do not need the Semi-Colons (;
)
Your code looks absolutely non-pythonic like that, it looks like it is trying to morph into Java or something.
You should also separate some more code into methods/functions like this one here
print "=" * 78;
I believe that this is to separate the game from the ending notifications.
What gave this away was 78
I have no clue what that is, and if I have different settings on my console/IDE then this won't look right anyway. 78
is a magic number and should be dealt with so that I know what it means
After a quick test, your code seems to be working fine.
Before going into interesting comments, let's go through quick details. Python has a style guide called PEP8. The document is definitly worth a read. Except if you have good reason not to, you should probably comply to this guideline. You'll find various tools to check you code like pep8
or to fix it autopep8
.
Here, pep8
gives :
hangman.py:3:14: E703 statement ends with a semicolon
hangman.py:3:15: E261 at least two spaces before inline comment
hangman.py:3:80: E501 line too long (106 > 79 characters)
hangman.py:5:26: E703 statement ends with a semicolon
hangman.py:6:8: E221 multiple spaces before operator
hangman.py:6:15: E703 statement ends with a semicolon
hangman.py:11:47: E703 statement ends with a semicolon
hangman.py:13:30: E703 statement ends with a semicolon
hangman.py:15:1: E302 expected 2 blank lines, found 1
hangman.py:17:44: E703 statement ends with a semicolon
hangman.py:18:24: E111 indentation is not a multiple of four
hangman.py:18:24: E113 unexpected indentation
hangman.py:20:1: E302 expected 2 blank lines, found 1
hangman.py:21:24: E703 statement ends with a semicolon
hangman.py:23:44: E703 statement ends with a semicolon
hangman.py:24:22: E703 statement ends with a semicolon
hangman.py:25:58: E703 statement ends with a semicolon
hangman.py:25:59: E261 at least two spaces before inline comment
hangman.py:25:80: E501 line too long (86 > 79 characters)
hangman.py:26:20: E703 statement ends with a semicolon
hangman.py:29:80: E501 line too long (92 > 79 characters)
hangman.py:29:92: E703 statement ends with a semicolon
hangman.py:30:44: E703 statement ends with a semicolon
hangman.py:31:24: E703 statement ends with a semicolon
hangman.py:32:23: E703 statement ends with a semicolon
hangman.py:33:80: E501 line too long (84 > 79 characters)
hangman.py:33:84: E703 statement ends with a semicolon
hangman.py:34:80: E501 line too long (87 > 79 characters)
hangman.py:34:87: E703 statement ends with a semicolon
hangman.py:35:54: E703 statement ends with a semicolon
hangman.py:37:55: E703 statement ends with a semicolon
hangman.py:40:35: E703 statement ends with a semicolon
hangman.py:42:42: E703 statement ends with a semicolon
hangman.py:43:41: E703 statement ends with a semicolon
hangman.py:45:37: E703 statement ends with a semicolon
hangman.py:46:56: E703 statement ends with a semicolon
hangman.py:47:26: E703 statement ends with a semicolon
hangman.py:49:59: E703 statement ends with a semicolon
hangman.py:51:45: E703 statement ends with a semicolon
hangman.py:52:19: E703 statement ends with a semicolon
hangman.py:53:29: E703 statement ends with a semicolon
hangman.py:54:38: E703 statement ends with a semicolon
hangman.py:55:17: E703 statement ends with a semicolon
hangman.py:58:24: E703 statement ends with a semicolon
hangman.py:59:26: E703 statement ends with a semicolon
hangman.py:61:26: E703 statement ends with a semicolon
hangman.py:61:27: E261 at least two spaces before inline comment
hangman.py:63:18: E703 statement ends with a semicolon
Also, the names do not follow the naming convention.
After fixing this, the code looks like :
import random
DEBUG = False # If set to true, displays chosen word before each game. For debugging and cheating only :)
DICT_FILE = "english.txt"
GUESSES = 10
def prompt(s):
while True:
ans = raw_input(s + " (Y/n) ").lower()
if ans in ["y", "n"]:
return ans == "y"
def load_words():
with open(DICT_FILE, "r") as f:
return f.read().strip().split("\n") # Strip in case of newline at end of file
def play(word):
word = word.lower()
if DEBUG:
print "[DEBUG] word is %r" % (word)
guesses = GUESSES
to_guess = set(l for l in word.upper() if l.isalpha()) # Handles apostrophes, etc.
letters = set()
while guesses > 0:
if len(to_guess) == 0:
print "You win with %d guess%s left!" % (guesses, "" if guesses == 1 else "es")
print "The word is %r" % (word)
return True
print "=" * 78
print "You have %d guess%s left." % (guesses, "" if guesses == 1 else "es")
print "Word: " + " ".join("_" if c in to_guess else c for c in word.upper())
print "Letters: " + " ".join(sorted(letters))
while True:
l = raw_input("Choose a letter: ").upper()
if l.isalpha():
if l not in letters:
letters.add(l)
if l in to_guess:
to_guess.remove(l)
print "Correct!"
else:
guesses -= 1
print "Sorry, not in the word."
break
else:
print "You already chose that letter!"
else:
print "That's not a letter!"
print "=" * 78
print "Sorry, you lose."
print "The word was: %r" % (word)
return False
if __name__ == "__main__":
words = load_words()
random.shuffle(words)
while True:
play(words.pop()) # Remove a random word so it's not chosen again
if not prompt("Would you like to play again?"):
break
Now, for the actual comments.
It would probably be worth writing a function to get the new letter. Something like :
def prompt_new_letter(s, letters):
while True:
l = raw_input(s).upper()
if l.isalpha():
if l not in letters:
return l
else:
print "You already chose that letter!"
else:
print "That's not a letter!"
...
l = prompt_new_letter("Choose a letter: ", letters)
letters.add(l)
if l in to_guess:
to_guess.remove(l)
print "Correct!"
else:
guesses -= 1
print "Sorry, not in the word."
Not much else to say at the moment.
-
\$\begingroup\$ I was thinking about using a new function for asking for another letter, but I didn't think I should since I use the set of letters in the code. Anyway, I'll (leave the original code and) add the final, refactored code to my question once I think there is nothing else to say. \$\endgroup\$Mateon1– Mateon12014年12月22日 16:28:40 +00:00Commented Dec 22, 2014 at 16:28
-
1\$\begingroup\$ It is much a matter of preference to create or not a function here. I think having the new method separates the concern : you have the input logic outside the fame logic. As for the update of your question, "You also should not append your revised code to the question" as per meta.codereview.stackexchange.com/questions/1763/… . \$\endgroup\$SylvainD– SylvainD2014年12月22日 16:33:02 +00:00Commented Dec 22, 2014 at 16:33
-
\$\begingroup\$ Thank you for linking me that meta post. I'll combine both of the answers with everything I found helpful. \$\endgroup\$Mateon1– Mateon12014年12月22日 16:37:55 +00:00Commented Dec 22, 2014 at 16:37
First, I would get rid of the semicolons and the extra blank lines; they're distracting. A single blank line should be enough to break things up.
Other than that, the play
function is a bit too long. Any time you have that much code in a single function or method, it's time to break it up a bit.
And as an aside, it seems a little odd to me that you first convert the word to lowercase, and then use uppercase for the actual letters. Seems like you could use just the one case conversion there.
From what I've seen answered so far, my main issue is using semicolons after every statement and not following pep8
. I've fixed this in my code by:
- removing the semicolons, as Malachi and Josay pointed out,
- formatting the comments to follow the pep8 guide,
- Changing
print "=" * 78
toprint "==="
, a three character wide separator is enough, while removing magic numbers. The separator will now look good in any console/terminal, (As pointed out by Malachi), I don't think creating a new function is worth it, because this is a simple statement used only twice. - Renamed all word-related variables to phrase, the game can handle any phrase, not just a single word.
- As Mark and Josay pointed out, my play() function is a bit too long. I broke it up a bit.
- Mark pointed out, that I needlessly convert my phrase to lowercase, only to convert it to uppercase later. I changed it to be stored as uppercase.
Here's my final code:
import random
# If set to true, displays chosen word before each game.
# For debugging and cheating only :)
DEBUG = False
DICT_FILE = "english.txt"
GUESSES = 10
def prompt(s):
while True:
ans = raw_input(s + " (Y/n) ").lower()
if ans in ["y", "n"]:
return ans == "y"
def read_phrases():
with open(DICT_FILE, "r") as f:
return f.read().strip().split("\n")
# Strip before splitting in case of newline at end of file
def prompt_letter(letters_so_far):
while True:
c = raw_input("Choose a letter: ").upper()
if c.isalpha():
if c not in letters_so_far:
return c
else:
print "You already chose that letter!"
else:
print "That's not a letter!"
def play(phrase):
phrase = phrase.upper()
if DEBUG:
print "[DEBUG] phrase is %r" % (word)
guesses = GUESSES
to_guess = set(l for l in phrase if l.isalpha()) # Handles apostrophes, etc.
letters = set()
while guesses > 0:
if len(to_guess) == 0:
print "You win with %d guess%s left!" % (guesses, "" if guesses == 1 else "es")
print "The phrase is %r" % (phrase)
return True
print "==="
print "You have %d guess%s left." % (guesses, "" if guesses == 1 else "es")
print "Phrase: " + " ".join("_" if c in to_guess else c for c in phrase)
print "Letters: " + " ".join(sorted(letters))
while True:
c = prompt_letter(letters)
letters.add(c)
if c in to_guess:
to_guess.remove(c)
print "Correct!"
else:
guesses -= 1
print "Sorry, not in the phrase."
break
print "==="
print "Sorry, you lose."
print "The phrase was: %r" % (phrase)
return False
if __name__ == "__main__":
phrases = read_phrases()
random.shuffle(phrases)
while True:
play(phrases.pop()) # Remove a random phrase so it's not chosen again
if not prompt("Would you like to play again?"):
break
- Removed all semicolons - they are not required in Python
- Removed all parentheses around the
word
variable as they're unnecessary - Renamed the
toGuess
variable inplay()
toto_guess
- Renamed the
loadWords
function toload_words
- Created new
separator
andmsg
functions to clean up print statements in code
To expand a bit, we are renaming the function and variables in order to align with PEP8 naming conventions.
Here's the code:
import random
DEBUG = False # If set to true, displays chosen word before each game.
DICT_FILE = "english.txt"
GUESSES = 10
def prompt(s):
while True:
ans = raw_input(s + " (Y/n) ").lower()
if ans in ["y", "n"]:
return ans == "y"
def load_words():
with open(DICT_FILE, "r") as f:
return f.read().strip().split("\n") # Strip in case of newline at end of file
def separator():
print "=" * 78
def msg(*message):
for line in message:
print line
def play(word):
word = word.lower()
if DEBUG:
print "[DEBUG] word is %r" % word
guesses = GUESSES
to_guess = set(l for l in word.upper() if l.isalpha()) # Handles apostrophes, etc.
letters = set()
while guesses > 0:
if len(to_guess) == 0:
msg("You win with %d guess%s left!" % (guesses, "" if guesses == 1 else "es"), "The word is %r" % word)
return True
separator()
msg("You have %d guess%s left." % (guesses, "" if guesses == 1 else "es"),
"Word: " + " ".join("_" if c in to_guess else c for c in word.upper()),
"Letters: " + " ".join(sorted(letters)))
while True:
l = raw_input("Choose a letter: ").upper()
if l.isalpha():
if l not in letters:
letters.add(l)
if l in to_guess:
to_guess.remove(l)
msg("Correct!")
else:
guesses -= 1
msg("Sorry, not in the word.")
break
else:
msg("You already chose that letter!")
else:
msg("That's not a letter!")
separator()
msg("Sorry, you lose.", "The word was: %r" % word)
return False
if __name__ == "__main__":
words = load_words()
random.shuffle(words)
while True:
play(words.pop()) # Remove a random word so it's not chosen again
if not prompt("Would you like to play again?"):
break
-
\$\begingroup\$ I am not quite sure to see the point of
if len(message) == 0: return False
. If the length is 0, the function wouldn't do much anyway, would it ? \$\endgroup\$SylvainD– SylvainD2014年12月22日 16:57:12 +00:00Commented Dec 22, 2014 at 16:57 -
\$\begingroup\$ could you explain in your answer why you would rename those variables, it would make for a better review. \$\endgroup\$Malachi– Malachi2014年12月22日 17:29:59 +00:00Commented Dec 22, 2014 at 17:29
-
1\$\begingroup\$ @Josay Good point - it's a personal preference of mine to return early if there's nothing to work with...a bit of a leftover from another language I work with :) removing \$\endgroup\$jsanc623– jsanc6232014年12月22日 17:51:51 +00:00Commented Dec 22, 2014 at 17:51
;
)? \$\endgroup\$from __future__ import braces
\$\endgroup\$