7
\$\begingroup\$

I'm relatively new to Python, and I've been using the language for about a month. I set out to do the Hangman game. I would like to know if there are things to improve in the code of the program I made.

This is the code:

import random

HANGMAN_PICS = ('''
  +---+
  |   |
      |
      |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
      |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
  |   |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
 /|   |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
 /|\  |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
 /|\  |
 /    |
      |
=========''', '''
  +---+
  |   |
  O   |
 /|\  |
 / \  |
      |
=========''')
WORDS = ('Daniel', 'Cristian', 'Lucio', 'Alejandra', 'Delfina', 'Diana', 'Joshua', 'Pedro', 'Rick')
SECRET_WORD = random.choice(WORDS)
found_letters = []
blanks = len(SECRET_WORD) * '_'
print(f'\nSecret word: {blanks}')
print(f'{HANGMAN_PICS[0]}\n')
print('-' * 40)
blanks = list(blanks)
trials = 6

while trials > 0:
    letter = input('\nEnter a letter: ').lower()

    if len(letter) != 1 or not letter.isalpha():
        print('The value entered is invalid.\n')
        print('-' * 40)
    elif letter in found_letters:
        print('This letter has already been entered. Enter another letter.\n')
        print('-' * 40)
    elif letter in SECRET_WORD.lower():
        for i in range(len(SECRET_WORD)):
            if SECRET_WORD.lower()[i] == letter:
                blanks[i] = letter
        found_letters.append(letter)
        print(''.join(blanks))
        print('-' * 40)
    else:
        found_letters.append(letter)
        trials -= 1
        print(f'You missed and lost a life. You have {trials} trials left.')
        print(f'{HANGMAN_PICS[6-trials]}\n')
        print('-' * 40)

    if ''.join(blanks) == SECRET_WORD.lower():
        print(f'You won. The secret word is: {SECRET_WORD}.')
        break
else:
    print(f'You lost. The secret word is: {SECRET_WORD}.')
\$\endgroup\$

2 Answers 2

9
\$\begingroup\$

This looks pretty good for 1-month coder. Keep it up!

Use functions, classes and tests

If you have already learned them - use them. If you haven't - learn them. Start with functions. They are in the language to organize the code. When you have functions, you can test the code.

Avoid "magic numbers"

What's 40? What's 6? If you ever want to change horizontal line length to 60 - you should search all the code, and still miss some places where it's not 40 but 39 or something like that, say, if you use '+'+'-'*38+'+' somewhere as a spacer. Instead, introduce a new variable - it can be SPACER_SIZE = 40 or even SPACER = '-'*40 - and use it like print(SPACER). This gives you the possibility to change all instances at once and, at the same time, the name to what you're doing and save your time reading the code later.

Comparing strings and lists

You have two variables, SECRET_WORD and blanks, to save the secret word and its guessed part. And all over the code you're transforming them this or that way around. SECRET_WORD is a string with different cases; blanks changes from string to a list of lowercase letters and underscores. I think, the last data type would fit better for both variables; of course, there's a reason to preserve the original SECRET_WORD, so let's introduce a new variable:

SECRET_WORD = random.choice(WORDS))
secret_word = list(SECRET_WORD.lower())
blanks = ['-'] * len(secret_word) # this works for lists too
...
elif letter in secret_word: #no call to .lower()
...
    if secret_word[i] == letter:
            blanks[i] = letter
...
if secret_word == blanks: #no join, no lower

Move identical and related code together

In if-elif-elif-else the last line of all branches is print('-' * 40). You can simply move it out of there:

if ...: ...
elif ...: ...
elif ... : ...
else: ...
print('-' * 40)

The same way you can combine both found_letters.append(letter):

if ...: ...
elif ...: ...
else:
    found_letters.append(letter)
    if ...: ...
    else: ...

Btw, set is usually better than list to search for something, so it should be found_letters = set() and found_letters.add(letter); but in this case the difference would be so small you'll never notice the difference.

\$\endgroup\$
0
4
\$\begingroup\$

