2
\$\begingroup\$

This is my take on a simple hangman game, which is played from command prompt.

import random

wordlist = []

word =  random.choice(open("words.txt").readlines()) 
#word.txt is a list of words, in the same directory with the game file.

length  = len(word) 

mock_word = "_ " * length 
#The underscores presented to the player

mock_word =  mock_word.split() 
#making it a list, since strings can't be assigned

used_lets = []

lives = 7

while True:
    print " ".join(mock_word) #Needs space in join, so the underscores are separated.
    let = raw_input("Give a letter: ")

    if len(let) > 1: #User can only give one letter
        print "Only one!"

    elif (let not in word):
        if let not in used_lets:
            lives = lives - 1 
            if lives == 0:
                print "Sorry you lost, the word was:", word
                break
            print "Try again, you have only %r lives left!" % lives 
            used_lets.append(let)
            print "So far you have used:"
            print used_lets 

    else:
        for i in range(length):
            if let == word[i]:
                mock_word[i] = let

    if "".join(mock_word) == word: #Making it a string again
        print word + " You won!"
        break

If you want to test, just set word to something. That is how I tested the loop. I would appreciate criticisms of my coding technique, and perhaps the suggestion of an alternative of the while loop use, which feels wrong.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! While your post is on-topic, you could make it stand out more: perhaps you could add some console output of you playing a sample game. And, you could include specifically what you would like in a review. \$\endgroup\$ – SirPython Sep 23 '15 at 19:49
2
\$\begingroup\$

tl;dr: This is pretty good for a beginner. It’s robust to bad input and plays smoothly. To improve it, you could use functions to make it more reusable, and I’ve suggested a bunch of small ways to tidy up your Python. But a high level it works well.


I’m just going to dive in, run the script, and see what happens. You didn’t provide a words.txt file, so I just used this small sample:

american
captain
dictionary
fish
hangman
letter
list

Comments:

  • Overall, it seems pretty robust. I was unable to cause a traceback or exception by passing in dodgy input – kudos! 👍

    (And yes, I passed in an emoji thumbs-up, and it handled it fine.)

  • At the risk of getting bludgeoned to death by Jeff Atwood, I found a pluralisation bug:

    Try again, you have only 1 lives left!
    So far you have used:
    ['b', 'c', 'd', 'e', 'f', 'i']
    

    Should be “lives”.

  • The program treats uppercase and lowercase letters as distinct. That’s not what I expect when I play Hangman – at least whenever I’m played it, I’m just guessing letters, not their associated case as well.

  • There are a few cases where the program just asks for the next letter, and doesn’t give a helpful message. When I guess a letter that I’ve already guessed (correct or not), it goes straight to prompting me for a new letter without explaining why.

    That’s more adding polish than fixing a bug – but it would be nice.

  • I think a newline between guesses would make the output much easier to read. That’s a personal preference though, not a bug.


At a high-level, the problem with this code is that it isn’t very reusable. It’s good to break your code down into functions – that allows it to be reused within the same file, or across other files.

For example, this script only allows me to play a single game of Hangman. Having a play_hangman() function would allow you to keep playing Hangman until the player got bored. Here are some suggestions to get you started:

  • play_hangman(word, number_of_lives)
  • get_random_word_from_file(file_path) – letting me specify the file path means I could drop in another word list easily

I think the while loop is fine for this purpose.

Once you have some functions in the file, you should use the if __name__ == '__main__' construction, which allows the same file to be used both as a module and a script. Like so:

if __name__ == '__main__':
    play_hangman('pomegranate', number_of_lives=8)

The code inside that if block only runs if the script is run directly; if you access part of the file via an import statement, it doesn’t run. That means you can import from the file without having to play a game of Hangman first.


Finally, here are some minor line-by-line comments and/or nitpicks:

  • You’re pretty good for PEP 8 compliance. One line is a bit long, and you need a bit of extra spacing around your comments, but otherwise fine.

  • The wordlist variable is unused.

  • When you choose the word, you open the words.txt file, but don't close it again. You could add an explicit close, but a better approach is to use the with keyword when opening files, like so:

    with open("words.txt") as wordsfile:
        word = random.choice(wordsfile.readlines())
    

  • The length variable is only used twice (and I’m going to get rid of the second usage shortly), so I’d remove it and just use it directly.

  • The way you’ve constructed the mock_word list is a little strange. You could go straight to it in one line:

    mock_word = ['_'] * len(word)
    

    Also note that the word “mock” has a special meaning in programming, so I’d suggest picking a different name for this variable – perhaps player_guess?

  • Don’t be stingy with variable names – characters are cheap. Names like used_lets and let can become used_letters and guessed_letter. It makes your program easier to read.

