5
\$\begingroup\$

I recently started learning Python, and made a simple text based Hangman game. I wanted to know if there could be anything done to make the code cleaner or more efficient.

import random
import os
import sys

def main():
    pickword()

def start(word, twoplayer):
    wordarray = ''
    userguess = []
    guessright = 0
    lettersright = 0
    if len(word) < 5:
        tries = 6
    else:
        tries = 10
    usedletters = []

    if twoplayer == False:
        print(word)
        print("Welcome to hangman")
        print("I will think of a word and you will have to guess it, are you ready!")
        input("Press enter to continue")
    else:
        print("Welcome to hangman")
        print("Player 1 has already thought of a word, let's begin")
        input("Press enter to continue")

    lengthword = len(word)

    if twoplayer == False:
        for i in range(1, lengthword):                  #creates a new variable, all underscores
            userguess.append("_ ")
    else:
         for i in range(1, lengthword):                  #creates a new variable, all underscores
            userguess.append("_ ")



    wordarray = (list(word))

    if twoplayer == False:
        for i in range(0, lengthword):                  #removes \n from end of word
            if wordarray[i] == "\n":
                wordarray.remove(wordarray[i])
                break

    usedletters = []
    getuserguess(wordarray, userguess, tries, usedletters, word)

def pickword():
    word = 0
    twoplayer = input("Do you want to play 2 player[Y/N]")
    if twoplayer.lower() == "y":
        word = input("Player 1, enter the word you want player 2 to guess, don't let them see! ")

        if len(word) < 3:
            print("That word is too short")
            pickword()

        print("\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n")
        print("\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n")
        print("\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n")

        word = word.lower()
        twoplayer = True
        start(word, twoplayer)
    else:
        try:
            word = random.choice(open('hangman.txt').readlines())
            twoplayer = False
            start(word, twoplayer)

        except:
            input("Error, no word list found, press enter to exit")
            sys.exit()



def getuserguess(wordarray, userguess, tries, usedletters, word):

    print(' '.join(userguess))
    guess1 = input("Enter your guess: ")

    if len(guess1) < 1 or len(guess1) > 1 or guess1.isdigit():
        print("Invalid input")
        getuserguess(wordarray, userguess, tries, usedletters, word)
    else:
        checkuserguess(wordarray, userguess, guess1, usedletters, tries, word)



def lose(word):
    again = input("Sorry, you lost, the word was {}Would you like to play again[Y/N]".format(word))
    if again.lower() ==  "y":
        pickword()
    else:
        sys.exit()

def win():
    again = input("Congrats, you won hangman, do you want to play again?[Y/N]")
    if again == "Y" or "y":
        pickword()
    else:
        sys.exit()

def checkuserguess(wordarray, userguess, guess1, usedletters, tries, word):

    guess1list = (list(guess1))

    if guess1list[0] in usedletters:
        print("You already used that letter")
        getuserguess(wordarray, userguess, tries, usedletters, word)

    if guess1 in wordarray:
        usedletters.append(guess1)
        for i in range(0, len(wordarray)):
            if wordarray[i] == guess1list[0]:
                userguess[i] = guess1list[0]
                usedletters.append(guess1list[0])

        if "_ " not in userguess:
            print(' '.join(userguess))
            win()
        else:
            getuserguess(wordarray, userguess, tries, usedletters, word)

    else:
        if tries > 0:
            print("Sorry, that letter is not in the word, you have ", tries, " remaining")
            tries -= 1
            getuserguess(wordarray, userguess, tries, usedletters, word)
        else:
            lose(word)





main()
\$\endgroup\$
8
\$\begingroup\$

