4
\$\begingroup\$

I've been trying to practice more Python recently so I decided to try and make different simple text-based game programs. I'd hope anybody could help me see how to make the code neater or more efficient in any aspect.

import random

words = []
guessed_letters = []
word_guess = []
joined_word = None


def fill_list():
    f = open('words.txt', 'r')
    for line in f:
        if len(line.strip()) > 3:
            words.append(line.strip())
    f.close()


def reset(secret):
    for letter in secret:
        word_guess.append('_')


def random_word(word_list):
    chosen = random.choice(word_list)
    return chosen


def guess_letter():
    while True:
        letter = input('Guess a letter a-z: ').lower().strip()
        if len(letter) != 1:
            print('Enter a single character only')
        elif not letter.isalpha():
            print('Enter only alphabetical characters')
        elif letter in guessed_letters:
            print('Already guessed {}'.format(letter))
        else:
            return letter


def print_opening():
    print('Welcome to Hangman!\n'
          'Try to guess a random word one letter at a time\n'
          'Good luck and have fun!\n')


def play_again():
    again = input().lower()
    if again.lower().startswith('y'):
        return True
    return False


def play():
    fill_list()
    tries = 5

    while True:
        print_opening()
        secret_word = random_word(words)
        reset(secret_word)

        while tries != 0 and '_' in word_guess:
            print('You have {} tries left'.format(tries))
            joined_word = ''.join(word_guess)
            print(joined_word)

            player_guess = guess_letter()
            for index, letter in enumerate(secret_word):
                if player_guess == letter:
                    word_guess[index] = player_guess

            guessed_letters.append(player_guess)

            if player_guess not in secret_word:
                print('Wrong!')
                tries -= 1

        if '_' not in word_guess:
            print('You win!\n{} was the word!\n'.format(secret_word))
        else:
            print('You lose!\nThe word was {}'.format(secret_word))

        print('Would you like to play another game? y/n')

        if play_again():
            word_guess.clear()
            tries = 5
        else:
            print('Thanks for playing!')
            break


if __name__ == '__main__':
    play()
\$\endgroup\$
2
\$\begingroup\$

Try to avoid global variables. For example words doesn't need to be a global. Instead, return a list of words from fill_list(), directly in the play() method:

def fill_list():
    words = []
    f = open('words.txt', 'r')
    for line in f:
        if len(line.strip()) > 3:
             words.append(line.strip())
    f.close()
    return words

...

def play():
    words = fill_list()
    ...

Use a resource manager (aka a with statement) for opening closable resources. That takes the burden of calling .close() off of you, and ensures the resource is properly closed even when exceptions occur:

def fill_list():
    words = []
    with open('words.txt', 'r') as f:
        for line in f:
            if len(line.strip()) > 3:
                words.append(line.strip())
    return words

Avoid repeating the same calculations. You strip the line to check its length, then you strip the line again to append it to words. Save the stripped line in a variable.

def fill_list():
    words = []
    with open('words.txt', 'r') as f:
        for line in f:
            stripped = line.strip()
            if len(stripped) > 3:
                words.append(stripped)
    return words

Use list comprehension.

def reset(secret):
    word_guess = ['_' for letter in secret]

Or without the global variable:

def reset(secret):
    return ['_' for letter in secret]

...

def play():
    ...
    word_guess = reset(secret_word)
    ...

f-strings (f'') are a new feature as of Python 3.6. You can use them to avoid .format() statements by embedding variables directly in the strings. Eg)

        print('Already guessed {}'.format(letter))

becomes

        print(f'Already guessed {letter}')

Don't repeat yourself. You have tries = 5 at the top of play() and the bottom if the user elects to play again. If you moved tries = 5 inside the while True: loop, at the top of the loop, you'd just need it once.

Similarly for word_guess.clear(), you should move it to the beginning of the loop ... except it can be removed entirely because of the word_guess = reset(secret_word) change above.


Bugs

If words.txt contains uppercase characters, hyphens, apostrophe's, etc., the user will not be able to guess the word. You should:

  • normalize the secret_word by calling .lower() on it.
  • replace only letters with the underscore, leave hyphens, apostrophes, spaces alone.

    def reset(secret):
        return ['_' if letter.isalpha() else letter for letter in secret]
    
\$\endgroup\$
2
\$\begingroup\$

One thing I would do is wrap this up in a hangman class. That way you could just toss away the hangman instance for a new one if the user ever decided to play again rather than resting all global variables. It would also decouple the handling of user input from the actual game. If the game size would grow this would make catching bugs a lot easier.

But lets focus on some smaller things that are easier to change

chosen = random.choice(word_list)
return chosen

Here your variable has no purpose, I would do

return random.choice(word_list)

Similar kind of story here

if again.lower().startswith('y'):
        return True
    return False

to

return again.lower().startswith('y')

Next is avoid magic constants at all costs, for example

if len(line.strip()) > 3:

What is 3? Why is 3 important here? You should replace this with a global ALL_CAPS variable to represent what 3 is.

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