Object-Oriented Hangman In Python [Follow-Up]

Question

This is a follow up after revisions made from this question.

I've separated the program into 2 classes. The HangManData class is used to store game data such as the word, letters guessed(separating the ones actually in the word from those that arent), remaining number of guesses and the phase of the image.

The second class UI controls user interaction and sends the data back to the HangManData class to be stored.

This is definitely a lot more readable compared to the original code. Let me know of any improvements I could make, especially if there is something I've done unnecessarily that could be simplified.

I'll be leaving the unnecessary phases file and word file out to prevent any repetition, refer to original question if required

import random
from phases import hang_phases
class HangManData:
 def __init__(self, random_word):
 self.word = random_word
 self.missed_letters = []
 self.successful_letters = []
 self.man = hang_phases
 self.remaining_chances = 9
 def get_man(self):
 self.man = hang_phases[self.remaining_chances]
 return self.man
 def get_word(self):
 return self.word
 def set_successful_letters(self, letter):
 self.successful_letters.append(letter)
 def get_successful_letters(self):
 return self.successful_letters
 def set_missed_letters(self, letter):
 self.missed_letters.append(letter)
 def get_missed_letters(self):
 return self.missed_letters
class UI:
 def __init__(self):
 self.word_list = open('Words').read().split("\n")
 self.word = random.choice(self.word_list).upper()
 self.filled = ""
 def generate_word(self):
 return self.word
 def fill_blanks(self, hm):
 self.filled = ""
 for i in range(0, len(hm.get_word())):
 if hm.get_word()[i] in hm.get_successful_letters():
 self.filled += hm.get_word()[i]
 else:
 self.filled += "_"
 return self.filled
 def display(self, hm):
 print(hm.get_man())
 print(self.fill_blanks(hm))
 print(f"Incorrect Letters Guessed {hm.get_missed_letters()}")
 def play(self):
 hm = HangManData(self.generate_word())
 while True:
 self.display(hm)
 if hm.get_word() == "":
 break
 elif "_" not in self.fill_blanks(hm):
 print("Congrats!")
 break
 elif hm.remaining_chances == 0:
 print(f"The Word Was {hm.get_word()}")
 break
 else:
 guessed_letter = input("Guess A Letter: ").upper()
 if guessed_letter == "QUIT":
 print(f"The Word Was {hm.get_word()}")
 break
 elif guessed_letter in hm.get_missed_letters() or guessed_letter in hm.get_successful_letters():
 print("You Already Guessed This Letter")
 elif guessed_letter in hm.get_word():
 number_of_appearances = hm.get_word().count(guessed_letter)
 if number_of_appearances == 1:
 print(f"{guessed_letter} Appears Once In The Word")
 else:
 print(f"{guessed_letter} Appears {number_of_appearances} Times In The Word")
 hm.set_successful_letters(guessed_letter)
 else:
 hm.set_missed_letters(guessed_letter)
 hm.remaining_chances -= 1
def main():
 play = True
 while play:
 u = UI()
 u.play()
 while True:
 print("Play Again?")
 play_again = input("Y/n > ").upper()
 if play_again == "Y":
 print("Setting Up...")
 break
 elif play_again == "N":
 print("Goodbye")
 play = False
 break
 else:
 print("Invalid Input")
if __name__ == "__main__":
 main()

Answer 1

This is minor, but the thing that jumps out to me is the fill_blanks method. First thing is instead of looping through an index, you can just iterate through letters in the phrase:

def fill_blanks(self, hm):
 self.filled = ""
 for letter in hm.get_word():
 if letter in hm.get_successful_letters():
 self.filled += letter
 else:
 self.filled += "_"
 return self.filled

Then the if/else can be on one line:

def fill_blanks(self, hm):
 self.filled = ""
 for letter in hm.get_word():
 self.filled += letter if letter in hm.get_successful_letters() else "_"
 return self.filled

Then you can use a list comprehension:

