1
\$\begingroup\$

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()
\$\endgroup\$
1
  • \$\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\$ Apr 1 at 6:37
2
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.