3
\$\begingroup\$

I have just decided to get into programming as a hobby (and possibly a job in the distant future). After learning the basics of Python, I made Tic-Tac-Toe to apply what I've learned. I would appreciate any and all feedback on my code and the way I set up the logic of the game. I would like to avoid picking up bad coding habits.

from random import randint
from time import sleep
from copy import deepcopy
#Setup game
def main():
    #Set default board values
    global board
    board = [[0, " 0 "], [1, " 1 "], [2," 2 "], [3," 3 "], [4," 4 "], [5," 5 "], [6," 6 "], [7," 7 "], [8," 8 "]]
    #Sets default counter value:
    global counter
    counter = 0
    #Calls function to draw board
    drawBoard()
#Draw game board
def drawBoard():
    #Draws game board with proper variables
    print(board[0][1] + "|"+ board[1][1] +"|" + board[2][1] + "\n---+---+--- \n" + board[3][1] +"|" + board[4][1] + "|"
          + board[5][1] + "\n---+---+--- \n" + board[6][1] + "|" + board[7][1] + "|" +  board[8][1] + "\n")
    #After board has been updated, initiates next turn
    nextTurn(counter)
#Execute the next turn    
def nextTurn(count):
    #If count is divisible by two, let the player make a move and increase turn counter
    if count % 2 == 0:
        global counter
        counter += 1
        playerMove()
    #If count is not divisible by two, let the cmputer make a move and increase turn counter
    else:
        counter += 1
        computerMove()
#Code for the player's move
def playerMove():
    #Get player input and try to convert it to an integer.  If it cannot be, prompt user to enter again
    try:
        boardSpot = int(input("It's your turn! Please select an empty space on the board by typing a number 1-9\n"))
    except ValueError:
        playerMove()
    #If the space is empty, proceed
    if board[boardSpot][1] != " X " and board[boardSpot][1] != " O " :
        #Set board spot to X and redraw the board
        board[boardSpot][1] = " X "
        if checkWinner(board, " X ") == True:
            print("You have won! \n")
            endOfGame()
        checkTie(board)
        drawBoard()
    #Else, let them input a new number by recalling playerMove 
    else:
        playerMove()
def computerMove():
    #Display some dialouge alerting the user that the computer is making its move
    print("It's the computer's Turn!")
    sleep(1)
    print("Thinking...")
    sleep(1)
    #For loop iterating through every board spot.  Checks to see if bot can win.  If it can, make that winning move!
    for boardSpot in range (0,9):
        #Makes a copy of the board
        boardCopy = deepcopy(board)
        #If boardspot is empty, continue with the check.  Else, move onto the next spot!
        if boardCopy[boardSpot][1] != " X " and boardCopy[boardSpot][1] != " O ":
            boardCopy[boardSpot][1] = " O "
            if checkWinner(boardCopy, " O "):
                board[boardSpot][1] = " O "
                print("You have lost to the bot! \n")
                endOfGame()
                drawBoard()
                return
    #For loop iterating through every board spot.  Checks to see if player can win.  If it can, prevent him from winning!
    for boardSpot in range(0,9):
        #Makes a copy of the board 
        boardCopy = deepcopy(board)
        #If boardspot is empty, continue with the check.  Else, move onto the next spot!
        if boardCopy[boardSpot][1] != " X " and boardCopy[boardSpot][1] != " O ":
            boardCopy[boardSpot][1] = " X "
            if checkWinner(boardCopy, " X "):
                board[boardSpot][1] = " O "
                drawBoard()
                return
    #If code has progressed this far, simply chose a random empty space
    randomMove = True
    while randomMove:
        boardSpot = randint(0,8)
        if board[boardSpot][1] != " X " and board[boardSpot][1] != " O ":
            board[boardSpot][1] = " O "
            randomMove = False
            drawBoard()
#Check if someone has won
def checkWinner(brd, lttr):
    return ((brd[6][1] == lttr and brd[7][1] == lttr and brd[8][1] == lttr) or # across bottom
     (brd[3][1] == lttr and brd[4][1] == lttr and brd[5][1] == lttr) or # across middle
     (brd[0][1] == lttr and brd[1][1] == lttr and brd[2][1] == lttr) or # across top
     (brd[6][1] == lttr and brd[3][1] == lttr and brd[0][1] == lttr) or # down left side
     (brd[7][1] == lttr and brd[4][1] == lttr and brd[1][1] == lttr) or # down middle
     (brd[8][1] == lttr and brd[5][1] == lttr and brd[2][1] == lttr) or # down right side
     (brd[6][1] == lttr and brd[4][1] == lttr and brd[2][1] == lttr) or # diagonal
    (brd[8][1] == lttr and brd[4][1] == lttr and brd[0][1] == lttr)) # diagonal
