Python Forum

Hi all,

I was hoping to get some feedback from a hangman game i just created. It runs correctly but being a beginner I'd really like to know what I've done wrong (or right) and what could be improved in terms of readability, maintainability, general coding practise and so on.

Feedback greatly appreciated!

def listtostrings(string):
    str = ""
    return str.join(string)


print("How many guesses do you want? ")
no_guesses = input()

word_dictionary = {1:"i", 2:"if", 3:"and", 4:"elif", 5:"print", 6:"python", 7:"jupyter", 8:"anaconda"}

print("How many letter word do you want to guess? ")
letter_word = input()

word = word_dictionary.get(int(letter_word))

translate = ""
for letter in word:
    translate = translate + "*"
print ("Your word is: "+ translate)

translate=[None]*len(word)

j=0
while j < len(word):
    translate[j]="*"
    j+=1

attempt = 1
i=0

while attempt<= int(no_guesses):
    print("Guess a letter: ")
    guess = input()

    if guess in word:
        for letter in word:
            if str(guess) == str(word[i]):
                translate[i] = guess
                i += 1
            elif str(translate[i]) in "abcdefghijklmnopqrstuvwxyxz":
                translate[i] = translate[i]
                i += 1

            else:
                translate[i] = "*"
                i += 1
        i = 0
        print (listtostrings(translate))
        if str(listtostrings(translate)) == str(word):
            print("Well done! you won the game")
            break

    else:
        print("Sorry your letter is not in the word!")
        attempt += 1

if attempt>int(no_guesses):
    print("Sorry! You lost the game")
    print("The word was: "+ word)

My code review is in the comments:

# From what this function is doing, it looks like the name is wrong. It's converting a list of strings into a string.
# The name sounds like it's going to make more than one string. The input is also a list, not a string, right?
# So "strings" is probably a better name for the parameter.
# str has an intrinsic meaning in Python, so you shouldn't use that. You can call is sep for "separator" or something.
# I've updated it closer to how I would do it, by not naming the separator. You should change the function name though.
def listtostrings(strings):
    return "".join(strings)
 
# You call int() on no_guesses multiple times. Doing it once here is cleaner. I also pass the prompt string to input
# to save a line calling print().
no_guesses = int(input("How many guesses do you want? "))
 
# I've formatted this in what I believe is a more readable way. I also left a comma on the last line, which is something
# I do that most others don't. I do it because I believe it leads to better version control. If you're not sure what I'm
# talking about, dont' worry about it for now.
word_dictionary = {
  1: "i",
  2: "if",
  3: "and",
  4: "elif",
  5: "print",
  6: "python",
  7: "jupyter",
  8: "anaconda",
}

# combined some lines; I would call this num_letters probably, and use it in place of len(word)
letter_word = int(input("How many letter word do you want to guess? "))
 
# Note that if they chose an int that is outside your range, word will be None here and you'll get an error from
# the loop below. I recommend using regular brackets, which would cause an exception instead, or writing custom
# logic to check for None and print a message based on that.
word = word_dictionary.get(letter_word)

# I've left the existing code here commented out for reference. I've replaced it with a one-liner more direct.

# translate = ""
# for letter in word:
#     translate = translate + "*"
# print ("Your word is: "+ translate)
 
# translate=[None]*len(word)
 
# j=0
# while j < len(word):
#     translate[j]="*"
#     j+=1

# I'd probably call this so_far by the way
translate = ['*'] * len(word)
 
# This loop will run no_guesses number of times. We can remove the attempt variable entirely.
for _ in range(no_guesses):
    # combined print and input
    guess = input("Guess a letter: ")
 
    if guess in word:
        # I use enumerate() instead of managing i manually. Consider Googling the documentation for it,
        # we'd be happy to answer followup questions.
        for i, letter in enumerate(word):
            # None of the str calls were necessary.
            if guess == word[i]:
                translate[i] = guess
            # Setting something to itself doesn't do anything, so I've commented this out. It should be removed.
            # elif str(translate[i]) in "abcdefghijklmnopqrstuvwxyxz":
            #     translate[i] = translate[i]
            # Unless I'm missing something, this assignment is also unnecessary.
            # else:
            #     translate[i] = "*"

        guess_so_far = listtostrings(translate)
        print(guess_so_far)
        if guess_so_far == word:
            print("Well done! you won the game")
            break
    else:
        print("Sorry your letter is not in the word!")

# The condition here (commented out) was always true. It's still always true, even though we removed
# one of the variables. So, I simply print since the loop exited.
# if attempt>int(no_guesses):
print("Sorry! You lost the game")
print("The word was: "+ word)

I basically went line by line, cleaning it up as I saw fit, leaving some commented out code for ease of comparison. I didn't consider any fundamental changes. Other than my comments, nothing stands out too strongly.

NV12

micseydel