def fill_blanks(self, hm):
 self.filled = "".join([x if x in hm.get_successful_letters() else "_" for x in hm.get_word()])
 return self.filled

Also, self.filled is only being used inside this method and is not referenced elsewhere, so it isn't necessary:

def fill_blanks(self, hm):
 return "".join([x if x in hm.get_successful_letters() else "_" for x in hm.get_word()])

Then you can remove self.filled = "" from the UI constructor.

While you're at it, self.word_list isn't used anywhere else either so can be a local variable or inlined.

Answer 2

Since my previous review suggested confining your HangMan class to a non-UI, data-oriented implementation (hence the new name, HangManData), I'll focus my comments on that.

Python classes don't require boilerplate getters/setters. For routine attribute getting and setting, you don't need to write methods. Instead of hm.get_word(), just use hm.word. Same thing applies to your get_successful_letters() and get_missed_letters() methods: they are not needed; just use ordinary attribute access. (The same thing can be said for UI.generate_word().)

Be careful when classes have correlated attributes. The HangManData class has attributes that are logically bound to each other. Only two of the attributes are fundamental: the word and the prior guesses. All other attributes (hits, misses, remaining guesses, etc) can be computed from the two fundamental pieces of information. As a programmer, you are constantly battling against the potential for future errors. One kind of error is when an object responsible for maintaining correlated information ends up in a logically contradictory state. The super-robust way to prevent that kind of error is to store only the fundamental data and simply compute all other pieces of information on demand. In the code example below, I follow that approach for several derived attributes (see all of the methods with the @property decorator). A less-robust, but sometimes more practical approach is to allow the storage of some correlated data but to do it in a way where the user of the class has clear guidance about how to use the class. In the example below, I take this approach by allowing the HangManData to store self._hits and self._misses as quasi-private lists rather than computing them on demand. A reader of the class can infer that they should be calling add_guess() rather than interacting with those two attributes directly.

Boundaries: don't let the responsibilities of a class creep outward. Your UI class knows too much about the implementation details of HangManData. Some examples: it knows to decrement remaining_chances when a guess misses; it computes of how many times a guess appears in the word; it figures out which letters in the word have already been guessed or not; and it figures out whether the puzzle has been solved. All of those behaviors are directly tied to the core data in HangManData and, as such, they seem like good candidates for read-only properties of that class. If you make such adjustments, your UI class will become much simpler. To give one example, here's the portion of the play() method where a letter guess is being handled. Notice the contrast: in the old code, UI is quite involved in the details of the secret word and the related computation (that is a UI creeping into the data layer); in the revised code, UI is just asking HangManData for basic facts and informing the user accordingly (that is a UI being a user interface).

# Old code: UI is mucking around in HangManData's turf.
elif guessed_letter in hm.get_word():
 number_of_appearances = hm.get_word().count(guessed_letter)
 if number_of_appearances == 1:
 print(f"{guessed_letter} Appears Once In The Word")
 else:
 print(f"{guessed_letter} Appears {number_of_appearances} Times In The Word")
 hm.set_successful_letters(guessed_letter)
else:
 hm.set_missed_letters(guessed_letter)
 hm.remaining_chances -= 1
# Revised code: UI is just being a user interface.
n_appearances = hm.add_guess(guessed_letter)
if n_appearances is None:
 print('You Already Guessed This Letter')
elif n_appearances:
 # Side note: most pluralization code can be finessed away by rephrasing.
 print(f'{guessed_letter} appearances: {n_appearances}')

Possible revisions to HangManData. As promised, here's some code to consider. As I'm posting the answer, I notice already that I've been inconsistent: if I bothered to cloak hits/misses by putting a leading underscore in their name and adding read-only properties, I probably should have done the same with guesses. Oh well, I'll let you decide how to handle it! But that's a good illustration of the pitfalls of managing correlated attributes. You are constantly on guard for pitfalls/inconsistencies and weighing tradeoffs between ease/practicality and robustness.

