8
\$\begingroup\$

This is my first ever program created after reading a book on Python. Do you have any suggestions for me? Anything that are considered bad habits that I should correct for my new project?

#HangMan - 2014
import random
import time

#TODO: add word support
#TODO: add already guessed letters

secret = ""
dash = ""
HANGMANPICS = ['''
  +---+
  |   |
      |
      |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
      |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
  |   |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
 /|   |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
 /|\  |
      |
      |
=========''', '''
  +---+
  |   |
  O   |
 /|\  |
 /    |
      |
=========''', '''
  +---+
  |   |
  O   |
 /|\  |
 / \  |
      |
=========''']

def create_hangman():
    create_hangman.guessess = create_hangman.guessess = 0
    create_hangman.already_guessed = ""
    #List of words, pick a word, then set it to a var
    words = ["soccer", "summer", "windows", "lights", "nighttime", "desktop", "walk"]
    d = random.randint(0, 6)

    #Tell the compiler we want the global secret var
    global secret
    #Change the global secret v to a string while we choose the word
    secret = str(words[d])

    #The blank spaces. Find how many letters the word is and replace it with underscores
    create_hangman.dash = ['_' for x in range(len(secret))]

    #Print the hangman
    print(HANGMANPICS[0], "\n",' '.join(create_hangman.dash))

def add_letter(letter):
    create_hangman.already_guessessed = create_hangman.already_guessed, letter

def guess():
    while True:
        think = input("Pick a letter: ")
        letter = think
        alreadyGuessed = ""
        if(len(letter) != 1):
            print("Please enter only one letter.")
        elif(letter not in 'abcdefghijklmnopqrstuvwxyz'):
            print("Please guess a letter.")
        elif(letter not in secret):
            wrong_word(create_hangman.guessess)
            add_letter(letter)
        elif(letter in secret):
            print("Congratulations!", letter, " was found!")
            remove_dash(letter)
            print_hangman()
            check()

def wrong_word(hmpic):
    create_hangman.guessess = create_hangman.guessess + 1
    hmpic = create_hangman.guessess
    if(create_hangman.guessess == 7):
        you_loose()
    else:
        print(HANGMANPICS[hmpic], "\n", ' '.join(create_hangman.dash), "\n", "That letter is not in the word.")

def print_hangman():
    print(HANGMANPICS[create_hangman.guessess] + "\n")
    print(' '.join(create_hangman.dash))

def you_loose():
    print("Sorry you lost! The correct word was", secret)
    play_again = input("Would you like to play again: ");
    if(play_again == "Y" or play_again == "y"):
        create_hangman()
        print("Creating a new game...")
    elif(play_again == "N" or play_again == "n"):
        print("Thanks for playing, bye!")
        quit()
    else:
        print("Error: Please choose either 'Y' or 'N'")
        return you_loose()

def you_win():
    print("Congratulations! You won and got the word", secret)
    play_again = input("Would you like to play again: ")
    if(play_again == "Y" or play_again == "y"):
        create_hangman()
        print("Creating a new game...")
    elif(play_again == "N" or play_again == "n"):
        print("Thanks for playing, bye!")
        quit()
    else:
        print("Error: Please choose either 'Y' or 'N'")
        return you_loose()

def check():
    if(''.join(create_hangman.dash) == secret):
        you_win()
    else:
        guess()

def remove_dash(letter):
    for i in range(len(secret)):
        if secret[i] == letter:
            create_hangman.dash = list(create_hangman.dash)
            create_hangman.dash[i] = letter

name = input("Whats your name? ")
print("Hey", name, "welcome to HangMan 1.6")
create_hangman()
guess()
\$\endgroup\$
8
\$\begingroup\$

You are making a common beginner mistake of misusing functions as if they were goto labels. For example, from the last line of the program, you call guess(), which calls check(), which calls guess(), which calls check(), which calls guess(), …, which calls check(), which calls you_win(), which can call you_loose() (?!)

At some point, you can hit ControlC to see the deep call stack that results from this weird mutual recursion. A properly structured program should have a nice, simple stack trace.

See other examples of code with this problem:

Here is an implementation restructured to use functions properly. Note the use of while loops.

The state of a game, at any point in a game, is entirely summarized by secret and guesses. Therefore, those two variables are frequently passed from calls within play_hangman(). An object-oriented solution would avoid such parameter passing, but I opted to stay somewhat close to the original design instead.

import random

HANGMANPICS = …

def pick_word():
    """Return a random word from the word bank."""
    words = ["soccer", "summer", "windows", "lights", "nighttime", "desktop", "walk"]
    return random.choice(words)

def print_hangman(secret, guesses):
    """Print the gallows, the man, and the blanked-out secret."""
    wrong_guesses = [guess for guess in guesses if not guess in secret]
    word_display = ' '.join(letter if letter in guesses else '_' for letter in secret)
    print(HANGMANPICS[len(wrong_guesses)])
    print()
    print(word_display)

