28
\$\begingroup\$

I am new to Python (it's my first language), been coding for a couple of weeks now.

I have already made a couple of simple scripts to download and manipulate some financial data, but lately I thought about making a simple hangman game. I tested it thoroughly and it seems to work just fine.

However, as I do not yet know a lot about the best practices of writing an optimal code, could somebody please review the attached code and give me some feedback about how could I improve it?

#import packages
from random_words import RandomWords
from colorama import Fore, Back, Style
rw = RandomWords()
import os
import time

#list of 7 possible states of the hangman
hangman_pics = ['''
  +---+
  |   |
      |
      |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
      |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
  |   |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
 /|   |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
 /|\  |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
 /|\  |
 /    |
      |
=========''', '''
  +---+
  |   |
  O   |
 /|\  |
 / \  |
      |
=========''']

#define function to clear the screen
def clear_screen():
    return os.system('cls')

#game on/off switch
game_on = False

#opening statement
os.system('cls')
print ('Welcome to the Hangman!\n')

#ask player to start the game
while game_on is False: 
    game_start = input ('Would you like to start a new game? [y/n]... ').upper()
    if game_start == 'Y':
        game_on = True
    elif game_start == 'N':
        game_on = False
        clear_screen()
    else:
        clear_screen()
        print ("Please input [y] or [n]")
clear_screen()

while game_on is True:
    #generate a random word to guess and transform to a list
    word_to_guess = rw.random_word()
    word_to_guess_list = list(word_to_guess)
    length_of_word_to_guess = len((word_to_guess_list))
    #generate a placeholder list for tried but wrong guesses
    tried_but_wrong = []
    #create a representation of word_to_guess_list with hidden spaces
    hidden_word = ('_')*length_of_word_to_guess
    hidden_word_list = list(hidden_word)

    #info about generated word to guess
    print ('I have just generated a random word for you to guess!')
    print (f'\nThe word has {length_of_word_to_guess} letters.')

    #initialize the number of attempts left
    attempts_left = 6
    hangman_state = 0
    print (f'\nYou have {attempts_left} attempts left.')

    #start the while loop for the game's logic
    while attempts_left>0 or hidden_word_list != word_to_guess_list:

        #initialize the guessed letter
        guess = ('')

        #print the current state of the hangman
        print (hangman_pics[hangman_state])

        #print the letter already used an not in the word to guess
        if tried_but_wrong == []:
            pass
        else:
            print(f'\nTip: you have alread tried these letters: {tried_but_wrong}\n')

        #while loop for guessing a letter 
        while len(guess) != 1 or type (guess) != str:
            print (f'This is the word you are trying to guess:'+'\n'*2+f'{hidden_word_list}')
            guess = input ('\nPlease select a letter you think is in the hidden word... ')
        clear_screen()

        #check if guessed letter is in the word to guess and not already guessed
        if guess in word_to_guess_list and guess not in hidden_word_list:
            print (f'''Great! You guessed correctly, "{guess}" is in the word you are trying to guess.''')

            #check the indices of guessed letter(s)
            print (f'You have {attempts_left} attempts left.')
            indices_of_guessed_letter = [i for i, x in enumerate(word_to_guess_list) if x == guess]

            #replace blank spots in hidden_word_list with guessed letter(s)
            for indices in indices_of_guessed_letter:
                hidden_word_list [indices] = guess

        #inform the player that they already guessed the selected letter
        elif guess in hidden_word_list:
            print ('Woops! Looks like you have already guessed this one! Please try again!')

        #inform the player that they already tried that letter and it's not in the word to guess
        elif guess in tried_but_wrong:
            print (f'There is no "{guess}" in the word you are trying to guess, but you have already tried that one.')

        #else: inform the player that they guessed wrong
        else:
            print (f'There is no "{guess}" in the word you are trying to guess.')

            #add the guessed and wrong letter to a list of already tried guesses
            tried_but_wrong.append(guess)

            #reduce the number of attempts left
            attempts_left -= 1

            #progress the hangman state
            hangman_state += 1

            #print the info about the number of attempts left
            print (f'You have {attempts_left} attempts left.')

        #check for win or loss

        #check for win
        if word_to_guess_list == hidden_word_list:
            time.sleep(2)
            clear_screen()
            print (f'You correctly guessed the word, which is ' + Fore.GREEN + f'"{word_to_guess}"'+ Style.RESET_ALL+'.')
            print (f'You had {attempts_left} attempts left.')
            break

        #check for loss
        if attempts_left == 0:
            time.sleep(2)
            clear_screen()
            print
            print (f"You lost. The word you were trying to guess was " + Fore.RED+ f'"{word_to_guess}"' + Style.RESET_ALL + '.')
            print ('\nUnfortunately, you are dead.')
            print (hangman_pics[hangman_state])
            break

    #ask if player wants to replay
    restart = False
    while restart is False: 
        game_restart = input ('\nWould you like to start a new game? [y/n]... ').upper()
        if game_restart == 'Y':
            restart = True
            clear_screen()
            game_on = True
        elif game_restart == 'N':
            restart = True
            clear_screen()
            print ("Thank you for playing!")
            time.sleep(3)
            game_on = False
            clear_screen()
        else:
            clear_screen()
            print ("Please input [y] or [n]")
\$\endgroup\$
1

5 Answers 5

30
\$\begingroup\$

You define a clear_screen function, but then at the top you have

#opening statement
os.system('cls')  # Here
print ('Welcome to the Hangman!\n')

You might as well use the function there.


A little further down you have:

while game_on is True:

The is check is only necessary if game_on could be some truthy value other than True, and you wanted to check if it was literally only equal to True. game_on will only every have the values True or False though, so you can just write:

while game_on:

Which reads nicer anyways.


tried_but_wrong is a list, but you're using it to do membership tests when you write

guess in tried_but_wrong

If you're using in to test for membership like you are here, ideally, the collection shouldn't be a list. x in some_list requires that the entire list is potentially checked, which can be an expensive operation. It would be better if tried_but_wrong was a set, since you don't seem to need the insertion order maintained anyway.

tried_but_wrong = set()  # An empty set. Python doesn't have a literal for an empty set
. . .
if not tried_but_wrong:  # Empty sets and lists are falsey
. . .
tried_but_wrong.add(guess)

Membership lookups on sets are very fast due to how they're implemented. If the purpose of a collection is just to track what you've "seen" already, and you don't care about order, use a set.


In Python 3, print is a function call, yet you're using "detached braces":

print ("Please input [y] or [n]")

All this does is momentarily make your code look like Python 2. Since it's an ordinary function call, format it as such and have the braces "attached" to the call:

print("Please input [y] or [n]")

And the same goes for code like:

hidden_word_list [indices] = guess

[indices] is a part of hidden_word_list. Having the indexing floating there makes it slightly less obvious what's going on. Keep them attached.

And this isn't just my word. PEP 8, Python's style guide, explicitly recommends this.

And on the subject of white-space styling, make sure you have white-space around binary operators. Lines like

while attempts_left>0 or hidden_word_list != word_to_guess_list:

Are inconsistent and violate the guide. Have space around >:

while attempts_left > 0 or hidden_word_list != word_to_guess_list:

Even if you were fine violating PEP 8, you're inconsistent with how you style things. You space some things out in some places but not others. Be consistent. Consistency and proper naming are two very valuable tools that ensure your code is readable.


A few places, you're putting parenthesis around string literals for some reason:

hidden_word = ('_')*length_of_word_to_guess
. . . 
guess = ('')

I'm not sure why though. This momentarily makes it seem like they're tuples. Just use bare strings, and for the first line there, again, put space around *.


if tried_but_wrong == []:
    pass
else:
    print(f'\nTip: you have alread tried these letters: {tried_but_wrong}\n')

This has a couple things off; one of which I mentioned earlier:

  • Empty collections are falsey. It's generally regarded as idiomatic to use if some_coll to test if a collection has elements (or if not some_coll to test if it's empty).

  • You're testing for a condition, then only using the else. Just negate the condition if necessary. Here though, negation isn't even needed:

     if tried_but_wrong:
        print(f'\nTip: you have already tried these letters: {tried_but_wrong}\n')
    