#Check for tie
def checkTie(brd):
    emptySpaces = 0
    #Loop through all the board spaces.  If the board space is not X or O, then it is free.
    for x in brd:
        if x[1] != " O " and x[1] != " X ":
            emptySpaces += 1
    #If there are 0 empty spaces, prompt user to play again or to quit the game
    if emptySpaces == 0:
        print("The board is full!  You have tied! \n")
        endOfGame()
#Either start a new game or quit the program depending on player input        
def endOfGame():
    response = input("Would you like to play again? Y/N \n")
    if response == "Y" or response == "y":
        print("Starting new game...")
        sleep(2)
        main()
    elif response == "N" or response == "n":
        print("Thanks for playing!")
        sleep(1.5)
        quit()
    else:
        print("You did not input a valid response!")   

if __name__ == "__main__":
    main()
\$\endgroup\$
  • \$\begingroup\$ Can you possibly make your checkWinner() method iteratively check the cells rather than hard-coding in the conditions? \$\endgroup\$ – Tamoghna Chowdhury Apr 1 '16 at 14:04
2
\$\begingroup\$

First:

if __name__ == "__main__":
    main()

You've put your logic into a function rather than have it at the module level. This is a good thing (tm)!

Some style things:

If you're beginning, PEP 8, the official style guide, is a must read. A few things:

  • Limit all lines to a maximum of 79 characters.

Maybe an 80 column max is a bit conservative with todays screens, but you get up to column 120 with code and 122 with comments. Anything further than column 100 is probably pushing standard width, especially with Python.

  • Method definitions inside a class are surrounded by a single blank line.

This is a big one for readability. You have no blank lines in your source code, so everything is just sort of all squished together.

Docstrings are described in PEP 257. These can and should replace your comments before the methods, as support is build into the language for them.

Python recommends using lower_case_with_underscores to name your functions.

I would print the board after the final move has been made.

And one VERY opinionated suggestion: Your input is a 3x3 grid of numbers. Use the 3x3 grid of numbers that exist on most full-size keyboards: the numpad: 789 / 456 / 123

Now on to looking at the code itself:

global

Since you're just beginning, I'll let global slide. For the most part you want to avoid using global data as it can easily lead to problems, but for board here it's the easy way to do it. I'd get rid of counter, but I'll get to that in a few paragraphs.

board = [[0, " 0 "], [1, " 1 "], [2," 2 "], [3," 3 "], [4," 4 "], [5," 5 "], [6," 6 "], [7," 7 "], [8," 8 "]]

Why are you using a two dimensional list here? You never access the first element of each sublist after assigning it. And since you're putting the numbers in ascending order anyway, you can use a List Comprehension to accomplish the declaration simply:

board = [' ' + str(i) + ' ' for i in range(9)]

If you use the numpad-ordering, using a list comprehension becomes harder and not worth the effort.

Now, since we've changed board, we need to fix drawBoard():

print(board[0] + "|"+ board[1] +"|" + board[2] + "\n---+---+--- \n" + board[3] +"|" + board[4] + "|"
      + board[5] + "\n---+---+--- \n" + board[6] + "|" + board[7] + "|" +  board[8] + "\n")

Even with removing the second layer of list, that's still a doozy to read. Using String Formatting, I think we can clean this up a bit. Along with multi-line strings, we get

"""\
{0}|{1}|{2}
---+---+---
{3}|{4}|{5}
---+---+---
{6}|{7}|{8}""".format(*board)

One thing remains to be explained here: I'm using the *board to unpack board into the arguments. Basically, it's the same as board[0],board[1],board[2],etc.

def drawBoard():
    [...]
    nextTurn(counter)

This breaks the idea of one method should do one thing. Effectively, your draw call also starts the next turn. And since the nextTurn call leads to drawing the board again, you've got a recursive loop here that doesn't need to happen, and doesn't make sense, if drawBoard() does what you claim it to do: draw the game board. To address this loop, I move on to the next point:

