5
\$\begingroup\$

The problem:

Making a simple hangman game. This is a continuation of the following question: The right way to hang a man. There I just tried to make the ascii-gallow. Now I have tried to write the rest of the code for the hangman game, using the ideas from the answer.

I have tried very hard to produce the following output

   _______
   |     |
         |
         |
         |
         |
   -------------

 Solution: _ _ _ _ _ _ _

 Letters used: 

 Guess a letter cowboy! :

every time the user guesses a letter. This was hard as it varies when to update the gallow. Converting the generator to a list solved this problem, but it made the code less readable. I was wondering if it was possible to achieve the ascii above, without such a messy code.

After some suggestions in chat I tried to improve the printing in my code. Particularly I tried

ascii = '''\
{gown}

Solution: {secret}

Letters used: {used}
'''

print ascii.format(gown=gown, secret = ' '.join(temp_secret_word), used = ' '.join(guessed_letters))

Firstly this did produce an output, but I had two problems with it. Firstly it messed up the start of the top of the gallow.

enter image description here

secondly I am really unsure if it is clearer what the code does. I thought having lines longer than 80 lines were considered bad practice.

Questions:

The above discussion boils down into two questions. Recently I have been trying writing more readable code, and minimizing the unnecessary parts.

  • How readable is the code? (Particularly logic, unnecessary variable names, reasonable names?, too few/too many functions?)
  • hangman_game() uses many prints and wierd logic can I achieve the same output with less clutter?

The code:

import string

SECRET_WORD = 'Hangman'.upper()
ALPHABET = list(string.ascii_uppercase)
HEIGHT = 7
WIDTH = 13
INDENT = 3

_GALLOW_MODIFIER = {
    1: (' O ', 2),
    2: (' | ', 3),
    3: ('/| ', 3),
    4: ('/|\\', 3),
    5: ('/  ', 4),
    6: ('/ \\', 4),
}


def create_gallow(width, height):
    half_width = width // 2
    gallow = ['{}{}{}'.format(' '*INDENT, ' '*half_width, '|')]*height
    gallow[0] = '{}{}'.format(' '*INDENT, '_' * (half_width+1))
    gallow[1] = '{}{}{}{}'.format(' '*INDENT, '|', ' '*(half_width-1), '|')
    gallow[-1] = '{}{}'.format(' '*INDENT, '-' * width)
    return gallow


def next_gallow(width, height):
    gallow = create_gallow(width, height)
    half_width = width // 2

    yield '\n'.join(gallow)
    for attempt in range(1, 7):
        pattern, row = _GALLOW_MODIFIER[attempt]
        gallow[row] = '{}{}{}{}'.format(
            ' '*(INDENT-1), pattern, ' '*(half_width-len(pattern)+1), '|')
        yield '\n'.join(gallow)


def temp_secret(guessed_letters):
    temp_secret_word = ['_']*len(SECRET_WORD)
    for index, letter in enumerate(SECRET_WORD):
        if letter in guessed_letters:
            temp_secret_word[index] = letter
    return temp_secret_word


def manage_user_input(guessed_letters):
    valid_guess = False
    while not valid_guess:
        guess = raw_input(' Guess a letter cowboy! : ').upper()
        if guess not in ALPHABET:
            print 'That is not in the alphabet cowboy, please try again!'
        elif guess in guessed_letters:
            print 'You have already tried to guess', guess, 'cowboy, please try again!'
        else:
            valid_guess = True
    return guess


