4
\$\begingroup\$

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()
Toby Speight
87.1k14 gold badges104 silver badges322 bronze badges
asked Nov 21, 2021 at 6:21
\$\endgroup\$
1
  • 2
    \$\begingroup\$ This is really good for someone just starting out! Keep it up! \$\endgroup\$ Commented Nov 21, 2021 at 9:56

2 Answers 2

2
\$\begingroup\$

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

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 how player_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

enter image description here

So how do we get there?

  1. 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()
answered Nov 21, 2021 at 23:42
\$\endgroup\$
1
  • \$\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\$ Commented Nov 24, 2021 at 15:16
1
\$\begingroup\$

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!

answered Nov 21, 2021 at 11:46
\$\endgroup\$
1
  • 1
    \$\begingroup\$ I disagree with your print-aggregation suggestion - at this scale, performance differences will be invisible and legibility is more important. \$\endgroup\$ Commented Nov 24, 2021 at 2:13

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.