There are some things I like though:

  • You're making good use of f-strings. That certainly makes the string construction neater.

  • You use snake_case and use descriptive names. Both are good practices.

\$\endgroup\$
14
\$\begingroup\$

It's not that great of an optimization but if you're new to programming it can be interesting to consider something along the line of :

hangman_part = ['O','|','/','\\','/','\\']
hangman_base ='''
  +---+
  |   |
  0   |
 213  |
 4 5  |
      |
========='''

and

def get_current_figure():
    """Print current state of Hangman."""
    hangman_pic = hangman_base
    for i in range(6) :
        if 5-attempts >= i:
            hangman_pic = hangman_pic.replace(i, hangman_part[i])
        else:
            hangman_pic = hangman_pic.replace(i, ' ')
    print(hangman_pic)

(I don't use Python so I may have missed something, it's more for the idea anyway)

This code sacrifice some perfs (usually negligible) for an improved maintainability. Let's say you want to change the design of your gallows (or your man), now you only need to do it once !

Sure, in your specific case, there's not that much possible changes, nor that many gallows to change. But you're doing it to practice and in practice, it's nearly always a good idea to go for maintainability.

I'd like to argue that it also improved readability, since you now have way less lines, but it's not granted since we added a bit of complexity. I guess it may vary depending on the reader.

