As always, I am looking for as much criticism as possible. Forums and exchanges like this are where I have received my most solid feedback. Enjoy!
The code that I import is below:
from enum import Enum
from dataclasses import dataclass
import random
class Letters(Enum):
a = ('A', 'A')
b = ('B', 'B')
c = ('C', 'C')
d = ('D', 'D')
e = ('E', 'E')
f = ('F', 'F')
g = ('G', 'G')
h = ('H', 'H')
i = ('I', 'I')
j = ('J', 'J')
k = ('K', 'K')
l = ('L', 'L')
m = ('M', 'M')
n = ('N', 'N')
o = ('O', 'O')
p = ('P', 'P')
q = ('Q', 'Q')
r = ('R', 'R')
s = ('S', 'S')
t = ('T', 'T')
u = ('U', 'U')
v = ('V', 'V')
w = ('W', 'W')
x = ('X', 'X')
y = ('Y', 'Y')
z = ('Z', 'Z')
@property
def unused(self):
return self.value[1]
@property
def dead(self):
return self.value[0]
class Player:
def __init__(self):
self.name = self.get_name()
self.score = 0
def get_name(self):
while True:
user_name = input("Your Name: ")
confirmation = input(f"Your name is {user_name}? Y/N: ")
confirmation = confirmation.lower().strip()
if confirmation == "y":
return user_name
class GameWord:
def __init__(self):
self.word = self.populate()
###print(self.word)
def populate(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 print_word(self):
print(self.word)
class TheMan:
def __init__(self):
self.body_parts = ['O', '|', '/', '\\','/', '\\']
self.printed_body
'''What does the printed body look like?
12O35
/2|4\\
1/35円
'''
class UnusedLetters:
def __init__(self):
self.live_letters = []
self.initialize_live_letters()
self.dead_letters = []
self.board_letters = []
self.populate_board_letters()
def initialize_live_letters(self):
for letter in Letters:
self.live_letters.append(letter)
def live_to_dead(self, input_letter):
input_letter = input_letter.upper()
for letter in Letters:
if input_letter == letter.unused:
input_letter = letter
else:
pass
self.live_letters.remove(input_letter)
self.dead_letters.append(input_letter)
self.populate_board_letters()
def populate_board_letters(self):
if len(self.board_letters) > 0:
self.board_letters = []
for letter in Letters:
if letter in self.live_letters:
self.board_letters.append(letter.unused)
elif letter in self.dead_letters:
self.board_letters.append(letter.dead)
else:
print("ERROR: ONE LETTER DID NOT APPEND TO GAME BOARD")
def print_board_letters(self):
print(self.board_letters)
class Board:
def __init__(self, player, gameword, game_letters):
self.player = player
self.gameword = gameword
self.game_letters = game_letters
self.wrong_guesses = 0
self.display = ""
def display_game_word(self):
display_word = ''
for letter in self.gameword.word:
new_letter = self.convert_input(letter)
if new_letter in self.game_letters.dead_letters:
display_word += f"{letter} "
elif new_letter in self.game_letters.live_letters:
display_word += "_ "
else:
print("ERROR: That letter isnt found")
print(display_word)
self.display = display_word
return display_word
def convert_input(self, input_letter):
input_letter = input_letter.upper()
for letter in Letters:
if input_letter == letter.unused:
input_letter = letter
return input_letter
else:
pass
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()
return letter
def compare_letter(self, letter):
if self.convert_input(letter) in self.game_letters.live_letters:
if letter in self.gameword.word:
print("Found letter")
self.game_letters.live_to_dead(letter)
elif letter not in self.gameword.word:
#Hang the man
print("Letter not in Word")
self.game_letters.live_to_dead(letter)
self.wrong_guesses += 1
else:
print("Somehow Letter not Found Anywhere")
else:
print(f"{letter} was already Guessed. Please Pick a different Letter.")
The code that I use to run the file is below:
from hangman import GameWord, Player, UnusedLetters, Board
player1 = Player()
game_word = GameWord()
game_letters = UnusedLetters()
game_word_length = len(game_word.word)
game_board = Board(player1, game_word, game_letters)
while True:
game_board.game_letters.print_board_letters()
display_word = game_board.display_game_word()
if '_' not in display_word:
print("CONGRATULATIONS")
break
game_board.compare_letter(game_board.guess_letter())
if game_board.wrong_guesses >= 6:
print(f"MAN WAS HANGED!\nGAME OVER\nGame Word: {game_word.word}")
break
-
\$\begingroup\$ I don't understand this post. Is there something specific you're looking for here? \$\endgroup\$Chuck– Chuck2022年10月12日 18:09:52 +00:00Commented Oct 12, 2022 at 18:09
-
\$\begingroup\$ Efficiency, better use of classes and instances. A moderator removed my previous post because it had an issue, so I changed it and I guess the rule is you cannot post if your code has an issue. \$\endgroup\$Infinite Grasp– Infinite Grasp2022年10月12日 20:48:42 +00:00Commented Oct 12, 2022 at 20:48
2 Answers 2
Let's start with some trivial easy-to-fix things.
You have a lot of extra blank lines where they are not needed (PEP 8: E303). This decreases code clarity and can be easily fixed (Ctrl + Alt + L
to reformat file in PyCharm).
from dataclasses import dataclass
This statement is unused and therefore should be removed (Ctrl + Alt + O
to optimize imports in PyCharm).
Class TheMan
is unused. The purpose of the class, I suppose, is to print the hangman's body depending on the number of failed guesses. This can be done with a tuple containing strings for each of the hangman's states. Index in the tuple corresponds to the number of failed attempts, e.g.:
hangman_drawings = (
" \n" +
" \n" +
" O \n" +
" \n" +
" \n" +
" П ",
" \n" +
" \n" +
" O \n" +
" | \n" +
" \n" +
" П ",
" \n" +
" \n" +
" O \n" +
" | \n" +
" | \n" +
" П ",
" \n" +
" \n"
" O/\n" +
" | \n" +
" | \n" +
" П ",
" \n" +
" \n" +
" O/\n" +
" | \n" +
" /| \n" +
" П ",
" \n" +
" \n" +
" \\O/\n" +
" | \n" +
" /| \n" +
" П ",
" \n" +
" | \n" +
" \\O/\n" +
" | \n" +
" /| \n" +
" П ",
". \n" +
"| | \n" +
"| \\O/\n" +
"| | \n" +
"| /| \n" +
"| П ",
".____\n" +
"| | \n" +
"| \\O/\n" +
"| | \n" +
"| /| \n" +
"| П ",
".____\n" +
"| | \n" +
"| \\O/\n" +
"| | \n" +
"| / \\\n" +
"| "
)
This one contains 10 different states. You can get a drawing for the current state like so:
def get_hangman_drawing(total_attempts: int, failed_attempts: int) -> str:
if total_attempts > len(hangman_drawings):
raise IndexError('There are not enough hangman drawings to support this difficulty!')
return hangman_drawings[len(hangman_drawings) - total_attempts + failed_attempts - 1]
This works for an arbitrary difficulty (total number of guesses) as you can see from the declaration.
The entire purpose of the GameWord
class is to provide a word. You don't need a class for that:
game_word = get_game_word()
...
def get_game_word() -> str:
with open('words.txt') as file:
return random.choice(f.read().splitlines())
The name of the class UnusedLetters
doesn't correspond to the data the class encapsulates.
Game logic is hard to follow.
input_letter = input_letter.upper()
for letter in Letters:
if input_letter == letter.unused:
input_letter = letter
In this piece of code we reassign input_letter
(str) with an instance of enum Letters
.
Don't abuse dynamic typing like that.
After you change the type of the object the name is pointing to, its behaviour changes. Just a few lines later (after an else
statement that does nothing and can be removed) we do self.live_letters.remove(input_letter)
. live_letters
is a list of Letters
, we don't want any strings in there, while your code allows this to happen if none of the if checks in the for loop succeed.
In a neighbouring function you noticed a similar problem and blocked it:
else:
print("Somehow Letter not Found Anywhere")
That's much better, but raising an exception in such case is more fitting.
live_letters
and dead_letters
add up to all possible letters so you only need one of those lists. It can also be a set. Use sets when you don't care about order (those lists are only used to add something to them and check if something is in them).
Some methods have nothing to do with the classes they are in. It's easy to see by looking at the name of the method and the name of the class. E.g. class Board
and def convert_input
.
Some of those methods don't even use the class instance they are bound to, e.g. GameWord.populate
. You can see that keyword self
is not used inside the method: the function doesn't really care what the object is and can exist on its own.
Class Player
doesn't have any functionality (its only method doesn't depend on the Player instance as mentioned in the previous paragraph) and therefore can be omitted; its fields are never used anywhere in the game.
As for the rest of the code, make it so the Board
class (can be renamed to UI
) is only responsible for printing things to the console (single responsibility principle) and move the game to its own class (e.g. Game
). There is some game logic in the execute.py
file for some reason, it can be moved over to the new Game
class.
Create a separate file for each class. This helps with readability and avoids dumping all the imports in one place.
Run the program via if __name__ == '__main__':
construction.
Your execute.py
file will look like this:
from Game import Game
if __name__ == '__main__':
Game().run()
I think that's about it, feel free to ask any questions and good luck!
-
\$\begingroup\$ I have two questions: 1) I noticed that it seemed you were typing your inputs for methods with -->, is this correct? Is this best practice in all scenarios, or specific and why? I ask because the literature I was reading about object oriented programming mentioned assuming type. 2) I don't question the legitimacy of not abusing dynamic typing, but I am curious as to what types of errors this can begin to likely cause. This detail helps very much \$\endgroup\$Infinite Grasp– Infinite Grasp2022年10月14日 01:48:05 +00:00Commented Oct 14, 2022 at 1:48
-
\$\begingroup\$ @InfiniteGrasp , those are type annotations. They will save you from some of the run-time errors (you won't know something is wrong until you run your code and even then you may not encounter the bug) caused by dynamic typing by turning them into compile-time errors (the program won't even launch. Compile-type errors are much much easier to debug. The choice, however, is yours. I suggest this article on dynamic typing in Python and this one on its advantages and drawbacks. \$\endgroup\$QuasiStellar– QuasiStellar2022年10月14日 10:48:21 +00:00Commented Oct 14, 2022 at 10:48
-
\$\begingroup\$ @InfiniteGrasp as for the possible errors, check the paragraph in my answer that starts with "After you change the type of the object the name is pointing to, its behaviour changes." \$\endgroup\$QuasiStellar– QuasiStellar2022年10月14日 10:54:11 +00:00Commented Oct 14, 2022 at 10:54
In addition to Pierre's answer, you can make your life easier using a named tuple to represent the "dead" and "unused" letter pairs:
...
from typing import NamedTuple
class LetterPair(NamedTuple):
"""Pair of dead and unused representation of a letter."""
dead: str
unused: str
You can then derive the Enum
from that type:
...
class Letters(LetterPair, Enum):
"""Available letters."""
a = LetterPair('A', 'A')
b = LetterPair('B', 'B')
c = LetterPair('C', 'C')
...
Then you don't need the unused
and dead
properties on the Enum any longer.
However, it may even be simpler:
>>> ord('A') - ord('A')
127215
>>>
The character offset for each letter is constantly 127215
, so we can programatically derive the "dead" glyph using a function:
from string import ascii_uppercase
def to_dead(letter: str) -> str:
"""Return the dead letter representation."""
if len(letter) != 1:
raise ValueError('Need a single letter.')
if letter not in ascii_uppercase:
raise ValueError('Letter must be ASCII upper case.')
return chr(ord(letter) + 127215)
This way, you don't need an Enum at all, but can use ascii_uppercase
to check for the letters and use to_dead()
to get the used glyph representation of that letter.
-
\$\begingroup\$ Whew, So What would be the practice, to use an enum and NamedTuple, to exclude the enum completely and do as you explained? or does it not matter? If the circumstances matter, I am more involved in the direction of software development. Best Practices are key to me currently \$\endgroup\$Infinite Grasp– Infinite Grasp2022年10月14日 01:55:34 +00:00Commented Oct 14, 2022 at 1:55
-
\$\begingroup\$ Also, if you are familiar, how does NamedTuple in typing differ from nametuple in Collections? \$\endgroup\$Infinite Grasp– Infinite Grasp2022年10月14日 02:00:36 +00:00Commented Oct 14, 2022 at 2:00
-
1\$\begingroup\$ @InfiniteGrasp documentation explains it in details. \$\endgroup\$QuasiStellar– QuasiStellar2022年10月14日 10:59:11 +00:00Commented Oct 14, 2022 at 10:59
Explore related questions
See similar questions with these tags.