10
\$\begingroup\$

This is my first game and I'm really proud of it, but I think it's horribly messy. Is there any way to improve it?

#HANGMAN

# Valid Word Checker

def valid_word(word):

    ill_chars = ["!","@","#","$","%","^","&","*","(",")",",",".", " ","1","2","3","4","5","6","7","8","9","0"]
    for i in word:
        if i in ill_chars:
            print(f"\nError: It must be a letter. No symbols, numbers or spaces allowed. \n\n '{i}' is not allowed\n")
            return False
    if len(word) > 12:
        print("\n\nError: Word is too long. Use a word with eight characters or less.")
        return False
    return True

word_list = []
spaces = []
guessed_letters = []

ill_chars = ["!","@","#","$","%","^","&","*","(",")",",",".", " ","1","2","3","4","5","6","7","8","9","0"]

head ="    O"
armL = "   /"
torso = "|" 
armR = "\\"
legL = "/"
legR = " \\"

hangman_parts = [head, armL, torso, armR, legL, legR]
hangman_progress = ["", 
                    "|", 
                  "\n|", 
                  "\n|"]

hangman_final = ["|~~~~|\n"]

# Check if word is valid

wordisValid = False
while True:
    word = input("\n\n\nChoose any word\n\n\n").lower()
    if valid_word(word) == True:
        wordisValid = True
        break
    else:
        continue

# Add to list
for i in word:

    word_list.append(i)
    spaces.append("_ ") 

# Main Game Loop
bad_letter = 0

while wordisValid == True:  


    print("".join(hangman_final))
    print("\n\n\n\n")
    print("".join(spaces))
    print("\n\nThe word has: " + str(len(word)) + " letters.")
    print("\nYou've tried the following letters: " + "\n\n" + "".join(guessed_letters) + "\n\n")


    # Winning Loop

    if "".join(spaces) == word:
        print(f"YOU WIN! The word was: {word}")
        break
    # Choose Letters

    player_guess = input("\n\nPlease choose a letter: \n\n\n\n").lower()
    guessed_letters.append(" " + player_guess)

    if player_guess in ill_chars:
        print(f"\nError: It must be a letter. No symbols, numbers or spaces allowed. \n\n '{player_guess}' is not allowed\n")
    elif len(player_guess) > 1:
        print("\nError: You must use one letter.\n")
    elif player_guess == "":
        print("\nError: No input provided.\n")


    # Wrong Letter
    elif player_guess not in word_list:

        bad_letter += 1

        if bad_letter == 1:
            hangman_final.append(hangman_progress[1] + head)

        elif bad_letter == 2:
            hangman_final.append(hangman_progress[2] + "    " + torso)

        elif bad_letter == 3:
            hangman_final.pop(2)
            hangman_final.append(hangman_progress[2] + armL + torso)

        elif bad_letter == 4:
            hangman_final.pop(2)
            hangman_final.append(hangman_progress[2] + armL + torso + armR)

        elif bad_letter == 5:
            hangman_final.append(hangman_progress[3] + "   " + legL)

        elif bad_letter == 6:
            hangman_final.pop(3)
            hangman_final.append(hangman_progress[3] + "   " + legL + legR)
            print("\n\nThe word was: " + word)
            print("\n\n\n\n\n\n\n" + "".join(hangman_final))
            print(" YOU GOT HUNG ")
            break


        print("".join(hangman_final))
        print("\n\n\n\n")
        print("".join(spaces))
        print("\n\nThe word has: " + str(len(word)) + " letters.")
        print("\nYou've tried the following letters: " + "\n\n" + "".join(guessed_letters) + "\n\n")
        print(f"\n\n\n{player_guess} is not in the word. Try again.\n\n")


    # END GAME
    if bad_letter == 6:
        break

    # Add letters guessed to a list
    counter = 0
    for i in word:

        if player_guess == i:
            spaces[counter] = player_guess
        counter += 1

Even though it's working, I'm not sure that I have the right idea when it comes to making this type of project.

\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! You can take the tour for a quick overview of the site. \$\endgroup\$ – L. F. Sep 24 at 10:03
16
\$\begingroup\$