class HangManData:
 def __init__(self, word):
 self.word = word
 self.guesses = []
 self._hits = []
 self._misses = []
 def add_guess(self, letter):
 if letter in self.guesses:
 return None
 else:
 self.guesses.append(letter)
 n = self.word.count(letter)
 xs = self._hits if n > 0 else self._misses
 xs.append(letter)
 xs.sort()
 @property
 def hits(self):
 return self._hits
 @property
 def misses(self):
 return self._misses
 @property
 def remaining_chances(self):
 return 9 - len(self._misses)
 @property
 def is_dead(self):
 return self.remaining_chances <= 0
 @property
 def is_solved(self):
 return all(letter in self._hits for letter in self.word)
 @property
 def diagram(self):
 return hang_phases[self.remaining_chances]
 @property
 def display_word(self):
 return ' '.join(
 letter if letter in self._hits else '_'
 for letter in self.word
 )

Answer 3

This is the final code I'm content with. I'll definitely be doing more mini projects like this to improve my OOP skills.

I've added a is_invalid and appearances(returning a private variable) method in the HangManData class as well as making add_guess return false if the letter has already been guessed, and true if it hasnt allowing me to simplify the UI a little more. add_guess also defines _appearances because it was initially checking for the value anyways.

import random
from phases import hang_phases
class HangManData:
 def __init__(self, word):
 self.word = word
 self.guesses = []
 self._hits = []
 self._misses = []
 self._appearances = 0
 def add_guess(self, letter):
 if letter in self.guesses:
 return False
 else:
 self.guesses.append(letter)
 self._appearances = self.word.count(letter)
 xs = self._hits if self.appearances > 0 else self._misses
 xs.append(letter)
 xs.sort()
 return True
 @property
 def is_invalid(self):
 return self.word == ""
 @property
 def appearances(self):
 return self._appearances
 @property
 def hits(self):
 return self._hits
 @property
 def misses(self):
 return self._misses
 @property
 def remaining_chances(self):
 return 9 - len(self._misses)
 @property
 def is_dead(self):
 return self.remaining_chances <= 0
 @property
 def is_solved(self):
 return all(letter in self._hits for letter in self.word)
 @property
 def diagram(self):
 return hang_phases[self.remaining_chances]
 @property
 def display_word(self):
 return ' '.join(
 letter if letter in self._hits else '_'
 for letter in self.word
 )
class UI:
 def __init__(self):
 self.word_list = open('Words').read().split("\n")
 self.word = random.choice(self.word_list).upper()
 @staticmethod
 def display(hm):
 print(hm.word) # Left this here for testing purposes, prints word being looked for
 print(hm.diagram)
 print(hm.display_word)
 print(f"Incorrect Letters Guessed {hm.misses}")
 def generate_word(self):
 return self.word
 def play(self):
 hm = HangManData(self.generate_word())
 while True:
 self.display(hm)
 if hm.is_invalid:
 print("Invalid Word Was Generated")
 break
 elif hm.is_solved:
 print("Congrats!")
 break
 elif hm.is_dead:
 print(f"The Word Was {hm.word}")
 break
 else:
 guessed_letter = input("Guess A Letter: ").upper()
 if guessed_letter == hm.word:
 print("Congrats!")
 break
 else:
 check_added = hm.add_guess(guessed_letter)
 if guessed_letter == "QUIT":
 print(f"The Word Was {hm.word}")
 break
 elif not check_added:
 print("You Already Guessed This Letter")
 else:
 if hm.appearances == 1:
 print(f"{guessed_letter} Appears Once In The Word")
 else:
 print(f"{guessed_letter} Appears {hm.appearances} Times In The Word")
def main():
 play = True
 while play:
 u = UI()
 u.play()
 while True:
 print("Play Again?")
 play_again = input("Y/n > ").upper()
 if play_again == "Y":
 print("Setting Up...")
 break
 elif play_again == "N":
 print("Goodbye")
 play = False
 break
 else:
 print("Invalid Input")
if __name__ == "__main__":
 main()
```

• Richard Neumann: UI.play() and main() have nesting of doom.