def guess(secret, guesses):
    """Prompt for a single letter, append it to guesses, and return the guess."""
    while True:
        letter = input("Pick a letter: ")
        if len(letter) != 1:
            print("Please enter only one letter.")
        elif letter not in 'abcdefghijklmnopqrstuvwxyz':
            print("Please guess a letter.")
        else:
            guesses.append(letter)
            return letter

def won(secret, guesses):
    """Check whether the secret has been guessed."""
    right_guesses = [letter for letter in secret if letter in guesses]
    return len(right_guesses) >= len(secret)

def hanged(secret, guesses):
    """Check whether too many guesses have been made."""
    wrong_guesses = [guess for guess in guesses if not guess in secret]
    return len(wrong_guesses) >= len(HANGMANPICS)

def play_hangman():
    """Play one game of hangman. Return True if the player won."""
    secret = pick_word()
    guesses = []
    message = None
    while not hanged(secret, guesses):
        print_hangman(secret, guesses)
        if message is not None:
            print()
            print(message)
        new_guess = guess(secret, guesses)
        if won(secret, guesses):
            print("Congratulations! You won and got the word", secret)
            return True
        elif new_guess in secret:
            message = "Congratulations! {0} was found!".format(new_guess)
        else:
            message = "That letter is not in the word."
    print("Sorry you lost! The correct word was", secret)
    return False

def play_again():
    while True:
        play_again = input("Would you like to play again: ");
        if play_again == "Y" or play_again == "y":
            print("Creating a new game...")
            return True
        elif play_again == "N" or play_again == "n":
            print("Thanks for playing, bye!")
            return False
        else:
            print("Error: Please choose either 'Y' or 'N'")

while True:
    play_hangman()
    if not play_again():
        break
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for pointing that out. Can you show me an example of what you mean though? I see the examples of the code with the same problem. Wondering exactly how I can fix it. Thanks \$\endgroup\$ – ngngngn Dec 10 '14 at 21:29
  • \$\begingroup\$ I've added a complete solution. \$\endgroup\$ – 200_success Dec 10 '14 at 22:31
  • \$\begingroup\$ Alright, so I made a small translation calculator that add corods to other corods. Just so I make sure I understand this right, would this(pastebin.com/vFs78Pkj) be right? \$\endgroup\$ – ngngngn Dec 14 '14 at 23:53
  • \$\begingroup\$ Yes, the flow of control is much better! You might want to post that as a question for review. \$\endgroup\$ – 200_success Dec 14 '14 at 23:56
7
\$\begingroup\$

One thing that stands out immediately is that the structure of the program could be improved. This type of problem is highly amenable to a class that has various members to keep track of the state and methods to manipulate that state. You have done something a bit like that with your code by adding attributes to the create_hangman function but it's not as clear as just using a class. Generally speaking keeping track of state by using global variables make it much harder to reason about the flow of the program as functions execution now depends on these global variables. You could call the same function twice with the same parameters but get different results, then to track down why you'd have to examine all the code and not just the code in the relevant function. When you do:

def create_hangman():
    create_hangman.guessess = 0
    create_hangman.already_guessed = ""