Now I’m diving into the heart of the program: the while loop that processes the player guess.

  • The first if branch is fine. However, you’re only checking that I don’t enter too many characters; you don’t check that I enter enough characters. This loop could be improved by checking for an incorrect number of characters, and printing a generic message:

    if len(letter) != 1:
        print("Please enter a single character.")
    
  • In the elif branch, a common way to decrement a variable is to write lives -= 1. That’s a bit more compact.

    In the print statement telling me how many lives I have left, you’re using the %r token. That’s fairly generic – it just prints the string representation of whatever gets passed in. Since you know that the lives variable is an integer, I think it’s better to use the more explicit token %d. As I read the string, I know what sort of data will appear here.

  • In the else branch, a more Pythonic approach than iterating over the indexes is to use enumerate(), which iterates over the index and the array element together. Like so:

    for i, letter in enumerate(word):
        if guessed_letter == letter:
            player_guess[i] = letter
    
| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

First off I feel cheated, there is no hanging man.


Your code is really good! It follows PEP8 pretty well. But you came here for a review.

First, wordlist is not used and mock_word is instantly overwritten too. You can change most of this into a few small lines.

word =  random.choice(open("words.txt").readlines())
length = len(word)
mock_word = ("_ " * length).split()
used_letters = []
lives = 7

In-line comments are annoying to read.

print " ".join(mock_word) #Needs space in join, so the underscores are separated.

#Needs space in join, so the underscores are separated.
print " ".join(mock_word)

Which was easier to read? I vote the second one 100% of the time.

For the first if:

  • You may want to use continue in the first if. It may make the code nicer to read.

  • Lets say I enter ''. It will fail silently.

  • Also let is a bad variable name, use char or something better.

And if you were to implement that:

if len(char) > 1:
    print "Only one!"
    continue
if not char:
    print "Enter a letter!"
    continue

In the second if:

  • It's normally not good to have brackets around if statements.
  • You should use operators such as -=, it will make code easier to read, write and change.
  • You can merge most of your prints into one. And,
  • Finally you should use str.format.

This would look like:

if char not in word:
    if char not in used_lets:
        lives -= 1
        used_letters.append(char)

        if lives == 0:
            print "Sorry you lost, the word was:", word
            break

        print(
            "Try again, you have only {} lives left!"
            "So far you have used: {}"
        ).format(lives, used_letters)

As you are using Python2 you should use xrange rather than range. One makes a list, the other makes a generator. In short, xrange is faster to loop through.
You could also try using enumerate.

for index, letter in enumerate(word):
    if char == letter:
        mock_word[index] = char

So overall there is nothing really that wrong with your code.

To further improve it I would put all the code, except import random, in a function. And then make a loop to ask the player if they want to play again. As I have never met a person who asks to play a game of hang man, and you only play it once.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$
  • Do not abbreviate. If you do, do it so that the reader understands abbreviation easily. let for letter is very wrong. First, let is a valid English word, with a clear meaning to programmers. Second, vowels convey practically no information. Even ltr would be better.

  • You are accepting non-letters as a valid input; you also made the game case-sensitive. I am not sure it is intentional. See below.

  • Input validation is logically coupled together with input reading. Make them into function:

    def get_letter():
        while True:
            letter = raw_input("Give a letter ")
            if len(letter) > 1:
                print "Only one!"
            else if letter not in string.ascii_letters:
                print "Letter I said!"
            else:
                return letter.tolower()
            # You can add as much validation here
            # without disturbing the main loop.
    
  • By testing letter not in word and then adding letter to mock_word, you are doing the essentially same work twice. Blindly try to put a letter in, and count how many times you have done it:

        insert_count = 0
        for i,l in enumerate(word):
            if letter == l:
                mock_word[i] = l
                insert_count += 1
    

    If insert_count remains 0, you know the guess was wrong. Of course, this should also be factored out into a function.

    Also notice that the for i in range loop is almost always wrong. enumerate is more Pythonic.

  • Instead of testing for "".join(mock_word) == word I recommend to keep track of remaining unknown letters:

            remaining -= insert_count
    
  • Having the remaining counter you could change the loop structure from while True to while lives > 0 and remaining > 0.

  • You should inform user if the used letter is repeated.

| improve this answer | |
\$\endgroup\$

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.