global counter

I don't think you need it. Rather than recursively call nextTurn with an increasing turn counter, define a game_loop() function. Call it from main() after you set up, and loop back and forth between player and AI turns. It would look something like this:

def game_loop():
    while True:
        playerMove()
        if game_has_ended(): break
        computerMove()
        if game_has_ended(): break
    endOfGame()

I would have game_has_ended() be a simple check to perform in the loop here, then endOfGame() would be refactored to include the printing of congratulations to the winning team.

One function should have one job. That's the biggest issue here is that (nearly) every function here does one thing, then another thing.

One final thought:

return ((brd[6][1] == lttr and brd[7][1] == lttr and brd[8][1] == lttr) or # across bottom
 (brd[3][1] == lttr and brd[4][1] == lttr and brd[5][1] == lttr) or # across middle
 (brd[0][1] == lttr and brd[1][1] == lttr and brd[2][1] == lttr) or # across top
 (brd[6][1] == lttr and brd[3][1] == lttr and brd[0][1] == lttr) or # down left side
 (brd[7][1] == lttr and brd[4][1] == lttr and brd[1][1] == lttr) or # down middle
 (brd[8][1] == lttr and brd[5][1] == lttr and brd[2][1] == lttr) or # down right side
 (brd[6][1] == lttr and brd[4][1] == lttr and brd[2][1] == lttr) or # diagonal
(brd[8][1] == lttr and brd[4][1] == lttr and brd[0][1] == lttr)) # diagonal

That is a mess. First of all, I don't even see why you pass in brd as an alias for board if board is global; you might as well just use it. Secondly, that's a lot of hard coded cases. Unfortunately, I don't see an easy way to clean it up, but using the commutative property of equality we can clean it up a little, at the very least. Here's a modified version:

def check_for_win():
    row  = board[0]==board[1]==board[2] or \
           board[3]==board[4]==board[5] or \
           board[6]==board[7]==board[8]
    col  = board[0]==board[3]==board[6] or \
           board[1]==board[4]==board[7] or \
           board[2]==board[5]==board[8]
    diag = board[0]==board[4]==board[8] or \
           board[2]==board[4]==board[6]
    # It's still messy though
    return row or col or diag

Don't be discouraged that there's a lot here, nobody starts out perfect. Your code is working, and that's the first step. These guidelines are mostly just that: guidelines to help write code that you (and others!) will be able to work with later. So, please, continue your learning. Hopefully this will help in some small way.

\$\endgroup\$
  • \$\begingroup\$ Thanks man, this was very helpful & informative. I've chosen your response as the answer. \$\endgroup\$ – Mikeman111 Apr 13 '16 at 11:39
2
\$\begingroup\$
  1. The board positions are numbered 0-8. Players are supposed to enter corresponding integers 0-8 to play those positions, but you prompt them to enter 1-9.

  2. In endOfGame() where you query a yes/no response from the user, you check for both upper- and lowercase responses: there are a couple idioms that can be used instead.

    A. Convert the response to a desired case and then just compare the result to the single case:

    response = input("Would you like to play again? Y/N \n").lower()
    if response == "y":
       #...
    elif response == "n":
        #...
    else:
        #...
    

    B. Use the if response in (...) pattern:

    response = input("Would you like to play again? Y/N \n")
    if response in ("Y", "y"):
        #...
    elif response in ("N", "n"):
        #...
    else:
        #...
    

    Or, combine the two, and allow for other valid (albeit verbose) inputs:

    response = input("Would you like to play again? Y/N \n").lower()
    if response in ("y", "yes", "is a bear catholic?"):
        #...
    elif response in ("n", "no", "meh"):
        #...
    else:
        #...
    

    Of course, idiom (A) doesn't work in situations where you care about the upper/lower case, or where it's important to keep exactly what was entered (perhaps for logging / auditing purposes). If that's the situation, instead of calling the .lower() method directly on the input() return value, you can use: if response.lower() in (...):" for the input classes where letter case doesn't matter.

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! Good job on your first answer. Just as a warning, very thin answers like this may not be very well received in the future so it may be better to include code from the question and maybe sample code of your own (although, I don't know if that will apply in this case) \$\endgroup\$ – SirPython Apr 12 '16 at 23:28

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.