def hangman_game():
    temp_secret_word = ['_']*len(SECRET_WORD)
    guessed_letters = []

    gallows = list(next_gallow(WIDTH, HEIGHT))
    for gallow in gallows[0:-1]:

        print gallow
        print
        print ' Solution: ' + ' '.join(temp_secret_word)
        print
        print ' Letters used: ' + ' '.join(guessed_letters)

        guess = manage_user_input(guessed_letters)
        guessed_letters.append(guess)

        while guess in SECRET_WORD:
            temp_secret_word = temp_secret(guessed_letters)

            print '\n You guessed correctly cowboy!'
            print
            print gallow
            print
            print ' Solution: ' + ' '.join(temp_secret_word)
            print
            print ' Letters used: ' + ' '.join(guessed_letters)

            if ''.join(temp_secret_word) == SECRET_WORD:
                print '\n You won cowboy! '
                return
            guess = manage_user_input(guessed_letters)
            guessed_letters.append(guess) 

        print '\n You guessed incorrectly cowboy!'
    print gallows[-1]
    print
    print ' Solution: ' + ' '.join(temp_secret_word)
    print
    print ' Letters used: ' + ' '.join(guessed_letters)
    print
    print ' You died cowboy! '
    return


if __name__ == '__main__':
    hangman_game()
\$\endgroup\$
  • \$\begingroup\$ Haha ^^ Did not know what was a bad thing until I did some light reading on it. I was able to fix it though. However I know updating the code in question is bad practice. \$\endgroup\$ – N3buchadnezzar Jun 7 '16 at 19:18
  • 1
    \$\begingroup\$ Related discussion. It may be ok to edit as long as you don't have any answers and the changes are small. Especially since I already mentionned this point in your previous question. \$\endgroup\$ – 409_Conflict Jun 7 '16 at 19:55
  • \$\begingroup\$ I guess I will let you have a say wheter to update the code or not. Here is my new proposed code pastebin.com/q5dhr3wN. I changed the insert_correct_guess to not change in place. Other than that the only changes I made was changing the name of one function, and swap places of a few functions. Always hard to know wheter someone has started writing answers or not. \$\endgroup\$ – N3buchadnezzar Jun 7 '16 at 19:59
  • 1
    \$\begingroup\$ Changes are small enough that I would find it OK to modify the question. Be ready to rollback if someone is typing an answer and makes comments about parts you changed. \$\endgroup\$ – 409_Conflict Jun 7 '16 at 20:06
3
\$\begingroup\$

There are a few changes that you should probably make.

  • Change ALPHABET to a set not a list.
  • Change INDENT to the string ' ' * INDENT, and add INDENT_SIZE as the old indent. This is as you use ' ' * INDENT way more than you do INDENT as a number.
  • Change _GALLOW_MODIFIER to a list of tuples, rather than a dictionary. This is as the only time that you use it you use something equivalent to [_GALLOW_MODIFIER[i] for i in range(1, 7)]. Which could simply be [i for i in _GALLOW_MODIFIER].
  • Change temp_secret to use a list comprehension. You currently create a list, and then modify it, rather than create it as the modified data. By using a list comprehension and a single turnery we can change this function to:

    def temp_secret(guessed_letters):
       return [l if l in guessed_letters else '_' for l in SECRET_WORD]
    
  • Change manage_user_input to use break rather than a flag. Reading an infinite while loop with a break is somewhat easier to read than a flag to break out the loop.

  • Add a closure to hangman_game that prints the gallow, temp_secret_word and guessed_letters. This allows you to call the function the three times you do this. And reduces print statements.

    I'd go on to change this to a single print, using str.format. And you will need to add gallow out of the fors scope. (I don't think for statements have their own scope in Python2, but they do in Python3)

These are things that you should probably do. There is also something that I'd do, that some may disagree with. I'd change create_gallow to return a list of strings that are to be formatted to simplify next_gallow. This actually is quite simple to add to create_gallow. Just take the format that you are using in next_gallow and put it in create_gallow, the only tricky part is making the string work with two formats. This can then be:

'{}{{:<{}}}{}'.format(' '*(INDENT_SIZE - 1), half_width + 1, '|')

And since there's no point in passing '|' as an argument, you can use:

'{}{{:<{}}}|'.format(' '*(INDENT_SIZE - 1), half_width + 1)

