I've recently started learning Python again after a long time away from it and doing MATLAB at university. Today, I spent some time working on a first little project of mine. Right now it works as it should minus ending the game when all the fields are full. I would love to get advice on how I could improve the code and would like to fix any bad habits early on in learning.
mapA = [[" ", "|", " ", "|", " "],
[" ", "|", " ", "|", " "],
[" ", "|", " ", "|", " "]]
def printMap():
# prints out the map
map = ["".join(mapA[0]),
"".join(mapA[1]),
"".join(mapA[2])]
print(map[0])
print(map[1])
print(map[2])
# makes a list of just the fields with x and o
mapB = [map[0].split("|"),
map[1].split("|"),
map[2].split("|")]
for x in range(3): # converts the board to numbers
for y in range(3):
if mapB[x][y] == "X":
mapB[x][y] = 1
elif mapB[x][y] == "O":
mapB[x][y] = -1
elif mapB[x][y] == " ":
mapB[x][y] = 0
for x in range(3):
if abs(sum(mapB[x])) == 3: # checks for full rows
return "END"
elif abs(int(mapB[0][x]) + int(mapB[1][x]) + int(mapB[2][x])) == 3: # checks for full columns
return "END"
if abs(int(mapB[0][0]) + int(mapB[1][1]) + int(mapB[2][2])) == 3: # checks for full right diagonal
return "END"
elif abs(int(mapB[0][2]) + int(mapB[1][1]) + int(mapB[2][0])) == 3: # checks for full left diagonal
return "END"
counter = 0
def choice(): # allows the user to choose O or X
print("O or X?")
symbol = input().lower() # case insensitive
if symbol == "o":
return ["O", "X"]
elif symbol == "x":
return ["X", "O"]
else:
print("Invalid symbol")
return choice() # restarts the function if the user input neither O nor X
def placement(symbol):
print("Place " + symbol + " in form: rowNumber columnNumber")
x = input()
coordinates = x.split(" ")
while len(coordinates) != 2 or coordinates[0].isnumeric() != True or coordinates[1].isnumeric() != True:
print("Invalid input.")
print("Place " + symbol + " in form: rowNumber columnNumber")
x = input()
coordinates = x.split(" ")
while mapA[int(coordinates[0]) - 1][int(coordinates[1]) * 2 - 2] != " ":
if mapA[int(coordinates[0]) - 1][int(coordinates[1]) * 2 - 2] != " ":
print("That space is taken.")
else:
print("Invalid input.")
print("Place " + symbol + " in form: rowNumber columnNumber")
x = input()
coordinates = x.split(" ")
if coordinates[0] == "1" and coordinates[1] == "1":
mapA[0][0] = symbol
elif coordinates[0] == "1" and coordinates[1] == "2":
mapA[0][2] = symbol
elif coordinates[0] == "1" and coordinates[1] == "3":
mapA[0][4] = symbol
elif coordinates[0] == "2" and coordinates[1] == "1":
mapA[1][0] = symbol
elif coordinates[0] == "2" and coordinates[1] == "2":
mapA[1][2] = symbol
elif coordinates[0] == "2" and coordinates[1] == "3":
mapA[1][4] = symbol
elif coordinates[0] == "3" and coordinates[1] == "1":
mapA[2][0] = symbol
elif coordinates[0] == "3" and coordinates[1] == "2":
mapA[2][2] = symbol
elif coordinates[0] == "3" and coordinates[1] == "3":
mapA[2][4] = symbol
return printMap()
symbol = choice()
printMap()
end = ""
while end != "END":
end = placement(symbol[0])
if end == "END":
print("Game Finished")
break
end = placement(symbol[1])
if end == "END":
print("Game Finished")
break
tictac()
-
\$\begingroup\$ Welcome to CodeReview. Here we can review complete and working code. If your application is not finished and you are struggling with some problem then please consider to ask question on StackOverflow. \$\endgroup\$Peter Csala– Peter Csala2021年04月01日 06:37:24 +00:00Commented Apr 1, 2021 at 6:37
1 Answer 1
The first thing I notice is the two separate maps. Since the only actual information you're storing is X / O positions in a 3x3 grid, I think it is good practice to only store those values. Then add any additional formatting when you print. I've also used snake_case
for function and variable names, which is the preferred style. The first section could be rewritten:
x_o_grid = [
[' ', ' ', ' ', ],
[' ', ' ', ' ', ],
[' ', ' ', ' ', ]
]
def print_map():
for row in x_o_grid:
print('|'.join(row))
You can continue with this pattern by again only storing -1, 0, or 1 in this grid. Then doing your comparison in a slightly different way.
I would suggest refactoring the portion of printMap
where you check for matches into a new check_for_solution
function:
def check_for_solution():
# Check rows
for row in x_o_grid:
if all(x == row[0] for x in row):
if row[0] != ' ':
return row[0]
# Check columns
for i in range(len(x_o_grid)):
column = [row[i] for row in x_o_grid]
if all(x == column[0] for x in column):
if column[0] != ' ':
return column[0]
# Check diagonals
diagonal1 = [x_o_grid[i][i] for i in range(len(x_o_grid))]
diagonal2 = [x_o_grid[len(x_o_grid) - i][len(x_o_grid) - i] for i in range(len(x_o_grid))]
for diagonal in [diagonal1, diagonal2]:
if all(x == diagonal[0] for x in diagonal):
if diagonal[0] != ' ':
return diagonal[0]
return None
In the above rewrite, I have tried to avoid hardcoding the indices of the grid we are checking. And instead of using sum()
to check for equivalence, I'm using the all()
function where we check to see if the first element of the list is equal to every other element. This will return true if it is or false if any element is off.
This helps to make our code reusable and neater and allows for more flexible data storage. By not using range(3)
and instead just looping through the available rows, we can accommodate any square matrix when checking for solutions.
The placement you can change != True
to not
. I believe the below will also error out if you pass a None
or a float
, so these should be captured in try: except:
statements.
while not len(coordinates) == 2 or not coordinates[0].isnumeric() or not coordinates[1].isnumeric():
You might also consider f-strings, available in Python 3.6+. I believe this is preferred, but may come down to preference.
print(f"Place {symbol} in form: rowNumber columnNumber")
The placement logic can also be simplified to:
x_o_grid[int(coordinates[0]) - 1][int(coordinates[1]) - 1] = symbol
I would also recommend you change the end
variable to a bool
since you're only really checking for two states:
end = False
while not end:
...
I believe the check_solutions
function will require a rewrite of your main loop, as it now returns either a none or the winning value.