While it's clear that you're new to python, it's still pretty good that you got it to run first time. Good job!

Input Validation

Currently, you have a list of forbidden characters. While that can work, by default python allows unicode input. That means there's literally thousands of letters someone can input. I suggest instead using a whitelist of valid inputs. Like this:

VALID_LETTERS = "abcdefghijklmnopqrstuvwxyz"  # This won't ever change, and typically this sort
# of things  is a module-level variable. 

def valid_word(word):
    for letter in word:
        if letter not in VALID_LETTERS:
            print(f"Letter {letter} is not a valid letter. Please use just a-z.")
            return
    if len(word) > 8:  # This was 12 in your script. Little mistake? And you might want a 
        # module-level constant for this as well.
        print("\n\nError: Word is too long. Use a word with eight characters or less.")
        return False
    return True

main() and game()

Generally, in python we never execute code at the moment the module is loaded. Read here for details, and how we guard against it. I'll come back to this later. For now, it means we should put as much as possible into functions - lets call them main() and game().

main() will be the main menu function. Since we don't have a true menu, this will basically start a game, and perhaps ask if the player wants to play again.

game() will be a function that lets us play a single game. Most of this will be a loop that gets player input and calculates the game state. Basically everything in your script that isn't another function should be in the game() function.

I'll cut some variables we don't need later on. I'll explain them when we get to where we use them.

Like this:

def game():
    # Variable naming is important. This is an example of a good name.
    guessed_letters = []

    # ill_chars = [...]  We already have this variable before here!

    # Lets assign the parts directly. We can index them as in "3rd part".
    # And for ourselves, we comment the meaning to make sure we still know what it means a year 
    # from now.
    hangman_parts = [
        "    O",  # Head
        "   /",  # Left Arm
        "|" ,  # Torse
        "\\",  # Right Arm
        "/",  # Left Leg
        " \\"  # Right Leg
    ]
    hangman_progress = ["", 
                        "|", 
                      "\n|", 
                      "\n|"]

    hangman_final = ["|~~~~|\n"]

    # Check if word is valid

    # Naming: should be word_is_valid, to be consistent with your other variables.
    word_is_valid = False
    while not word_is_valid:
        word = input("\n\n\nChoose any word\n\n\n").lower()
        word_is_valid = valid_word(word)
    # With this loop condition, it'll keep asking until valid_word returns True. So we can just 
    # assign that value to word_is_valid directly.

    # Since this is the word we're trying to guess, lets just push it upwards in
    # the console a lot, so we won't see it all the time:
    print("\n" * 100)  # Prints 100 newlines.

    # We don't need word_list, just word will do. We don't need spaces either.
    while True:  # Main game loop. We don't need to check for anything, we'll just use break or
        # return to get out.

Now we're going to use a list comprehension. It's almost the same as a generator expression, but it gives us a true list, so we can ask python how long it is:

        # Calculate how many bad letters we have with a list comprehension:
        bad_letter = len([letter for letter in guessed_letters if letter not in word])
        draw_hangman(bad_letter)
        if bad_letter > 5:
            print("\n\n\n\nYOU GOT HUNG")
            return
        print("\n\n\n\n")

You used to draw your hangman at two different places, while it's more practical to do it in one place. It also ensures that if you want to change something, you'll only have to change it once.

I've also changed how to draw it. It's much more useful to have a function for this drawing operation, and have it calculate how to draw from the number of bad letters so far. This makes the drawing independent of the current game state. Here's the version I came up with, based on your modifications of hangman_final:

def draw_hangman(bad_letter):
    print(hangman_final, end="")
    if bad_letter > 0:  # Head
        print(hangman_progress[1] + hangman_parts[0], end="")
    if bad_letter == 2:  # Torso and arms
        print(hangman_progress[2] + "    " + hangman_parts[2], end="")
    elif bad_letter == 3:
        print(hangman_progress[2] + "".join(hangman_parts[1:3]), end="")
    elif bad_letter > 3:
        print(hangman_progress[2] + "".join(hangman_parts[1:4]), end="")
    if bad_letter > 4:  # Legs
        print(hangman_progress[3] + "   " + hangman_parts[4], end="")
    if bad_letter > 5:
        print(hangman_parts[5], end="")