Which you will later use to format in pattern. I'd then add a tuple to _GALLOW_MODIFIER to keep the gallow the same, and reduce the amount of yields to one.

If you wanted to improve this, making the entire thing a class could be a great benefit.

And could result in:

import string

SECRET_WORD = 'Hangman'.upper()
ALPHABET = set(string.ascii_uppercase)
HEIGHT = 7
WIDTH = 13
INDENT_SIZE = 3
INDENT = ' ' * INDENT_SIZE

_GALLOW_MODIFIER = [
    (0, ''),
    (2, ' O '),
    (3, ' | '),
    (3, '/| '),
    (3, '/|\\'),
    (4, '/  '),
    (4, '/ \\'),
]


def create_gallow(width, height):
    half_width = width // 2
    gallow = ['{}{{:<{}}}|'.format(' '*(INDENT_SIZE - 1), half_width + 1)] * height
    gallow[0] = '{}{}'.format(INDENT, '_' * (half_width + 1))
    gallow[1] = '{}{}{}{}'.format(INDENT, '|', ' ' * (half_width - 1), '|')
    gallow[-1] = '{}{}'.format(INDENT, '-' * width)
    return gallow


def next_gallow(width, height):
    gallow_format = create_gallow(width, height
    gallow = [g.format('') for g in gallow_format]
    for row, pattern in _GALLOW_MODIFIER:
        gallow[row] = gallow_format[row].format(pattern)
        yield '\n'.join(gallow)


def temp_secret(guessed_letters):
    return [l if l in guessed_letters else '_' for l in SECRET_WORD]


def manage_user_input(guessed_letters):
    while True:
        guess = raw_input(' Guess a letter cowboy! : ').upper()
        if guess not in ALPHABET:
            print 'That is not in the alphabet cowboy, please try again!'
        elif guess in guessed_letters:
            print('You have already tried to guess {} cowboy, please try again!'
                .format(guess))
        else:
            break
    return guess


def hangman_game():
    temp_secret_word = ['_'] * len(SECRET_WORD)
    guessed_letters = []
    gallow = None

    def display_gallow():
        print '{}\n\n Solution: {}\n\n Letters used: {}'.format(
            gallow,
            ' '.join(temp_secret_word),
            ' '.join(guessed_letters))

    gallows = list(next_gallow(WIDTH, HEIGHT))
    for gallow in gallows[0:-1]:
        display_gallow()
        guess = manage_user_input(guessed_letters)
        guessed_letters.append(guess)
        while guess in SECRET_WORD:
            temp_secret_word = temp_secret(guessed_letters)
            display_gallow()
            print '\n You guessed correctly cowboy!\n'
            if ''.join(temp_secret_word) == SECRET_WORD:
                print '\n You won cowboy! '
                return
            guess = manage_user_input(guessed_letters)
            guessed_letters.append(guess)
        print '\n You guessed incorrectly cowboy!'
    gallow = gallows[-1]
    display_gallow()
    print '\n You died cowboy! '
    return


if __name__ == '__main__':
    hangman_game()
\$\endgroup\$
  • \$\begingroup\$ Great suggestions =) I changed _GaLLOW_MODIFIER into a generator instead of a list. Which is prefered? Also in the manage_user_input instead of breaking could one return directly? Or is this somewhat less readable? (there still is only one return though). \$\endgroup\$ – N3buchadnezzar Jun 8 '16 at 9:27
  • \$\begingroup\$ @N3buchadnezzar " I changed _GALLOW_MODIFIER into a generator instead of a list." Now that I'd like to see, ;P Yes you could return from within the while I should have noticed that. And is probably preferable. ): \$\endgroup\$ – Peilonrayz Jun 8 '16 at 9:31
  • \$\begingroup\$ i.imgur.com/TOBXt7x.jpg \$\endgroup\$ – N3buchadnezzar Jun 8 '16 at 9:49
  • 2
    \$\begingroup\$ @N3buchadnezzar Keep it as a list. It's bad to use a generator there, it gives worse performance, and means you can only use it once. \$\endgroup\$ – Peilonrayz Jun 8 '16 at 9:52
  • \$\begingroup\$ Awesome suggestions, feels like I am learning so much =) Last question. Any suggestions for writing the main file hangman_main in a clearer way? Compacting the print sure helps on the readability, but it only "hides" the ugly logic behind. Turning the generator into a list seems bad + splitting up the generator. \$\endgroup\$ – N3buchadnezzar Jun 8 '16 at 10:06
