3
\$\begingroup\$

I have written a simple Tic Tac Toe game using some ASCII art. Is there anything I can do to improve upon the logic and structure of the program?

"""A basic command line tic tac toe game"""
import os
import sys

gameBoard       = [['' for j in range(3)] for i in range(3)]
displayBoard    = [[' ' for j in range(46)] for i in range(25)]

def main():
    """Main method to control game"""
    player = 'X'#Player x to go first
    moveCounter = 1 #Keeps track of how many turns have been taken

    #Setup game
    initGame()
    printInitScreen()

    while True:
        #Get player input
        square = input(f"Player {player}, choose your square ->")

        if validateInput(square):
            updateGameBoard(square, player)
            updateDisplayBoard(square, player)
            printDisplayBoard()

            if moveCounter >= 4:
                checkIfWon(player)

            #Switch player
            if player == 'X':
                player = 'O'
            else:
                player = 'X'

            moveCounter += 1

        else:
            print("Please try again")

def initGame():
    """Create and set up game components"""

    #Fill board
    for i in range(25):
        #If on a row boarder set to _
        if i == 8 or i == 17:
            for j in range(46):
                displayBoard[i][j] = '_'
        else:
            for j in range(46):
                #If on column boarder set |
                if j == 15 or j == 31:
                    displayBoard[i][j] = '|'

    #Put numbers in corner of square
    displayBoard[0][0]   = '1'
    displayBoard[0][16]  = '2'
    displayBoard[0][32]  = '3'
    displayBoard[9][0]   = '4'
    displayBoard[9][16]  = '5'
    displayBoard[9][32]  = '6'
    displayBoard[18][0]  = '7'
    displayBoard[18][16] = '8'
    displayBoard[18][32] = '9'


def validateInput(input):
    """Validates user input"""
    #Check given char is allowed
    try:
        square = int(input[0]) #Check first char of input is number
    except:
        return False

    #Check nothing already in that square
    #Get the gameBoard index of users chosen square
    index = numToIndex(input)
    if gameBoard[index[0]][index[1]] != '':
        return False

    #If all ok
    return True

def updateGameBoard(input, player):
    """Keeps track of users moves"""
    #Update the array with new move
    index = numToIndex(input[0])
    gameBoard[index[0]][index[1]] = player

def printDisplayBoard():
    """Prints a string representation of board"""
    os.system('cls' if os.name == 'nt' else 'clear')  # Clear screen

    for row in displayBoard:
        print(''.join(row))

    print("")

def checkIfWon(player):
    """Checks to see if the last move won the game"""
    gameWon = False
    #Check Horiz
    for row in gameBoard:
        if row[0] == row[1] == row[2] == player:
            gameWon = True

    #Check Vert
    for i in range(3):
        if gameBoard[0][i] == gameBoard[1][i] == gameBoard[2][i] == player:
            gameWon = True

    #Check Diag
    if gameBoard[0][0] == gameBoard[1][1] == gameBoard[2][2] == player:
        gameWon = True
    if gameBoard[0][2] == gameBoard[1][1] == gameBoard[2][0] == player:
        gameWon = True

    if gameWon:
        print(f"Congratualtions player {player}, you won!")
        sys.exit()

def printGameBoard():
    """For debugging, prints gameboard"""
    for row in gameBoard:
        print(row)

def printInitScreen():
    """Prints the initial welcome screen"""
    header = """
    888   d8b        888                   888
    888   Y8P        888                   888
    888              888                   888
    888888888 .d8888b888888 8888b.  .d8888b888888 .d88b.  .d88b.
    888   888d88P"   888       "88bd88P"   888   d88""88bd8P  Y8b
    888   888888     888   .d888888888     888   888  88888888888
    Y88b. 888Y88b.   Y88b. 888  888Y88b.   Y88b. Y88..88PY8b.
     "Y888888 "Y8888P "Y888"Y888888 "Y8888P "Y888 "Y88P"  "Y8888
    """
    os.system('cls' if os.name == 'nt' else 'clear')  # Clear screen
    print(header)
    input("Press enter to start!")
    printDisplayBoard()

def updateDisplayBoard(num, player):
    """Add the players shape to the chosen position on display board"""
    shapes = {"X":
              [[' ',' ',' ',' ','?','8','8','8','8','P',' ',' ',' ',' '],
               [' ',' ',' ',' ',' ','`','8','8','`',' ',' ',' ',' ',' '],
               ['8','b',',','_',' ',' ','8','8',' ',' ','_',',','d','8'],
               ['8','8','8','8','8','S','I','C','K','8','8','8','8','8'],
               ['8','P','~',' ',' ',' ','8','8',' ',' ',' ','~','?','8'],
               [' ',' ',' ',' ',' ',',','8','8','.',' ',' ',' ',' ',' '],
               [' ',' ',' ',' ','d','8','8','8','8','b',' ',' ',' ',' ']],
               "O":
              [[' ',' ',' ',' ',' ',' ','%','%',' ',' ',' ',' ',' ',' '],
               [' ',' ',' ',' ','%','%',' ',' ','%','%',' ',' ',' ',' '],
               [' ',' ','%','%',' ',' ',' ',' ',' ',' ','%','%',' ',' '],
               ['%','%',' ',' ',' ',' ',' ',' ',' ',' ',' ',' ','%','%'],
               [' ',' ','%','%',' ',' ',' ',' ',' ',' ','%','%',' ',' '],
               [' ',' ',' ',' ','%','%',' ',' ','%','%',' ',' ',' ',' '],
               [' ',' ',' ',' ',' ',' ','%','%',' ',' ',' ',' ',' ',' ']]}

    shape = shapes[player]
    num = int(num[0])

    offsets = [[0 ,0],[0 ,16],[0 ,32],
               [9 ,0],[9 ,16],[9 ,32],
               [17,0],[17,16],[17,32]]

    iOffset = offsets[num-1][0]
    jOffset = offsets[num-1][1]

    for i in range(iOffset, iOffset + 7):
        for j in range(jOffset, jOffset + 14):
            displayBoard[i+1][j] = shape[i - iOffset][j - jOffset]

