1
\$\begingroup\$

I'm a noobie in programming so I've made a simple hangman game in python and I'd like someone to tell me how I could improve the code.

edit:How could I do it using classes?

from sys import exit

def makeGallow(gallow):
        for i in range(8):
            gallow.append([])
        gallow[0].append(6 * '_')
        gallow[1].append(' |   |')
        for i in range(2, 7):
            gallow[i].append('     |')
        gallow[7].append(11 * '-')


gallow = list()
secret_word = raw_input("Choose a word: ").lower()
secret_wordls = list()
letters = len(secret_word)
for c in secret_word:
    secret_wordls.append(c)
blanks = list()
for i in range(len(secret_word)):
    blanks.append('_')
lettersUsed = list()
tries = 0
makeGallow(gallow)

while True:

    for i in gallow:
        print "".join(i)
    print "".join(i for i in blanks)
    used = ", ".join(i for i in lettersUsed)
    print "Letters used: " + used
    letter = raw_input("Type a letter: ").lower()
    if letter == 'quit':
        exit(1)
    elif letter in secret_word:
        letters -= 1
        if letters == 0:
            print "You won!"
            exit("Bye!")
        else:
            switch = secret_wordls.index(letter)
            blanks[switch] = letter
    elif letter not in secret_word:
        print "Wrong!"
        tries += 1
        lettersUsed.append(letter)
        if tries == 1:
            gallow[2] = ' O   |'
        elif tries == 2:
            gallow[3] = ' |   |'
        elif tries == 3:
            gallow[3] = '/|   |'
        elif tries == 4:
            gallow[3] = '/|\  |'
        elif tries == 5:
            gallow[4] = ' |   |'
        elif tries == 6:
            gallow[5] = '/    |'
        elif tries == 7:
            gallow[5] = '/ \  |'
            print "Sorry you lost :(."
            print "The word was: " + secret_word
            exit("Bye!")
    elif letter in lettersUsed:
        print "You have already typed that letter!"
        print "Try another one."

edit2: I found a bug where if the work had 2, 3... times the same letter it would count as one and you couldn't win. I change the while loop:

while True:

    for i in gallow:
        print "".join(i)
    print "".join(i for i in blanks)
    used = ", ".join(i for i in lettersUsed)
    print "Letters used: " + used
    letter = raw_input("Type a letter: ").lower()
    if letter == 'quit':
        exit("Bye")
    elif letter in secret_word:
        for i in range(secret_wordls.count(letter)):
            switch = secret_wordls.index(letter)
            blanks[switch] = letter
            secret_wordls[switch] = '-'
            letters -= 1
    elif letter not in secret_word:
        print "Wrong!"
        tries += 1
        lettersUsed.append(letter)
        if tries == 1:
            gallow[2] = ' O   |'
        elif tries == 2:
            gallow[3] = ' |   |'
        elif tries == 3:
            gallow[3] = '/|   |'
        elif tries == 4:
            gallow[3] = '/|\  |'
        elif tries == 5:
            gallow[4] = ' |   |'
        elif tries == 6:
            gallow[5] = '/    |'
        elif tries == 7:
            gallow[5] = '/ \  |'
            print "Sorry you lost :(."
            print "The word was: " + secret_word
            exit("Bye!")
    elif letter in lettersUsed:
        print "You have already typed that letter!"
        print "Try another one."
    if letters == 0:
        print "You won!"
        exit("Bye!")
\$\endgroup\$
  • \$\begingroup\$ Could you please just write a single final block of code for clarity? \$\endgroup\$ – Caridorc May 24 '16 at 19:21
  • 2
    \$\begingroup\$ I have rolled back the last edit. Please see What to do when someone answers. \$\endgroup\$ – Mast May 27 '16 at 11:13
  • 2
    \$\begingroup\$ Rolled it back. Again. \$\endgroup\$ – Mast May 27 '16 at 15:44
  • \$\begingroup\$ I can't vote the answer because I don't have enough reputation... \$\endgroup\$ – UnwrinkledRadar May 27 '16 at 16:11
3
\$\begingroup\$

Whenever I start a review, I open up PEP 8, the Python style guide. That document is so rarely read that it is depressing. Your naming is not in accordance with it. From Method Names and Instance Variables:

Use the function naming rules: lowercase with words separated by underscores as necessary to improve readability.

(There's more, but it isn't pertinent here)


Your indentation looks good mostly, but for some reason you did eight-space indentation in makeGallow().


In makeGallow(), you do a bunch of .append() calls, but you don't guarantee that your list was empty in the first place. It is very easy to clear a list. Just do gallow[:] = []. As long as you are doing something like that any way, you might as well do gallow[:] = [[] for _ in range(8)] and take out your first for loop. I used _ as my variable name instead of i because it is the convention in Python to name a variable _ when you aren't using it.


Your code could benefit from an if __name__ == '__main__': block. That way, you might later create a very fancy hangman game that imports this one to use its makeGallow function. As it is, importing this would start the game. I would suggest that you define a main function and call it in your if __name__ == "__main__": block.


Strings are iterable. That can be useful when making a list of each character in it. For example, list("foo") is ["f", "o", "o"]. Therefore, you can change

secret_wordls = list()
for c in secret_word:
    secret_wordls.append(c)

to

secret_wordls = list(secret_word)

and change

blanks = list()
for i in range(len(secret_word)):
    blanks.append('_')

to

blanks = list("_" * letters)

Notice that I also changed len(secret_word) to letters. You defined the variable; why not use it? While I'm there, is letters really a good name? From the name, I would expect it to be a list of characters or a string, not a number. Maybe word_length would be better?


I see that you are using generator expressions. That's good, but actually a list comprehension is faster when using str.join(). Even better, don't bother with either.

"".join(i for i in blanks)

could be

"". join(blanks)

for example.


I suggest that you add an elif len(letter) != 1 between if letter == "quit": and the rest of the elifs. That way, you can inform the user that he should type just one letter instead of penalizing him because what he typed isn't in your list of letters.

Th-th-th-th-that's all, folks! (Don't worry; it's a quotation.)

\$\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.