For this, I am going to go through your program in the order it is written:

  • Don't use if twoplayer == False:. You can simply say if not twoplayer: You don't even need that, though the first time because you do the same thing in the if and elseblocks`.

  • You can use " ".join("_" * lengthword) instead of for i in range(1, lengthword): userguess.append("_ "). Besides making it cleaner, you weren't adding enough underscores. The second argument to range() is exclusive. Imagine if the length of the word is two. In that case, range(1, 2) returns [1] because the second argument is exclusive. That has length 1 instead of the same length as the word: 2.

  • Don't use wordarray.remove(wordarray[i]). Instead, use wordarray.pop(i). If you use .remove(), the first occurrence of the word will be removed, not necessarily the one you want to be removed.

  • When you ask "Do you want to play 2 player?", you account for "y" and anything else. What if the user says "yes"? It would still count as no. Instead, you should use a while loop:

    twoplayer = input("Do you want to play 2 player? [Y/N] ")
    while twoplayer.lower() not in ("y", "n"):
        twoplayer = input("[Y/N] ")
    
  • Instead of setting twoplayer inside of the if and else blocks, you can just do twoplayer = ("n", "y").index(twoplayer.lower())

  • If the word is too short, you don't want to call pickword() again because pickword() asks "Do you want to play 2 player?" If the word is too short, that doesn't mean the user wants to change his mind on that question. Instead, use a loop:

    while len(word) < 3:
        print("That word is too short")
        word = input("Player 1, enter the word you want player 2 to guess, don't let them see! ")
    
  • How do you know how many lines to print? Instead of printing an arbitrary number of lines, print("\033c") will clear the terminal.

  • You call start(word, twoplayer) in both the if and the else block. That is a sign that it shouldn't be in either one. Put it after both blocks so that you aren't duplicating code.

  • While we're at that point in the program, you want only one line in your try: block: word = random.choice(...). twoplayer should be defined as I outlined above, so we don't need that here at all and start(word, twoplayer) will be outside of the if and else blocks, so we don't need that. Besides, the only reason you want the try block is because you are dealing with a file. If anything else is throwing an error, you want the traceback because it shouldn't be.

  • Don't use a bare except: You have certain errors that you think you might get; list them here. In this case, it's except IOError:

  • sys.exit(), when given an argument, uses that argument as the exit code. In this case it is failure, so you should use sys.exit(1).

  • Your check for guess1's validity is not the best. To make sure that it is one character, use len(guess1) != 1 as the check. Also, guess1.isdigit() is probably not the only thing that is invalid. For example, $ shouldn't be in the word, right? Instead, use not guess1.isalpha() to make sure it is an alphabetic character.

  • Instead of using recursion and typing out all of those variable names, use a loop:

    print(' '.join(userguess))
    guess = input("Enter your guess: ")
    while len(guess) != 1 or not guess.isalpha():
        print("Invalid input")
        guess = input("Try again: ")
    
    checkuserguess(wordarray, userguess, guess, usedletters, tries, word)
    
  • You have duplicate code in lose() and win(): Would you like to play again. Instead, define a function that asks that question, and use it in lose() and win(). Make sure that in that function you use if again.lower() == "y": as you did in lose(). if again == "Y" or "y": does not work how you would expect it to. See this StackOverflow post for why.

  • You check if "_ " is in userguess, but you should be checking for just the underscore, not the underscore with a space. If the underscore is at the end of the word, there shouldn't be a space. If there is, you're doing it wrong and you shouldn't depend on it.

This is my first complete review, so if anyone has suggestions, I would be happy to modify it.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the input, I understand everything except the part where you say use twoplayer = ("n", "y").index(twoplayer.lower()). Could you explain this a little bit more. \$\endgroup\$ – vkumar Mar 13 '16 at 21:48
  • \$\begingroup\$ twoplayer.lower() will be either "y" or "n". If it is "y", then ("n", "y").index(twoplayer.lower()) will be 1. If it is "n", then it will be 0. Since 1 has a boolean value of True and 0 has a boolean value of False, twoplayer can now be used as a boolean. \$\endgroup\$ – zondo Mar 13 '16 at 21:50
  • \$\begingroup\$ One more thing, could you please explain how I should use " ".join("_" * lengthword) \$\endgroup\$ – vkumar Mar 13 '16 at 22:02
  • \$\begingroup\$ You can take out the for loop and put that in. I edited my answer to make that more clear and to add one more thing on that. \$\endgroup\$ – zondo Mar 13 '16 at 22:06
  • \$\begingroup\$ Might just make it that little clearer for twoplayer = ("n", "y").index(twoplayer.lower()) to use twoplayer = bool(("n", "y").index(twoplayer.lower())) to make it explicit that the index will be a boolean value? Great work @zondo ! \$\endgroup\$ – srattigan Aug 31 '17 at 18:39

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.