def numToIndex(num):
    """Returns index [i,j] for given 'square' on board"""
    num = int(num[0])
    indexList = []
    for i in range (3):
        for j in range(3):
            indexList.append([i,j])

    return indexList[int(num)-1]

if __name__ == '__main__':
    main()
\$\endgroup\$
3
\$\begingroup\$
gameBoard       = [['' for j in range(3)] for i in range(3)]
displayBoard    = [[' ' for j in range(46)] for i in range(25)]

For me, time spend aligning source code is time wasted. It takes too much time initially. Moreover, if any variable is renamed throughout the code (or cleaned up etc. etc.), the aligning will get messed up. So it actively interferes with refactoring. This is highly subjective though, and I like to look at aligned code.


gameBoard       = [['' for j in range(3)] for i in range(3)]

A game board (or game board state, more precisely) consists of squares, filled with X'es or O's. What's missing is an object to represent those. For me, this is a bit too "stringly typed".

displayBoard    = [[' ' for j in range(46)] for i in range(25)]

This is the start of a problem in the code: unexplained numeric literals. Why 46 or 25? Just perform the calculations and let the computer compute. Make named constants out of the calculations and then use those.

player = 'X'#Player x to go first

If you name the identifier currentPlayer then the comment would not be necessary. Often choosing a good name will make comments unnecessary.

I'd try and avoid trailing comments as they tend to disappear on long lines. Again, this is somewhat subjective.

Use spaces before and after # please.

updateGameBoard(square, player)
updateDisplayBoard(square, player)

I spot a design issue here. The UI should display the state of the game, captured in gameBoard. So I would expect a printDisplayBoard(gameBoard) instead. Currently the game logic and UI is mixed and duplicated.

if moveCounter >= 4:
    checkIfWon(player)

This is not so intuitive to me. The game ends when a row, column or diagonal of three elements is formed. Currently you may have two winning players! If the board is filled without this happening then the game is a draw.

Deviating from this game logic is not a good idea as it is easy to make mistakes. Moreover, and more importantly, it fails if the application is ever extended to, for instance, a 4x4 board. Now I don't expect "tic tac toe too" to happen soon, but for actual application code, this is rather important.

My SudokuSolver was programmed as generically as possible. When I was done it literally took a minute to support differently sized and even special Sudoku's with additional diagonals and such. This shows the importance of programming as generically as possible.

def initGame():

I'd create drawHorizontalLine and drawVertialLine functions or similar.

displayBoard[0][0]   = '1'
displayBoard[0][16]  = '2'
displayBoard[0][32]  = '3'

I like how you clearly mark the squares of the board. However, this also is one of the clearest examples of you doing the computing instead of the computer. It should be relatively easy to create a single for loop and compute the x and y of the positions.

Basically you're buffering the image before displaying it, rather than making a single print with spaghetti code. That's very good.

os.system('cls' if os.name == 'nt' else 'clear')  # Clear screen
.
.
.
os.system('cls' if os.name == 'nt' else 'clear')  # Clear screen

Repeat 10 times: "I will keep to DRY principles".

If you repeat such a line, then put the line in a function and call that. If you add another method of clearing the screen then likely one of the locations with the same line of code will be forgotten. Looking at the comment, you've already thought of a name and started typing.

Of course, there is a lot of DRY (don't repeat yourself) failures where the same code is used but with different integer values, but this one stood out.

shapes = [...]

Do you really want that variable to be assigned the entire shape all the time? That needs to be a constant or - in real code - a resource that is being read once.

num = int(num[0])

Wait, what? Why? If the reason for code is not immediately clear, then I expect a comment!

offsets = [[0 ,0],[0 ,16],[0 ,32],
           [9 ,0],[9 ,16],[9 ,32],
           [17,0],[17,16],[17,32]]

Which offsets would that be? End of code, must be Friday :)

def numToIndex(num):

Finally, a function that calculates things! I knew you could do it.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Hi, thank you for taking the time to read through and help me with my code. I have learnt a lot from your answer. I'm sometimes cautious of using functions where maybe I don't need to, but you've cleared some things up for me. printDisplayBoard(gameBoard) This especially makes perfect sense now I have seen it. \$\endgroup\$ – WireInTheGhost Apr 28 at 9:44
  • \$\begingroup\$ Some people try and create methods with just a few lines, say 5-7 (for a higher level language). That's a bit too extreme for me, but in general large functions / methods are more troublesome than shorter ones, especially if they use a lot of branching (loops, if statements and switches). This is referred to as the cyclomatic complexity of a function. Programming is basically fighting complexity. \$\endgroup\$ – Maarten Bodewes Apr 28 at 10:26

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.