It looks pretty good! The coding style is consistent and it seems to follow the main PEP8 guidelines.

There are some things that I would change though.

Adding a main function

Adding the following pattern avoids polluting the global scope with local names:

def main():
    ...

if __name__ == '__main__':
    main()

It allows to define functions containing local variables without risking to overwrite an already existing global variable. Here is a StackOverflow question for more information.

Adding r before the HANGMAN_PICS strings

In those strings, the backslash \ must be displayed verbatim. To make it explicit, it could be wise to add the r prefix to each one of them

Magic numbers

Some constants could be named in the code, or even computed. For example, the number 40 appears multiple times in the print("-" * 40) statements. Why not create a function with a default parameter to centralize this value?

def print_sep(length=40):
    print("-" * length)

trials management

The trials variable is highly coupled with the HANGMAN_PICS. It's initial value could even be defined from HANGMAN_PICS. The total number of trials is also used later in the code, so it could be stored in its own variable:

max_trials = len(HANGMAN_PICS) - 1
trials = max_trials

and to avoid the magic number 6,

print(f'{HANGMAN_PICS[6-trials]}\n')

would become:

print(f'{HANGMAN_PICS[max_trials - trials]}\n')

Time complexity

This portion of code has a time complexity of O(len(secret_word)²) which does not really matter since secret_word is small:

for i in range(len(SECRET_WORD)):
    if SECRET_WORD.lower()[i] == letter:
         blanks[i] = letter

But it could be more pythonic and efficient to write:

for i, char in enumerate(SECRET_WORD.lower()):
    if char == letter:
        blanks[i] = letter

SECRET_WORD.lower() is called a lot of times in the code. It's a cheap method call, but I would probably store it in a variable.

Modified version

Here is the version of the code with the modifications I mentioned:

import random

HANGMAN_PICS = (r'''
  +---+
  |   |
      |
      |
      |
      |
=========''', r'''
  +---+
  |   |
  O   |
      |
      |
      |
=========''', r'''
  +---+
  |   |
  O   |
  |   |
      |
      |
=========''', r'''
  +---+
  |   |
  O   |
 /|   |
      |
      |
=========''', r'''
  +---+
  |   |
  O   |
 /|\  |
      |
      |
=========''', r'''
  +---+
  |   |
  O   |
 /|\  |
 /    |
      |
=========''', r'''
  +---+
  |   |
  O   |
 /|\  |
 / \  |
      |
=========''')
WORDS = ('Daniel', 'Cristian', 'Lucio', 'Alejandra', 'Delfina', 'Diana', 'Joshua', 'Pedro', 'Rick')


def print_sep(length=40):
    print('-' * length)


def main():
    secret_word = random.choice(WORDS)
    secret_word_low = secret_word.lower()
    found_letters = []
    blanks = len(secret_word) * '_'
    print(f'\nSecret word: {blanks}')
    print(f'{HANGMAN_PICS[0]}\n')
    print_sep()
    blanks = list(blanks)
    max_trials = len(HANGMAN_PICS) - 1
    trials = max_trials

    while trials > 0:
        letter = input('\nEnter a letter: ').lower()

        if len(letter) != 1 or not letter.isalpha():
            print('The value entered is invalid.\n')
            print_sep()
        elif letter in found_letters:
            print('This letter has already been entered. Enter another letter.\n')
            print_sep()
        elif letter in secret_word_low:
            for i, char in enumerate(secret_word_low):
                if char == letter:
                    blanks[i] = letter
            found_letters.append(letter)
            print(''.join(blanks))
            print_sep()
        else:
            found_letters.append(letter)
            trials -= 1
            print(f'You missed and lost a life. You have {trials} trials left.')
            print(f'{HANGMAN_PICS[max_trials - trials]}\n')
            print_sep()

        if ''.join(blanks) == secret_word_low:
            print(f'You won. The secret word is: {secret_word}.')
            break
    else:
        print(f'You lost. The secret word is: {secret_word}.')


if __name__ == '__main__':
    main()
\$\endgroup\$
0

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.