I just joined here. I have been doing python for a couple of weeks and I would like for some experienced programmers to review my code. I made this for a school project and I am proud of it. The code below works well but I would like to know what things I can improve on. Thank you for taking the time to review this.
main.py
main_grid = [['+' for col in range(3)] for row in range(3)]
player_one = {'key': 'P1', 'name': None, 'symbol': 'X', 'pos': []}
player_two = {'key': 'P2', 'name': None, 'symbol': 'O', 'pos': []}
player_list = [player_one, player_two]
def title_screen():
print(' '*15, ' _____ _____ _____')
print(' '*15, '|__ __|__ __| ____|')
print(' '*15, ' | | |_| |_| |___')
print(' '*15, ' _|_|_|_____|_____|')
print(' '*15, '|__ __| ___ | ____|')
print(' '*15, ' | | | | | | |___ ')
print(' '*15, '__|_|_|_|_|_|_____|')
print(' '*15, '|__ __| ___ | ____|')
print(' '*15, ' | | | |_| | ____|')
print(' '*15, ' |_| |_____|_____|')
def info_screen():
print('-'*51)
print('2021 L.A | A python school project. ')
print()
print("This game is a python version of 'TIC TAC TOE'.")
print('I hope you enjoy! :]')
print()
def intro_screen():
print('RULES:')
print('Rows and columns can only have inputs from [0-2].')
print('Players can only enter value once unless value is')
print('not valid.')
print("Players can't put their symbol in already occupied")
print('cells.')
print('Players win if their symbol is placed 3 in a row')
print('horizontally, vertically or diagonally. Anything')
print('else does not count as a win.')
print('If the grid is filled, the game results as a tie.')
print()
def start_screen():
title_screen()
info_screen()
intro_screen()
def get_name():
for player_type in player_list:
print(f"Enter [{player_type['key']}] name:")
player_type['name'] = input(f"[{player_type['key']}] > ")
print()
def get_input(player_type, desc):
print(desc)
user_input = input(f"[{player_type['key']}] {player_type['name']} > ")
print()
return user_input
def get_point(player_type, desc):
"""Makes user get a point value which could be a row or col."""
point = -1
while point not in range(3):
try:
point = int(get_input(player_type, desc))
except ValueError:
print('The entered value must be an integer.')
print('Please try again.')
print()
continue
if point in range(3):
return point
elif point not in range(3):
print('The entered value must be in range between 0-2.')
print('Please try again.')
print()
continue
else:
print('Unknown error')
print('Please try again.')
print()
continue
def check_pos(pos):
"""Check whether the pos value is in a player's coords list."""
if pos in player_one['pos'] or pos in player_two['pos']:
print('The entered coordinate is occupied by a player.')
print('Please try again.')
print()
return False
else:
return True
def update_grid(pos):
"""Updates grid using player coords."""
for player_type in player_list:
for prev_pos in player_type['pos']:
main_grid[prev_pos[0]][prev_pos[1]] = player_type['symbol']
def print_stats(player_type, turn_count):
"""Prints current game stats."""
print('-'*51)
print(f"CURRENT TURNS | {turn_count}/9")
print(f"CURRENT PLAYER | {player_type['key']}")
print(f"CURRENT SYMBOL | {player_type['symbol']}")
def print_grid():
"""Prints the grid"""
print()
for row in range(3):
print(' '*21, end='')
for col in range(3):
print(main_grid[row][col], end=' ')
print()
print()
def check_grid(player_type, pos):
"""End game when symbol is three in a row or grid is filled. Continues if not three in a row or not filled."""
row = pos[0]
col = pos[1]
symbol = player_type['symbol']
win_msg = f"[{player_type['key']}] {player_type['name']} has won the game."
tie_msg = 'The game has resulted a tie.'
if symbol == main_grid[row][0] == main_grid[row][1] == main_grid[row][2]:
print_result(win_msg)
return True
elif symbol == main_grid[0][col] == main_grid[1][col] == main_grid[2][col]:
print_result(win_msg)
return True
elif row == col and symbol == main_grid[0][0] == main_grid[1][1] == main_grid[2][2]:
print_result(win_msg)
return True
elif row + col == 2 and symbol == main_grid[0][2] == main_grid[1][1] == main_grid[2][0]:
print_result(win_msg)
return True
elif '+' not in main_grid[0] and '+' not in main_grid[1] and '+' not in main_grid[2]:
print_result(tie_msg)
return True
else:
return False
def print_result(msg):
"""Prints end result of game"""
print('-'*51)
print('RESULT:')
print_grid()
print(msg)
exit_result()
def setup_pos(player_type):
"""Sets up position value
Gets each point value.
Checks each point value
Checks position value
Adds position value to player dictionary
"""
valid_pos = False
while valid_pos is False:
row = get_point(player_type, 'Select a row:')
col = get_point(player_type, 'Select a column:')
new_pos = (row, col)
valid_pos = check_pos(new_pos)
if valid_pos is True:
player_type['pos'].append(new_pos)
return new_pos
def exit_result():
"""Choice to exit result screen."""
print()
print(f"Press any key to exit.")
user_input = input('> ')
exit(0)
def setup_game():
turn_count = 1
game_finish = False
while game_finish is False:
for player_type in player_list:
print_stats(player_type, turn_count)
print_grid()
new_pos = setup_pos(player_type)
update_grid(new_pos)
game_finish = check_grid(player_type, new_pos)
turn_count += 1
if game_finish is True: # If player won, break for loop on player's turn.
break
def main():
start_screen()
get_name()
setup_game()
if __name__ == '__main__':
main()
-
2\$\begingroup\$ This is really good for someone just starting out! Keep it up! \$\endgroup\$N3buchadnezzar– N3buchadnezzar2021年11月21日 09:56:47 +00:00Commented Nov 21, 2021 at 9:56
2 Answers 2
So this is a great start for someone just starting out. I will go off on a huge tangent here, but hopefully you can take something away from how I approached improving a very minor part of your code.
Player states
class Player:
id: int
name: str = ""
symbol: str = ""
is_winner: bool = False
Now we can build the list of players as follows
player_list = [Player(1, symbol="X"), Player(2, symbol="O")]
Where did pos
go? It would be better (and clearer) if the gamestate
was handled globally and not as a property of the players. It makes sense that the players are part of the game, not that the game is part of the players.
There is no reason why your players could not be implemented as classes
Printing
The line print("-"*51)
can be improved in a few ways. 51
(and in general all non specified numbers in code) are known colloquially as
magic-numbers
and is an anti-pattern. Where does the number come from?
Why is it not 52
or 50
? We can fix this by defining it as a constant
BLANKLINE = "-" * 51
As a sidenote notice that we wrote *
and not *
, this is because
Python has style guide that specifies that operators need a little breathing roomThis is a lie . If you use Black or another formater it fixes these things for you.
info_screen
can now look like
def info_screen() -> str:
"""Information about the game"""
return f"""{BLANKLINE}
2021 L.A | A python school project. "
This game is a python version of 'TIC TAC TOE'.
"I hope you enjoy! :]"
"""
Where we have added a very short docstring and typing hints.
Perhaps more importantly we have split generating the information
and printing the information. While now there would be nothing wrong
with printing in the info_screen
file, what if we wanted to append text to it later?
Overall principles
- Implement your game using classes and objects it will make it easier to handle the game logic.
- Consider (just for fun) implementing a bitboard to handle the game state, or again take an object oriented approach.
- Separate printing from creation (Programmers tend to refer to this as seperate UI (user interface) from business logic)
- Get rid of your magic-numbers (I see a floating 15, 21 and a few others)
- Consider using the walrus operator
Turing this
def setup_pos(player_type):
valid_pos = False
while valid_pos is False:
row = get_point(player_type, "Select a row:")
col = get_point(player_type, "Select a column:")
new_pos = (row, col)
valid_pos = check_pos(new_pos)
if valid_pos is True:
player_type["pos"].append(new_pos)
return new_pos
into this
def get_pos(player_type):
row = get_point(player_type, "Select a row:")
col = get_point(player_type, "Select a column:")
return (row, col)
def setup_pos(player_type):
while not valid_pos(pos := get_pos(player_type)):
pass
player_type["pos"].append(pos)
return pos
Where check_pos
was renamed valid_pos
.
- The
player_type["pos"].append(pos)
in the code above is another antipattern. Notice howplayer_type
is a global function, we try to avoid those as best as we can. Limiting the scope of functions and variables makes it much easier to squash bugs! Again the proper solution is classes =)
Overall principles
I will leave the bullet points above for you to implement and adapt. Instead I will focus on how I would "improve" (totally over-engineer).
Notice how I would never do the improvements I do here in production
If you can (and you should) find libraries that can handle the pretty printing to the command line. However, just for learning. Let us see how we can implement something simple
def title_screen():
print(' '*15, ' _____ _____ _____')
print(' '*15, '|__ __|__ __| ____|')
print(' '*15, ' | | |_| |_| |___')
print(' '*15, ' _|_|_|_____|_____|')
print(' '*15, '|__ __| ___ | ____|')
print(' '*15, ' | | | | | | |___ ')
print(' '*15, '__|_|_|_|_|_|_____|')
print(' '*15, '|__ __| ___ | ____|')
print(' '*15, ' | | | |_| | ____|')
print(' '*15, ' |_| |_____|_____|')
- There is repetition as aforementioned.
- While subjective I find the particular ASCII font hard to read. Particularly an issue is that the lines are not connected.
An improvement would be something like this
So how do we get there?
We split everything to do with generating a fancy title into it's own file. At the end of the day we will use it as follows
from title import Title, asciify, fullborder TIC_TAC_TITLE = Title(asciify("TIC TAC "TOE"), border=fullborder, width=79) def title_screen(): print(TIC_TAC_TITLE)
Which is your main
and with much imagination we named the new file "title.py".
It is getting quite late but I will just leave the code here with a few highlights
We split the generation of the fancy title into three parts
- We need to generate the ascii letters
- We need to make a border
- We need to combine the letters and the borders into a title
The first problem is solved in the
asciify
file. Using a dictionary we have defined a few handful of letters.The title and border can both naturally be expressed as a classes
We add typing hints, docstrings and doctests wherever necessary.
Note that for improving the rest of the code you would do exactly the same. You would try to split the code into separate parts, each part would go into it's own file. Here you would add classes as needed, add typing hints, doctests and then import it into your main file.
Code
"""Takes in a list of letters [A,C,E,O,T,I, ] and returns them as ascii letters
"""
from dataclasses import dataclass
from typing import Annotated
ASCII = {
"A": """
┌─────┐
│ ┌─┐ │
│ ├─┤ │
└─┘ └─┘
""",
"C": """
┌─────┐
│ ┌───┘
│ └───┐
└─────┘
""",
"E": """
┌─────┐
│ ───┤
│ ───┤
└─────┘
""",
"O": """
┌─────┐
│ ┌─┐ │
│ └─┘ │
└─────┘
""",
"T": """
┌─────┐
└─┐ ┌─┘
│ │
└─┘
""",
"I": """
┌─────┐
└─┐ ┌─┘
┌─┘ └─┐
└─────┘
""",
" ": """
""",
"-": """
┌───┐
└───┘
""",
}
PYTHON_MAX_CHAR_WIDTH = 79
AsciiLetters = Annotated[str, "A multiline string of big ascii letters"]
Length = Annotated[int, "How long a particular string should be"]
def asciify(letters: str, ascii_dict: dict = ASCII) -> AsciiLetters:
"""Turns a string into big ascii letters
Args:
letters: Optional. The word / collection of letters you want to asciify
ascii_dict: A dictionary (mapping) between letters and their ascii representation
Returns:
The inputed letters in asccii form formated as a multiline string
Example:
>>> print(asciify('A'))
┌─────┐
│ ┌─┐ │
│ ├─┤ │
└─┘ └─┘
>>> print(asciify('TAC'))
┌─────┐┌─────┐┌─────┐
└─┐ ┌─┘│ ┌─┐ ││ ┌───┘
│ │ │ ├─┤ ││ └───┐
└─┘ └─┘ └─┘└─────┘
>>> print(asciify('I-E-A-I-A-I-O'))
┌─────┐ ┌─────┐ ┌─────┐ ┌─────┐ ┌─────┐ ┌─────┐ ┌─────┐
└─┐ ┌─┘ ┌───┐ │ ───┤ ┌───┐ │ ┌─┐ │ ┌───┐ └─┐ ┌─┘ ┌───┐ │ ┌─┐ │ ┌───┐ └─┐ ┌─┘ ┌───┐ │ ┌─┐ │
┌─┘ └─┐ └───┘ │ ───┤ └───┘ │ ├─┤ │ └───┘ ┌─┘ └─┐ └───┘ │ ├─┤ │ └───┘ ┌─┘ └─┐ └───┘ │ └─┘ │
└─────┘ └─────┘ └─┘ └─┘ └─────┘ └─┘ └─┘ └─────┘ └─────┘
"""
ascii_letter_list = [ascii_dict[l] for l in list(letters)]
lines = [letter.split("\n") for letter in ascii_letter_list]
letter_widths = [max(map(len, line)) for line in lines]
ascii_art = []
for letterlines in zip(*lines):
line = ""
for i, letter in enumerate(letterlines):
line = line + f"{letter:{letter_widths[i]}}"
ascii_art.append(line)
ascii_letters = "\n".join(ascii_art)
return ascii_letters.strip()
def repeat_to_length(string: str, wanted: Length) -> str:
"""Repeats a string to the desired length"""
a, b = divmod(wanted, len(string))
return string * a + string[:b]
@dataclass
class Border:
SW: str = ""
SE: str = ""
NE: str = ""
NW: str = ""
top: str = ""
bottom: str = ""
right: str = ""
left: str = ""
symmetric: bool = False
def __post_init__(self):
if not self.symmetric:
return
self.top, self.bottom = self.equal_values([self.top, self.bottom])
self.left, self.right = self.equal_values([self.left, self.right])
self.SW, self.SE, self.NE, self.NW = self.equal_values(
[self.SW, self.SE, self.NE, self.NW]
)
def border_top(self, width):
return self.horizontal(self.top, self.NW, self.NE, width)
def border_bottom(self, width):
return self.horizontal(self.bottom, self.SW, self.SE, width)
@staticmethod
def horizontal(symbol, left_corner, right_corner, width):
if not symbol:
return ""
if width == 1:
return left_corner
main = repeat_to_length(symbol, width)
return left_corner + main[:-2] + right_corner if len(main) >= 2 else main
@staticmethod
def equal_values(values):
total_values = len(values)
for value in values:
if value is None:
continue
return [value for _ in range(total_values)]
return [None for _ in range(total_values)]
@dataclass
class Title:
text: str = ""
border: Border = Border()
center: bool = True
width: int = 0
def __post_init__(self):
left, center = "<", "^"
self.alignment = center if self.center else left
def __str__(self):
title = self.center_text() if self.center else self.text
if self.border == Border():
return title.rstrip()
return self.add_border(text=title.rstrip())
def center_text(self, width=None):
width = width if width is not None else self.width
textlines = self.text.split("\n")
textwidth = (
max(len(l) for l in textlines)
+ len(self.border.left)
+ len(self.border.right)
)
width = textwidth if not width else width
lines = [f"{line:^{width}}" for line in textlines]
minimum_indent = " " * min(map(lambda l: len(l) - len(l.lstrip()), lines))
return "\n".join(minimum_indent + line for line in textlines)
def add_border(self, text, width=None):
"""
Adds a border to the self.text property of the Title class
Args:
width: Optional. If set the top and bottom border keeps this width.
Returns:
A title formated as a multiline string
Example:
>>> print(Title("A", border=Border(), center=True, width=3))
A
>>> print(Title("A", border=Border(top="=", SW="*", left="|", symmetric=True), center=True, width=3))
*=*
|A|
*=*
>>> print(Title("A", border=Border(top="=", SW="*", left="|", symmetric=True), center=True, width=5))
*===*
| A |
*===*
>>> print(Title("title", border=Border(bottom="="), center=True, width=79))
title
=============================================================================
"""
text = text if text is not None else self.text
width = width if width is not None else self.width
textlines = text.split("\n")
textwidth = (
max(len(l.strip()) for l in textlines)
+ len(self.border.left)
+ len(self.border.right)
)
width = textwidth if not width else width
top_border = self.border.border_top(width)
bottom_border = self.border.border_bottom(width)
if len(top_border) < textwidth:
if self.center:
top_border = f"{top_border:^{textwidth}}".rstrip()
bottom_border = f"{bottom_border:^{textwidth}}".rstrip()
content_with_border = "" if not top_border else (top_border + "\n")
content_with_border += text
content_with_border += "" if not bottom_border else ("\n" + bottom_border)
return content_with_border
content_with_border_list = []
start = 0 if not self.border.left else 1
for line in textlines:
post_indent = " " * (
width
- len(line)
- (0 if not self.border.right else len(self.border.right))
)
content_with_border_list.append(
(
self.border.left + line[start::] + post_indent + self.border.right
).rstrip()
)
content_with_border = "\n".join(content_with_border_list)
return "\n".join([top_border, content_with_border, bottom_border]).rstrip()
starborder = Border(SW="*", top="=-", left="", symmetric=True)
fullborder = Border(
SW="└", SE="┘", NE="┐", NW="┌", top="─", bottom="─", left="│", right="│"
)
if __name__ == "__main__":
import doctest
doctest.testmod()
-
\$\begingroup\$ "As a sidenote notice that we wrote
*
and not*
" – I think the Stackexchange software gobbles any spaces that you put into the code markup. \$\endgroup\$Schmuddi– Schmuddi2021年11月24日 15:16:39 +00:00Commented Nov 24, 2021 at 15:16
As a beginner, you have done fantastic!
There is always something to improve, in your code, mainly, performance.
print()
Instead of priniting again and again, create a string of output, and print it in a single call. It is way faster than printing in sperate calls.
Your code has doc-strings, follows pep8 and is overall very readable. You could also add type-hints to make it even more explicit.
In terms of user experience, it would be more comfortable to type 1-9 instead of both the row and column. You could manage your grid as a nine element list.
At the end, your project is awesome!
-
1\$\begingroup\$ I disagree with your
print
-aggregation suggestion - at this scale, performance differences will be invisible and legibility is more important. \$\endgroup\$Reinderien– Reinderien2021年11月24日 02:13:32 +00:00Commented Nov 24, 2021 at 2:13