Here we need to print the letters we guessed, and underscores otherwise. We can calculate and print it in one line:

        print(" ".join(letter for letter in word if letter in guessed_letters else "_"))

For every letter, this will print it if it's contained in guessed_letters, and otherwise print an underscore. This is a generator expression. This one is almost equal to the function:

def make_word(word, guessed_letters):
    result = []
    for letter in word:
        if letter in guessed_letters:
            result.append(letter)
        else:
            result.append("_")
    return result

I say almost, because it's not exactly a list that comes out of it, even if it acts the same if you use it in a for loop or feed it to a function that does, like "".join(). It also acts more like a loop than a function, but that's not really important right now.

We feed the result to the join. And note that we feed it to " ".join(), with a space in between, which will put a space between every letter like you did before.

        print(f"\n\nThe word has: {len(word)} letters.")  # f-strings are shorter, and easy to use. 
        print(f"\nYou've tried the following letters:\n\n{''.join(guessed_letters) }\n\n")
        # For a string literal inside an f-string, use the other type of quotes

        # This is another generator expression - this one returns a not-quite-list of booleans, and
        # the all() builtin function returns True if and only if all results of the generator
        # expression are True.
        if all(letter in guessed_letters for letter in word):
            print(f"You WIN!\nThe word was: {word}")
            return

        input_valid = False
        while not input_valid:
            new_letter = input("\n\nPlease choose a letter: \n\n\n\n").lower()
            # VERY GOOD that you lowercase it! Prevents tons of weird behaviour.
            if len(new_letter) > 1:
                print("Please enter a single letter only.")
            elif new_letter not in VALID_LETTERS:
                print(f"{new_letter} is not a valid letter. Please enter a letter a-z")
            elif not new_letter:  # Empty strings evaluate to False
                print("Please enter a letter before pressing enter.")
            elif new_letter in guessed_letters:
                print("You already guessed {new_letter} before!")
            else:
                guessed_letters.append(new_letter):
                input_valid = True

This is pretty close to your input. But I put it in a loop, so if we have an invalid input, we don't have to traverse the entire game loop again. I use a pretty simple signalling boolean to keep it going, but perhaps there's more elegant ways to do this. Never wrong to keep it simple, though.

Right now, to play the game, all you have to do is call the game() function. But perhaps we want to play multiple games in series? Lets make our main() function:

def main():
    game()
    while "y" in input("Play again? [Y/n]").lower():
        game()

Why is this in a separate function? Again, for the sake of extensiblility. Perhaps, we want to import our game from another module. If so, we don't want it to run when we import, but when we call a function. So we end the script with this guard:

if __name__ == "__main__":
    main()

If we execute this file directly, this will run our main() function and let us play the game. But if we import it, it won't, so we can call the function when we want it.

\$\endgroup\$
  • 4
    \$\begingroup\$ Instead of defining VALID_LETTERS manually: from string import ascii_lowercase as VALID_LETTERS? \$\endgroup\$ – 409_Conflict Sep 24 at 11:30
  • \$\begingroup\$ Would work just as well, but personally I wouldn't import for something like this. Unless you need localization for countries with different alphabets, but in that case you'd need to go a lot more complicated than that. \$\endgroup\$ – Gloweye Sep 24 at 11:33
  • \$\begingroup\$ If the whitelist is just lower case letters, one can also check the entire input string using word.islower() and word.isascii(). \$\endgroup\$ – GZ0 Sep 24 at 12:58
  • 1
    \$\begingroup\$ Using both, you mean? Yeah, it could work, but I don't think the current is a bad solution. I wanted to keep it someone similar to the question, and I also reused it to check the letters again. And if it was performance limited, I'd use ord() and check it's value instead. However, This is both readable and generalizable while not being slow or an antipattern. So that's why I think this is the better way. \$\endgroup\$ – Gloweye Sep 24 at 13:14
  • \$\begingroup\$ Thank you so much for your amazing detailed explanation! \$\endgroup\$ – Python Novice Sep 24 at 17:59

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.