\$\endgroup\$
4
  • 2
    \$\begingroup\$ hangman_part = ['O','|','/','\','/','\'] this line contains a syntax error due to the so-called backslash you added and since it looks like you're new to Python and maybe programming in general, I suggest you check PEP8 python.org/dev/peps/pep-0008 the official Python style guide before writing some code in Python to avoid making mistakes like this for i in range(0, 5) (and it is 6 btw not 5 attempts) and is expressed : for i in range(5) (no need to add 0) Please revise your code and run it instead of this leap of faith. \$\endgroup\$
    – user203258
    Aug 26, 2019 at 19:19
  • \$\begingroup\$ @emadboctor : (I don't use Python so I may have missed something, it's more for the idea anyway) ^^... As you can see in the hangman_base, there is only [0-5] numbers (and parts) to replace, going up to 6 should issue some error no ? Thanks for correcting my `\` \$\endgroup\$
    – Nomis
    Aug 27, 2019 at 8:42
  • \$\begingroup\$ @Nomis The Python range(start, stop) object is “half-open”; it includes the start value but excludes the stop value. So range(0, 5) only includes the 5 values 0, 1, 2, 3, 4. It is similar to the C/C++ loop: for(int i=0; i<5; i++) {...} in this respect; the loop ends when the endpoint is reached instead of after the endpoint is reached. \$\endgroup\$
    – AJNeufeld
    Aug 27, 2019 at 14:00
  • \$\begingroup\$ @AJNeufeld : wow, it was so intuitive that I never got up to this part in the doc ><. Thanks to you two for pointing it out and taking the time to explain it ! \$\endgroup\$
    – Nomis
    Aug 27, 2019 at 14:16
11
\$\begingroup\$

Welcome to code review...

Import statements

from random_words import RandomWords
from colorama import Fore, Back, Style

Assuming that someone wants to run your program, how would he run it without having this random_words thing? You should include it with the rest of your code unless it's an official Python module.

Back in the second import statement is not used, should be ommitted/cleaned up.

Style

I suggest you check PEP0008, the official Python style guide.

Here are a few comments:

#define function to clear the screen
def clear_screen():
    return os.system('cls')
  • Docstrings: Python documentation strings (or docstrings) provide a convenient way of associating documentation with Python modules, functions, classes, and methods. An object's docstring is defined by including a string constant as the first statement in the object's definition. You might write a docstring instead of the comment above the function.

    def clear_screen():
        """Clear the screen."""
        return os.system('cls')
    
  • Comments: You might want to include comments on a need basis and omit the unnecessary explanations; according to PEP0008 you should use comments sparingly. lots of things are self-explanatory. And a comment starts with # comment not #comment.

    # import packages
    # define function to clear the screen
    # opening statement
    
  • Blank lines: (pep008) Extra blank lines may be used #(sparingly) to separate groups of related functions. Blank lines may be omitted between a bunch of related one-liners (e.g. a set of dummy implementations).

  • Too long lines: PEP 8 suggests lines should be limited to 79 characters.

  • while game_on is True:
    

    can be expressed:

    while game_on:
    
  • Missing white space around operators: 1 space on both sides of binary operators(+ - / * // & | ^ % = > < == !=) except for function default values.

    hidden_word = ('_')*length_of_word_to_guess
    print (f'You correctly guessed the word, which is ' + Fore.GREEN + 
    f'"{word_to_guess}"'+ Style.RESET_ALL+'.')
    

Bugs

Tip: you have alread tried these letters: ['7']

Invalid input: no catching of problematic inputs such as numbers (7 in the case above, the program indicates that I already tried 7 before). The same goes for invalid inputs in the first question (do you want to start the game? y/n) - suppose a user entered yes or no instead of n or y or N or Y, then the program should indicate the invalid input or have cases covering such possible occurrences (it's not very unlikely that someone enters yes or no).

Clear screen function os.system('cls') clears only on Windows systems, os.system('clear') for Unix (including Mac and Linux) systems otherwise are going to throw some error; you should indicate that in the docstring or implement another function to support Unix systems (I have a macbook, so I had to change it to run properly).

Would you like to start a new game? infinite loop if the answer is no (n) at the very start of the game, so the user is compelled to play; he has no actual choice!

Upper case letters the program assumes the user will enter only lower case letters.

os.system('cls')
print ('Welcome to the Hangman!\n')

Why use os.system('cls') and you already defined a function to do so? why not ...

clear_screen()
print ('Welcome to the Hangman!\n')

Functions

A function is a block of code which only runs when it is called. You can pass data, known as parameters, into a function. A function can return data as a result. You can enclose your code into separate functions that perform different tasks; it's better for readability/modularity and easier to debug/ change (what if you want to change some value that keeps repeating through the code?).

Here's a refactored version of the code:

You will need to download this word list for running the code (put the file in the same folder with the script).

import random
import string

words = [word for word in open('random_words.txt').read().split()]
attempts = 6
hangman_pics = ['''
  +---+
  |   |
      |
      |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
      |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
  |   |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
 /|   |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
 /|\  |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
 /|\  |
 /    |
      |
=========''', '''
  +---+
  |   |
  O   |
 /|\  |
 / \  |
      |
========='''][::-1]


def get_current_figure():
    """Print current state of Hangman."""
    print(hangman_pics[attempts])


def play_game():
    """Play game."""
    global attempts
    available_letters = list(string.ascii_lowercase)
    letters_guessed = []
    secret_word = random.choice(words)
    slots = ['_' for _ in range(len(secret_word))]
    valid_responses = ['y', 'yes', 'n', 'no']
    print('Welcome to the hangman!')
    confirm_start = input('Would you like to start a new game?' 'y/n: ').lower()
    while confirm_start not in valid_responses:
        print(f'Invalid response! {confirm_start}  Enter y/n')
        confirm_start = input('Would you like to start a new game?' 'y/n: ').lower()
    if confirm_start == 'n' or confirm_start == 'no':
        print('Thank you for playing Hangman.')
        print(29 * '=')
        exit(0)
    while confirm_start == 'y' or confirm_start == 'yes':
        if not attempts:
            get_current_figure()
            print(f"You're dead, the word was {secret_word}")
            break
        check_win = 0
        for letter in secret_word:
            if letter in letters_guessed:
                check_win += 1
        if check_win == len(secret_word):
            print(f'Well done! you win.\nThe correct word is {secret_word}')
        print(45 * '=')
        get_current_figure()
        print(f"Available letters: {''.join(available_letters)}")
        print(f"Letters used: {''.join(sorted(letters_guessed))}")
        print(f'You have {attempts} attempts left.')
        print(slots, '\n')
        letter_guessed = input('Guess a letter:  ').lower()
        if letter_guessed in letters_guessed:
            print(f'You already tried that letter {letter_guessed}')
            attempts -= 1
        if letter_guessed.isalpha() and len(letter_guessed) == 1 and letter_guessed not in letters_guessed:
            available_letters.remove(letter_guessed)
            letters_guessed.append(letter_guessed)
            if letter_guessed in secret_word and len(letter_guessed) == 1:
                for index, letter in enumerate(secret_word):
                    if letter_guessed == letter:
                        slots[index] = letter
                print(f'Correct guess! {letter_guessed} is in the word.')
            if letter_guessed not in secret_word:
                attempts -= 1
                print(f'Wrong guess! {letter_guessed} not in the word.')
        if not letter_guessed.isalpha() or len(letter_guessed) > 1:
            print(f'Invalid entry {letter_guessed}')
            print('You have been penalized and lost 2 attempts!')
            attempts -= 2


def replay_game():
    """Return True for a game replay, False otherwise."""
    valid_responses = ['y', 'yes', 'n', 'no']
    replay = input('Would you like to play another game? y/n ').lower()
    while replay not in valid_responses:
        print(f'Invalid response {replay}')
        replay = input('Would you like to play another game? y/n ').lower()
    if replay == 'y' or replay == 'yes':
        return True
    if replay == 'n' or replay == 'no':
        print('Thank you for playing Hangman.')
        print(29 * '=')
        exit(0)
        return False


if __name__ == '__main__':
    while True:
        play_game()
        replay = replay_game()
        if replay:
            attempts = 6
            play_game()
        else:
            exit(0)
\$\endgroup\$
7
  • 2
    \$\begingroup\$ Even just looking at your if __name__ == "__main__" block, there are problems - making attempts a global variable here is very bad form, just pass it as an argument to play_game() (which then passes it as an argument to get_current_figure()) or alternatively just make it a local variable in play_game(). Also, get_current_figure() is a bad name for something that prints something - it should either return the string (not print it) or be called draw_current_figure(). \$\endgroup\$
    – Jack M
    Aug 26, 2019 at 12:33
  • \$\begingroup\$ I didn't like the global variable thing but since the get_current_figure uses the variable attempts to print state. regarding the naming of get_current_figure, at first, I made it print_current_figure and for some reason the editor highlighted the name, so I changed it to get_current_figure ... print_current_figure is better anyway \$\endgroup\$
    – user203258
    Aug 26, 2019 at 16:11
  • \$\begingroup\$ And I suggest you focus more on the actual code being reviewed here, not on comments \$\endgroup\$
    – user203258
    Aug 26, 2019 at 16:30
  • 7
    \$\begingroup\$ Critiquing code in answers is arguably more important than critiquing the original code, since by answering this question you're automatically assuming sort of a position of authority - if no one says anything, then the OP, as a beginner, can only assume that code in answers is good code and that they should code that way. \$\endgroup\$
    – Jack M
    Aug 26, 2019 at 17:09
  • 1
    \$\begingroup\$ I guess you have a point. \$\endgroup\$
    – user203258
    Aug 26, 2019 at 17:12
8
\$\begingroup\$

Clearing the screen

os.system('cls') is a horrible way of clearing the screen.

The cross-platform os.system('cls' if os.name == 'nt' else 'clear') alternative is just as bad.

What this is doing is forking a child process, and in the child process (which is currently a copy of the current process's code and data memory):

  • replace the current process image with the "shell" image (such as cmd.exe or /bin/sh)
  • is run to interpret the 'cls' or 'clear' command, which in turn may fork yet another new child process to execute that command, if not a built-in command.

In addition, the cls or clear commands might not be the standard commands if another program by that name is discovered on the shell's $PATH.

You already using colorama, so simply use it to clear the screen:

import colorama

def clear_screen():
    print(colorama.ansi.clear_screen())

colorama.init()

clear_screen()
\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the tip! What if I didn't use colorama? Would importing colorama just to clear the screen still be an optimal way or shall I try something else? \$\endgroup\$
    – Mike
    Aug 27, 2019 at 14:07
  • \$\begingroup\$ colorama.ansi.clear_screen() is a verbose way of getting (IIRC) the string "\x1b[2J". If the terminal you are running in supports ANSI escape sequences, you may simply use print("\x1b[2J"). On Window, on a non-ANSI terminal, colorama.init() will replace sys.stdout with its own stream that filters out ANSI escape sequences and calls the appropriate Win32 functions in their place. The fact that you used Fore.GREEN and Fore.RED without calling colorama.init() suggests the terminal already supports ANSI escape sequences... \$\endgroup\$
    – AJNeufeld
    Aug 27, 2019 at 14:41
3
\$\begingroup\$

Layout

One trivial thing I didn't notice in the other answers is that

hangman_pics = ['''
 ...
=========''']

would be better written as

hangman_pics = [
  '''
 ...
=========''',
]

just because the triple-quoted multi-line strings slightly mess with the usual indentation protocol for the individual elements of such a list, doesn't mean that you should not bother to make the open and close square brackets as prominent as possible (with the ] re-establishing the indentation level for a human reader of the code which follows, which here is zero indentation).

I once coded something similar in which the triple-quoted strings had to span even more lines and were less similar, and I then felt that building the list dynamically kept it clearer. I don't think it's necessary here, but for reference:

hangman_pics = []
hangman_pics.append( '''
    ...
''' )
hangman_pics.append( '''
    ...
''' )
# etc. 
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.