2
\$\begingroup\$

You are right, your gallows list and its management is ugly.

What I had in mind when introducing the generator in my previous answer was that it would yield gallows to print when the next mistake occurs. The idea being that when the last mistake will be made, the last gallow will print and the for loop will terminate. So any code after the loop can deal with the fact that the player lost. And a win can be managed with a return within the loop.

Now you seems to struggle because you want to print an initial gallow before asking for a first letter. There are several ways to manage that:

  • Do not print anything before the first input. I feel like it is a viable solution as there is no need to show the player some kind of "death countdown" before their first fail;
  • Use next on the generator once before the for loop to extract the first state out and print it;
  • Modify _GALLOW_MODIFIER, next_gallow and possibly create_gallow to achieve the desired effect. Mainly, create_gallow will return the initial state when no error occured (as it is currently?) but next_gallow won't yield it and only yield modified versions. This way, you can call create_gallow before the for loop to print it and iterate with for gallow in next_gallow(…).

I’d go with the second one as it involves less modifications:

def print_summary(gallow, secret, guesses):
    print gallow
    print
    print ' Solution: ' + ' '.join(secret)
    print
    print ' Letters used: ' + ' '.join(guesses)


def hangman_game():
    temp_secret_word = ['_']*len(SECRET_WORD)
    guessed_letters = []

    gallows = next_gallow(WIDTH, HEIGHT)
    print_summary(next(gallows), temp_secret_word, guessed_letters)
    for gallow in gallows:
        while True:
            guess = manage_user_input(guessed_letters)
            guessed_letters.append(guess)
            if guess not in SECRET_WORD:
                print '\n You guessed incorrectly cowboy!'
                break

            temp_secret_word = temp_secret(guessed_letters)
            print_summary('', temp_secret_word, guessed_letters)

            if ''.join(temp_secret_word) == SECRET_WORD:
                print '\n You won cowboy! '
                return
        print_summary(gallow, temp_secret_word, guessed_letters)
    print ' You died cowboy! '

As you can see, I removed printing the gallow when the user guessed a letter right because I feel that it improves the flow of things. If you want the old behaviour back, you can use a current_state variable that you initialize with next(gallows) and you update with current_state = gallow at the end of the for loop.

Or you can use the pairwise recipe to make everything more smooth:

from itertools import tee, izip


def pairwise(iterable):
    "s -> (s0,s1), (s1,s2), (s2, s3), ..."
    a, b = tee(iterable)
    next(b, None)
    return izip(a, b)

def hangman_game():
    temp_secret_word = ['_']*len(SECRET_WORD)
    guessed_letters = []

    for current_state, gallow in pairwise(next_gallow(WIDTH, HEIGHT)):
        while True:
            print_summary(current_state, temp_secret_word, guessed_letters)
            guess = manage_user_input(guessed_letters)
            guessed_letters.append(guess)
            if guess not in SECRET_WORD:
                print '\n You guessed incorrectly cowboy!'
                break

            temp_secret_word = temp_secret(guessed_letters)

            if ''.join(temp_secret_word) == SECRET_WORD:
                print '\n You won cowboy! '
                return
    print_summary(gallow, temp_secret_word, guessed_letters)
    print ' You died cowboy! '
\$\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.