1
\$\begingroup\$

I am looking for as much criticism as possible. Specifically I am trying to institute BEST PRACTICES for OOP and game design. Enjoy!

Github hangman repository

Each Class is in a separate file and imported.

BASIC LETTERS CLASS

from enum import Enum class Letters(Enum): a = ('πŸ„°', 'A') b = ('πŸ„±', 'B') c = ('πŸ„²', 'C') d = ('πŸ„³', 'D') e = ('πŸ„΄', 'E') f = ('πŸ„΅', 'F') g = ('πŸ„Ά', 'G') h = ('πŸ„·', 'H') i = ('πŸ„Έ', 'I') j = ('πŸ„Ή', 'J') k = ('πŸ„Ί', 'K') l = ('πŸ„»', 'L') m = ('πŸ„Ό', 'M') n = ('πŸ„½', 'N') o = ('πŸ„Ύ', 'O') p = ('πŸ„Ώ', 'P') q = ('πŸ…€', 'Q') r = ('πŸ…', 'R') s = ('πŸ…‚', 'S') t = ('πŸ…ƒ', 'T') u = ('πŸ…„', 'U') v = ('πŸ……', 'V') w = ('πŸ…†', 'W') x = ('πŸ…‡', 'X') y = ('πŸ…ˆ', 'Y') z = ('πŸ…‰', 'Z') @property def unused(self): return self.value[1] @property def dead(self): return self.value[0] 

GAME LETTERS CLASS

from Letters import Letters class GameLetters(): def __init__(self): pass def populate_board_letters(self, guessed_letters): for letter in Letters: if letter.unused.lower() in guessed_letters: self.board_letters.append(letter.dead) else: self.board_letters.append(letter.unused) return self.board_letters 

UI Class

from GameLetters import GameLetters import random class UI(GameLetters): def __init__(self): self.game_word = self.initialize_game_word() self.current_letter = '' self.board_letters = [] self.guessed_letters = [] self.wrong_guesses = 0 self.hangman_drawings = [(" _____\n| |\n|\n|\n|\n|\n__"),(" _____\n| |\n| O\n|\n|\n|\n__"),(" _____\n| |\n| O\n| |\n|\n|\n__"), (" _____\n| |\n| O\n| /|\n|\n|\n__"),(" _____\n| |\n| O\n| /|\\\n|\n|\n__"),(" _____\n| |\n| O\n| /|\\\n| /\n|\n__"),(" _____\n| |\n| O\n| /|\\\n| / \\\n|\n__")] def get_hangman_drawing(self, failed_attempts): return self.hangman_drawings[failed_attempts] def initialize_game_word(self): game_words = [] with open('words.txt') as file: for line in file: game_words.append(line.rstrip('\r\n')) word = random.choice(game_words) return word def display_game_word(self): display_word = '' for letter in self.game_word: if str(letter) in self.guessed_letters: display_word += f"{letter} " elif str(letter).upper() in self.board_letters: display_word += "_ " else: print("ERROR: That letter isnt found") return display_word def guess_letter(self): while True: letter = input("GUESS: ") if len(letter) == 1: break print("Please Only Choose One Letter at a Time") letter = letter.strip().lower() self.guessed_letters.append(letter) self.current_letter = (letter) def compare_letter(self): if self.current_letter in self.game_word: print("This Letter was found") if "_" in self.display_game_word(): return True else: print("You Won") return False else: print("This letter was most definitely not in the word") self.wrong_guesses += 1 return True def display_game_state(self): self.board_letters.clear() self.board_letters = self.populate_board_letters(self.guessed_letters) print(f"\n\n\n{self.get_hangman_drawing(self.wrong_guesses)}\n{self.populate_board_letters(self.guessed_letters)}\n{self.display_game_word()}") 

Game LOGIC

from UI import UI class Game: def run(self): self.game = UI() keep_playing = True while keep_playing: self.game.display_game_state() self.game.guess_letter() keep_playing = self.game.compare_letter() if self.game.wrong_guesses >= len(self.game.hangman_drawings): print("Your man has Hanged. Please Try again!") break 

Execute File

