I was need of help and besides helping me, people told me my code was, although working, a complete mess and under the "Spaghetti Code" category.
Can someone please assist me on how to refactor my code for efficiency and readability purposes?
Any help you can give me is much appreciated.
This is my code: The Hangman Game on Python
import random
import time
from theHangmanStages import * # This imports ASCII hangman figure.
from theHangmanIntro import *
intro() # This just imports an ASCII art as an intro.
startGame = input()
game = 'on'
words = ''' In a strange city lying alone
Far down within the dim West
Where the good and the bad and the worst and the best
Have gone to their eternal rest
There shrines and palaces and towers'''.lower().split()
secretWord = random.sample(words, 1)
numberOFLetters = len(secretWord[0])
missedLetters = ''
alreadyGuessed = ''
print('I\'m thinking of a word...')
time.sleep(1)
print(f'\nThe word has {numberOFLetters} letters...')
for char in secretWord[0]:
print('_ ', end='')
wordSpaces = len(secretWord[0]) * '_'
listedWordSpaces = list(wordSpaces)
def char_positioner(word, guessAttempt):
i = word.index(guessAttempt)
if guessAttempt in word:
listedWordSpaces[i] = word[i]
print(listedWordSpaces)
return listedWordSpaces
def converter(list):
# Converts a list of chars into a string.
initialStr = ''
for char in list:
initialStr += char
print(initialStr)
return initialStr
def getGuess(guess):
# Returns the letter the played entered. This function makes sure the
# player entered a single letter and not something else.
while True:
guess = guess.lower()
if len(guess) != 1:
print('Please enter a single letter.')
guess = input()
elif guess in alreadyGuessed:
print('You have already guessed that letter')
guess = input()
elif guess not in 'abcdefghijklmnopqrstuvwxyz':
print('Please enter a letter.')
else:
return guess
misses = 0
while game == 'on':
print('\nChoose a letter')
chosenLetter = input()
guess = getGuess(chosenLetter)
if guess in secretWord[0]:
print('\nYour guess is correct!')
alreadyGuessed += guess
position = char_positioner(secretWord[0], guess)
converter(position)
elif guess not in secretWord[0]:
print('\nWrong letter, try again.')
missedLetters += guess
alreadyGuessed += guess
print('\nMissed letters: ', missedLetters, end=' ')
misses += 1
if misses == 1:
hangmanOne()
elif misses == 2:
hangmanTwo()
elif misses == 3:
hangmanThree()
elif misses == 4:
hangmanFour()
elif misses == 5:
hangmanFive()
elif misses == 6:
hangmanSix()
print('You lose! - The word was:' + '"' + secretWord[0] + '"')
break
else:
print('Wrong input.')
I stumbled upon
secretWord = random.sample(words, 1)
where my expectation was that secretWord is a word (i.e. a string). However, you always access it with [0]. I had to look in the debugger to see that secretWord is a list containing 1 word. This violates the Clean Code "principle of least astonishment" and "don't repeat yourself" (you need to repeat the indexer [0] in several places).
numberOFLetters should be numberOfLetters.
You have already calculated the number of letters, but don't make consistent use of it. wordSpaces = len(secretWord) * '_' could be wordSpaces = numberOfLetters * '_'.
The same way you calculate wordSpaces = numberOfLetters * '_', you could get rid of the loop printing underscores:
for char in secretWord:
print('_ ', end='')
could be
print(numberOfLetters * '_ ')
You have
listedWordSpaces = list(wordSpaces)
where wordSpaces is a string containing underscores. A string is already a list of chars. To me, that sounds duplicate, violating the KISS ("keep it simple and stupid") and YAGNI ("You ain't gonna need it") principle. In fact, it seems that decision forces you to write more code: def converter(list): is the reversing operation.
BTW, that's not only a function, it also has a side effect: it prints the string. And the return value is never used. Also, that method is more complex than needed. initialStr = ''.join(list) should give the same result.
A similar printing side effect is in def char_positioner(word, guessAttempt):. The variable name at the usage is position = char_positioner(secretWord, guess), where I expected position to be an int, but obviously it's a list of characters. That's confusing.
You have a PEP 8 violation in this line:
misses = 0
because there should be 2 newlines after a function definition.
Also a PEP 8 issue with spacing around operators here:
print('You lose! - The word was:' + '"' + secretWord[0] + '"')
and potentially a missing newline at the end (might be a SO copy/paste issue, though).
You also have several shadowing issues:
def converter(list): [...]
for char in list: [...]
def getGuess(guess): [...]
I also see a DRY ("don't repeat yourself") issue here:
if misses == 1:
hangmanOne()
[...]
Imagine a more complex hangman. Would you write 100 methods and 100 if/else statements?
Some function names are named like nouns but should be verbs instead:
def converter(list): [...]
def char_positioner(word, guessAttempt): [...]
Personally, I'd like to see type hints, so I can better understand what the expected type of an argument is and what the return type is.
I see while game == 'on':. Typically we'd not use a string here, but a boolean. It also seems there's no way to stop the game by turning it "off". The exit case is implemented via break, so this could be a while True loop at the moment. As the game evolves, the break statement is a bit brittle and might not break out of the outermost loop any more. A boolean condition like while isPlaying or while not isGameOver might be a better choice.