(Note the redundant part in the first line didn't make any sense so I cleaned that up) You are adding some attributes to that function. In the REPL you can see what happens:

>>> def create_hangman():
...     create_hangman.guesses = 0
...     create_hangman.already_guessed = ""
... 
>>> dir(create_hangman)
['__call__', '__class__', '__closure__', '__code__', '__defaults__',
 '__delattr__', '__dict__', '__doc__', '__format__', '__get__',
 '__getattribute__', '__globals__', '__hash__', '__init__', '__module__',
 '__name__', '__new__', '__reduce__', '__reduce_ex__', '__repr__',
 '__setattr__', '__sizeof__', '__str__', '__subclasshook__',
 'func_closure', 'func_code', 'func_defaults', 'func_dict', 'func_doc',
 'func_globals', 'func_name']
>>> create_hangman.guesses
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'function' object has no attribute 'guesses'

Interestingly enough you should note that until you execute the function those attributes don't exist. This isn't going to be a problem in the program in the question but it is good to be aware of this.

>>> create_hangman()
>>> dir(create_hangman)
['__call__', '__class__', '__closure__', '__code__', '__defaults__',
 '__delattr__', '__dict__', '__doc__', '__format__', '__get__',
 '__getattribute__', '__globals__', '__hash__', '__init__', '__module__',
 '__name__', '__new__', '__reduce__', '__reduce_ex__', '__repr__', 
 '__setattr__', '__sizeof__', '__str__', '__subclasshook__',
 'already_guessed', 'func_closure', 'func_code', 'func_defaults',
 'func_dict', 'func_doc', 'func_globals', 'func_name', 'guesses']

Now that we call the function we see that the attributes we defined are now in there. These are what are saving the state. We can be a lot more explicit about this though if we just use a class, the first part of the class will look like this:

class Hangman():
    """Class to keep track of data for our hangman program"""
    def __init__(self, words):
        self.words = words
        self.current_secret_word = random.choice(words)
        self.guesses = 0
        self.already_guessed = {}
        self.dash = ['_' for ch in current_secret_word]

Note that I've added a parameter here with the words so we can then instantiate the program with various different word lists fairly easily. We can then use the random.choice to pick out our random word from the list. All the state we need to run our program is now here so we can remove the global variables, for example secret is now now longer needed.

There's a couple of other changes too. Generally speaking with strings you don't need to do range(len(my_string)) you can just iterate of the string directly. I've also changed the already_guessed data type to be a set because really that's what you want here.

Now we just need to make the appropriate functions into methods:

class Hangman():
    """Class to keep track of data for our hangman program"""
    def __init__(self, words):
        self.words = words
        self.current_secret_word = random.choice(words)
        self.guesses = 0
        self.already_guessed = {}
        self.dash = ['_' for ch in current_secret_word]

    def add_letter(self, letter):
        self.already_guessed.add(letter)

For the most part this just involves updating the names to refer to the class variables.

One other thing I notice is that there is no documentation in the code here, adding in some docstrings can be very beneficial.

\$\endgroup\$
  • \$\begingroup\$ Thanks! A very clear post with a lot of information that never occurred to me. I will defiantly try using classes instead to organize it. \$\endgroup\$ – ngngngn Dec 10 '14 at 21:28
2
\$\begingroup\$

Some issues in create_hangman():

  • To pick a word from a list, use random.choice().
  • Global variables are bad. Game state information, such as the current secret word, should be stored in the stack or in an object.
  • Storing state information as attributes of the create_hangman function is very weird, as @shuttle87 points out.
  • To create a list of underscores, use ['_'] * len(secret).
\$\endgroup\$
2
\$\begingroup\$

I recommend adding a line space after the congratulations and one before Pick a letter... To do this just type \n within the print statement. Also, like 200_success mentioned, adding more words... It would help you alot to use lists to make the game longer, or actually have replayability. Use random.choice("The lists name here") to select a random element from a list of words. Use len("Variables name) to get the number of _ you need. I do not know exactly how you would make the input work, but I am guessing using lists and variables and a few loops. And I just noticed you do use \n in your code, i just recommend using it more. You also need to add Guessed letters, whisch would be easy. Just use this:

GuessedLetters = []
guess = input("This is where you have your input")
GuessedLetters.append(guess)

#For when you you want to print it
print(GuessedLetters)

So to finish this up, your coding style is great. Keep up the good coding, you are doing greate.

\$\endgroup\$
  • \$\begingroup\$ Thanks, I just made a way to do that yesterday. I used create_hangman.already_guessed = "" then did create_hangman.already_guessed = create_hangman.already_guessed, letter which clearly is the wrong way for me to do that now that I read these posts. Here's the thing I can't find a way to access the GuessedLetters to do .append on it. How do I access variables that were created in a function? I used create_hangman.guessed_letters as a fix for that. Another better way? \$\endgroup\$ – ngngngn Dec 10 '14 at 21:20
  • \$\begingroup\$ @user192881 If you put it in a while loop, you can just have the variable (Which must be a list for .append to work) be equal to blank, so when the game resets so does the list. Make sure it is not in the actual definitions but outside the definitions because when you get a correct letter, it resets the letter. Also, in the def Guess, do not put the list there, but add the letter there, and add another statement to make sure the user does not guess the same letter. Add the letter at the end of the definition. \$\endgroup\$ – Amateur Programer Dec 11 '14 at 15:19
2
\$\begingroup\$
Sorry you lost! The correct word was lights
Would you like to play again: yes
Error: Please choose either 'Y' or 'N'
Sorry you lost! The correct word was lights
Would you like to play again: 

I see 2 flaws in user experience:

  • 1) yes is not recognized, to fix this use: play_again.lower().startswith('y')
  • 2) You repeat that I lost when I enter a wrong input.

def new_game(message):
    print(message.format(secret))
    play_again = input("Would you like to play again: ");
    if(play_again == "Y" or play_again == "y"):
        create_hangman()
        print("Creating a new game...")
    elif(play_again == "N" or play_again == "n"):
        print("Thanks for playing, bye!")
        quit()
    else:
        print("Error: Please choose either 'Y' or 'N'")
        return new_game(message)

def you_loose():
    new_game("Sorry you lost! The correct word was {}")


def you_win():
    new_game("Congratulations! You won and got the word {}")

Use the above because the only thing that changes between win and lose is the message.


Be consistent with your naming style: alreadyGuessed should be already_guessed

\$\endgroup\$
  • 1
    \$\begingroup\$ I would favour play_again.startswith('y'). \$\endgroup\$ – Veedrac Dec 10 '14 at 20:35
  • 1
    \$\begingroup\$ Even better play_again.lower().startswith('y') so that "YES" and "Yes" are also accepted. \$\endgroup\$ – Caridorc Dec 10 '14 at 20:41
  • \$\begingroup\$ Heck, since this is Python 3 I'd suggest play_again.casefold().startswith('y'), even though it won't matter in this case. \$\endgroup\$ – Veedrac Dec 10 '14 at 21:10
  • \$\begingroup\$ Awesome! It never occurred to me to do it that way. Looks a lot simpler, thanks a lot. \$\endgroup\$ – ngngngn Dec 10 '14 at 21:16

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.