from Game import Game if __name__ == '__main__': Game().run() ```
\$\endgroup\$

    2 Answers 2

    1
    \$\begingroup\$

    The entry point ("execute file") as well as the main game loop both look OK. Your Letters class doesn't make any sense to me. That's, by the way, also not how enums usually are used. And I don't understand why you have GameLetters; you're basically only using it to store a method in a different place.

    The overall structure of UI is pretty clear. But to call it UI is maybe a bit misleading, since it basically is the entire game.

    There's a bunch of smaller improvements you can do here and there. For reading in the list of possible words, you could just do something like

    def initialize_game_word(self) -> str: with open('words.txt') as file: game_words = file.readlines() return random.choice(game_words) 

    In UI.display_game_word() you don't have to convert to str since the letters already are strings, and you don't have to use str.strip() in UI.guess_letter() since you've already asserted that the letter is a single character. But you probably want to make sure that the letter is validated to be a character between a and z - if someone entered a space you would be trying to add an empty string as a guess (since you've stripped the space).

    \$\endgroup\$
    2
    • \$\begingroup\$Super disappointed in not realizing I didnt need to convert to a string, because it is already a string and not an object. Also, I havent really used ENUMS and I am trying to get comfortable with how they can be used. Do you have any suggestions for projects that may implement ENUMS in a manner more standard?\$\endgroup\$CommentedOct 17, 2022 at 1:31
    • \$\begingroup\$I don't know how useful it would be to point at a specific project and its usage - I'd suggest that you look into a tutorial or two where they use enums instead.\$\endgroup\$
      – ades
      CommentedOct 17, 2022 at 6:14
    1
    \$\begingroup\$

    A note on object oriented programs like this. Usually, defining a class with two methods, one of which being __init__, means that whatever function is in the class could probably just be standalone.

    For instance, Game is not tracking any sort of state. It seems to be a class just for its own sake. If we remove the class:

    def game(): ui = UI() keep_playing = True while keep_playing: ui.display_game_state() ui.guess_letter() keep_playing = ui.compare_letter() if ui.wrong_guesses >= len(ui.hangman_drawings): print("Your man has Hanged. Please Try again!") break 

    There is zero loss in functionality.

    Another example is in GameLetters:

    from Letters import Letters class GameLetters(): def __init__(self): pass def populate_board_letters(self, guessed_letters): for letter in Letters: if letter.unused.lower() in guessed_letters: self.board_letters.append(letter.dead) else: self.board_letters.append(letter.unused) return self.board_letters 

    Again, the __init__ doesn't need to be there, since you pass. Reading the functions with that lacking __init__ means that you have to know that this class is to be inherited for self.board_letters to make sense. This sends a signal to me that really populate_board_letters should be in the UI class:

    # drop the inheritance class UI: ~snip~ def populate_board_letters(self, guessed_letters): for letter in Letters: if letter.unused.lower() in guessed_letters: self.board_letters.append(letter.dead) else: self.board_letters.append(letter.unused) return self.board_letters 

    Now the attributes are easy to trace and don't "magically" appear in the instance.

    Hangman Drawings

    This attribute is invariant and could probably just be a class variable:

    class UI: hangman_drawings = [ (" _____\n| |\n|\n|\n|\n|\n__"), (" _____\n| |\n| O\n|\n|\n|\n__"), (" _____\n| |\n| O\n| |\n|\n|\n__"), (" _____\n| |\n| O\n| /|\n|\n|\n__"), (" _____\n| |\n| O\n| /|\\\n|\n|\n__"), (" _____\n| |\n| O\n| /|\\\n| /\n|\n__"), (" _____\n| |\n| O\n| /|\\\n| / \\\n|\n__") ] 

    I've also broken up each drawing into its own line for easier reading, which declutters the program a bit.

    Initialized Word

    This doesn't belong in the UI class. At the very least, it should be a @staticmethod since it doesn't use self:

    class UI: ~snip~ @staticmethod def initialize_game_word(): with open('words.txt') as file: # just collect the lines into a list words = file.readlines() # choose the word and strip off whitespace word = random.choice(words).strip() return word 

    One other thing I might add is the capability to pass in a different file:

     @staticmethod def initialize_game_word(file='words.txt'): with open(file) as fh: # just collect the lines into a list words = fh.readlines() # choose the word, strip off whitespace, and set to uppercase word = random.choice(words).strip().upper() return word 

    Checking guessed letters

    There are a few things going on with checking guessed letter, tracking the last guessed letter, and printing the letters that match the game word. To rework this, I'd track display_word in a list with a spot that represents a letter in the game word:

    class UI: def __init__(self): ~snip~ self.display_word = ['_' for _ in self.game_word] 

    The guess_letter function can then be modified accordingly:

    class UI: ~snip~ def guess_letter(self): while True: letter = input("Guess a letter: ").strip().upper() if len(letter) != 1: print("One guess at a time, please") elif letter in self.guessed_letters: print("You've already guessed that letter!") print(f"Try one not in {self.guessed_letters}") else: break self.guessed_letters.add(letter) if letter not in self.game_word: print("That letter is not in the word") self.wrong_guesses += 1 return print("That letter was found! Good guess!") for i, char in enumerate(self.game_word): if letter == char: self.display_word[i] = letter # and to refactor the display function def display_game_word(self): # much easier print(self.display_word) 

    Note how this rids us of the need to have a compare_letter function, which really is a shorthand of letter in game_word at its core. I've also forced letter to be uppercase, since that's how your display letters are formatted.

    Since display_game_word doesn't do much at this point, we can get rid of that function entirely.

    Showing used letters

    Because the only use for Letters is to iterate over it and pick either unused or dead, I'd say an Enum is not the right choice for a data structure. I'd lean towards a list here. A trick to build it quickly is that there is a constant distance between your uppercase and blocked letters:

    ord('πŸ„°') - ord('A') 127215 

    We can use this to really easily build this list:

    from string import ascii_uppercase as uppercase dist = ord('πŸ„°') - ord('A') LETTERS = [(letter, chr(ord(letter) + dist)) for letter in uppercase] LETTERS[25] ('Z', 'πŸ…‰') 

    And now, to use your function:

    class UI: ~snip~ def populate_board_letters(self, guessed_letters): for letter, dead in LETTERS: if letter in guessed_letters: self.board_letters.append(dead) else: self.board_letters.append(letter) return self.board_letters 

    Looking closer, though, I don't really love that self.board_letters gets cleared. Why not just rebuild it when you call it? Set it as a @property:

    class UI: ~snip~ @property def board_letters(self): return [ dead if letter in self.guessed_letters else letter for letter, dead in LETTERS ] 

    Now, this highlights that display_game_state would be most convenient to just print(UI). So let's use the __str__ method:

    class UI: ~snip~ def __str__(self): return ( f"\n\n\n{self.hangman_drawings[self.wrong_guesses]}" f"\n{self.board_letters}" f"\n{self.display_word}" ) # use like this ui = UI() print(ui) 

    Notice that now we are directly indexing hangman_drawings, referencing board_letters, and also referencing display_word. We don't need to hide these behind other functions anymore.

    Combining Changes

    Now, since we've cut down on how many classes are in this program, I think almost all of it, if not all of it, can effectively be in one file. Here's what we have:

    from string import ascii_uppercase as uppercase import random dist = ord('πŸ„°') - ord('A') LETTERS = [(letter, chr(ord(letter) + dist)) for letter in uppercase] class UI: hangman_drawings = [ (" _____\n| |\n|\n|\n|\n|\n__"), (" _____\n| |\n| O\n|\n|\n|\n__"), (" _____\n| |\n| O\n| |\n|\n|\n__"), (" _____\n| |\n| O\n| /|\n|\n|\n__"), (" _____\n| |\n| O\n| /|\\\n|\n|\n__"), (" _____\n| |\n| O\n| /|\\\n| /\n|\n__"), (" _____\n| |\n| O\n| /|\\\n| / \\\n|\n__") ] def __init__(self, words_file='words.txt'): self.game_word = self.initialize_game_word(words_file) self.display_word = ['_' for _ in self.game_word] self.guessed_letters = set() self.wrong_guesses = 0 def __str__(self): return ( f"\n\n\n{self.hangman_drawings[self.wrong_guesses]}" f"\n{self.board_letters}" f"\n{self.display_word}" ) @property def board_letters(self): return [ dead if letter in self.guessed_letters else letter for letter, dead in LETTERS ] @staticmethod def initialize_game_word(file='words.txt'): with open(file) as fh: # just collect the lines into a list words = fh.readlines() # choose the word, strip off whitespace, and set to uppercase word = random.choice(words).strip().upper() return word def guess_letter(self): while True: letter = input("Guess a letter: ").strip().upper() if len(letter) != 1: print("One guess at a time, please") elif letter in self.guessed_letters: print("You've already guessed that letter!") print(f"Try one not in {self.guessed_letters}") else: break self.guessed_letters.add(letter) if letter not in self.game_word: print("That letter is not in the word") self.wrong_guesses += 1 return print("That letter was found! Good guess!") for i, char in enumerate(self.game_word): if letter == char: self.display_word[i] = letter def game(): ui = UI() while True: print(ui) ui.guess_letter() if ui.wrong_guesses >= len(ui.hangman_drawings): print("Your man has Hanged. Please Try again!") break elif ui.display_word == list(ui.game_word): print("You won!") break 

    A few last minute tweaks

    I'd rename the game function to main, the UI class should be called Game. I'd also add in a lost function to check if the player has hit the max number of guesses and a won function to see if the player guessed the word:

    class Game: ~snip~ def lost(self): return self.wrong_guesses >= len(self.hangman_drawings) def won(self): # all letters are filled in properly return self.display_word == list(self.game_word) def main(): game = Game() while not (game.won() or game.lost()): # print the game board directly print(game) game.guess_letter() if game.won(): print("You guessed the word!") else: print("Your man was hanged! Better luck next time!") 
    \$\endgroup\$

      Start asking to get answers

      Find the answer to your question by asking.

      Ask question

      Explore related questions

      See similar questions with these tags.