3
\$\begingroup\$

I've been teaching myself Python3 for about a week now and I decided to put my skills to the test by writing a simple Tic-Tac-Toe game. Here is what I came up with:

#!/usr/bin/env python3

import random

board = [0]*9

def resetBoard():
    for index, i in enumerate(board):
        board[index] = 0

def printBoard():
    counter = 0
    s = ""
    print("|-----|-----|-----|")
    for index, i in enumerate(board):
        s += "|  "
        if(board[index] == 0):
            s += str(index)
        if(board[index] == 1):
            s+= "x"
        if(board[index] == 2):
            s += "o"
        s += "  "
        counter += 1
        if(counter == 3):
            s += "|"
            print(s)
            print("|-----|-----|-----|")
            s = ""
            counter = 0

def getPlayers():
    return input("1 or 2 Players?")

def moveSquare(xOro, ai):
    square = 0
    flag = 0
    while(board[square] != 0 or flag == 0):
        if(ai == 0):
            if(xOro == 1):
                square = eval(input("Choose a square to place X: "))
            if(xOro == 2):
                square = eval(input("Choose a square to place O: "))
        if(ai == 1):
            square = random.randint(0, 8)
        flag = 1
    board[square] = xOro
    print("\n\n")

def checkForWin():
    if(board[0] == board[1] and board[1] == board[2] and board[2] != 0):
        return 1
    if(board[3] == board[4] and board[4] == board[5] and board[5] != 0):
        return 1
    if(board[6] == board[7] and board[7] == board[8] and board[8] != 0):
        return 1
    if(board[0] == board[3] and board[3] == board[6] and board[6] != 0):
        return 1
    if(board[1] == board[4] and board[4] == board[7] and board[7] != 0):
        return 1
    if(board[2] == board[5] and board[5] == board[8] and board[8] != 0):
        return 1
    if(board[0] == board[4] and board[4] == board[8] and board[8] != 0):
        return 1
    if(board[2] == board[4] and board[4] == board[6] and board[6] != 0):
        return 1
    for index, i in enumerate(board):
        if(i  == 0):
            break
        if(index == 8):
            return 2
    return 0

def gameOver():
    return eval(input("\nPress 0 and then ENTER to play again!"))

# Main
random.seed()
quit = 0
while(quit == 0):
    players = 0
    win = 0
    while(players != 1 and players != 2):
        players = eval(getPlayers())
    resetBoard()
    game = 0 #game = 1 when game is finished
    while(game == 0): #game loop
        printBoard()
        moveSquare(1, 0)
        game = checkForWin()
        if(game > 0):
            win = game
            break
        printBoard()
        if(players == 1):
            moveSquare(2, 1)
        if(players == 2):
            moveSquare(2, 0)
        game = checkForWin()
        if(game > 0):
            win = game + 1
            break
    if(win == 1):
        print("\nX Wins!")
    if(win == 2):
        print("\nO Wins!")
    if(win == 3):
        print("\n The game is a draw!")
    quit = gameOver()
exit()

I am interested in the following points about my code:

  • How can I make my code smaller and more optimized?
  • The game displays "O Wins" instead of "The game is a draw!" and I'm not sure why.
  • How can I sanitize input to prevent crashes if a string is entered instead of an integer (for example when choosing a square on the board)?
  • Any additional points to improve my code or general tips that will help me become a better Python programmer.
\$\endgroup\$
5
\$\begingroup\$
#!/usr/bin/env python3

import random

board = [0]*9

Having global variables that change is considered a bad idea. You'd be better off putting this variable and the methods inside a class.

def resetBoard():
    for index, i in enumerate(board):
        board[index] = 0

You could also do board[:] = [0] * 9

def printBoard():
    counter = 0

In python you rarely need to keep track of counters like this

    s = ""

In python, you should avoid building strings by adding different pieces together

    print("|-----|-----|-----|")
    for index, i in enumerate(board):
        s += "|  "
        if(board[index] == 0):

The point of enumerate is that i is the value in board. Use it rather then refetching it here.

            s += str(index)
        if(board[index] == 1):
            s+= "x"
        if(board[index] == 2):
            s += "o"
        s += "  "

By building the string this way its hard to follow exactly what the string is.

        counter += 1
        if(counter == 3):

Instead of making the counter use `if index % 3 == 2:' Also you don't need the parens

            s += "|"
            print(s)
            print("|-----|-----|-----|")
            s = ""
            counter = 0

Your whole algorithm here is rendered more complicated because you are iterating over cells rather the rows. Your algorithm would be much more natural with rows

def getPlayers():
    return input("1 or 2 Players?")

Shouldn't this return an int, not a string?

def moveSquare(xOro, ai):

variable names are recommended to follow x_or_o as a naming convetion

    square = 0
    flag = 0

If this is a boolean flag, use False not 0

    while(board[square] != 0 or flag == 0):

No need for the parens around the while expression

        if(ai == 0):

Again use True/False for boolean flags. No need for parens

            if(xOro == 1):
                square = eval(input("Choose a square to place X: "))
            if(xOro == 2):
                square = eval(input("Choose a square to place O: "))

Avoid magic numbers like 1 and 2 to denote X and O. In this case, I'd probably pass 'x' or 'o' strings.

        if(ai == 1):

Use an else:

            square = random.randint(0, 8)
        flag = 1

Flag is really terrible variable name because I don't know what it means. Boolean flags are usually an ugly solution

    board[square] = xOro
    print("\n\n")

def checkForWin():
    if(board[0] == board[1] and board[1] == board[2] and board[2] != 0):
        return 1

Don't use magic numbers like that. Instead create some global constants to return or something

    if(board[3] == board[4] and board[4] == board[5] and board[5] != 0):
        return 1
    if(board[6] == board[7] and board[7] == board[8] and board[8] != 0):
        return 1
    if(board[0] == board[3] and board[3] == board[6] and board[6] != 0):
        return 1
    if(board[1] == board[4] and board[4] == board[7] and board[7] != 0):
        return 1
    if(board[2] == board[5] and board[5] == board[8] and board[8] != 0):
        return 1
    if(board[0] == board[4] and board[4] == board[8] and board[8] != 0):
        return 1
    if(board[2] == board[4] and board[4] == board[6] and board[6] != 0):
        return 1

A lot duplication here.

    for index, i in enumerate(board):
        if(i  == 0):
            break
        if(index == 8):
            return 2
    return 0

def gameOver():
    return eval(input("\nPress 0 and then ENTER to play again!"))

Eval is a bit dangerous because the user can put in anything they want.

# Main

You should your main logic in a function

random.seed()
quit = 0
while(quit == 0):

Make quite a boolean variable

    players = 0
    win = 0
    while(players != 1 and players != 2):
        players = eval(getPlayers())
    resetBoard()
    game = 0 #game = 1 when game is finished

Game becoming 1 when the game is finished seems backwards

    while(game == 0): #game loop
        printBoard()
        moveSquare(1, 0)
        game = checkForWin()
        if(game > 0):
            win = game
            break

Not much point checking game == 0 above if you are just going to break anyways

        printBoard()
        if(players == 1):
            moveSquare(2, 1)
        if(players == 2):
            moveSquare(2, 0)
        game = checkForWin()
        if(game > 0):
            win = game + 1
            break

See how the two parts of the loop are very similar? Suggests that you should get rid of the duplication.

    if(win == 1):
        print("\nX Wins!")
    if(win == 2):
        print("\nO Wins!")
    if(win == 3):
        print("\n The game is a draw!")
    quit = gameOver()

Having gameOver() return whether or not you should quit seems confusing. The name doesn't suggest that

exit()

Not much point in calling quit, the program is about to terminate anyways

And just to demonstrate how I'd do this:

#!/usr/bin/env python3
import random

WINNING_LINES = [
    [0, 1, 2],
    [3, 4, 5],
    [5, 6, 7],

    [0, 3, 5],
    [1, 4, 6],
    [2, 5, 7],

    [0, 4, 7],
    [2, 4, 5]
]

STALEMATE = '!'

class TicTacToe(object):
    def __init__(self):
        self.board = [' '] * 9

    def print(self):
        # make a copy of the board
        cells = self.board[:] 

        # put a digit in every blank cell
        for index, cell in enumerate(cells):
            if cell == ' ':
                cells[index] = str(index)

        # put spaces around the cells
        cells = [' %s ' % cell for cell in cells]

        # print the actual board
        print('|---|---|---|')
        for start in range(0, 9, 3):
            row = cells[start:start+3]
            print('|', '|'.join(row), '|', sep = '')
            print('|---|---|---|')

    def move_square(self, player, ai):
        square = -1
        while square < 0 or square >= 9 or self.board[square] != ' ':
            if ai:
                square = random.randrange(9)
            else:
                question = "Choose a square to place {}:".format(player.upper())
                square_text = input(question)
                print(square_text)
                try:
                    square = int(square_text)
                except ValueError:
                    # user didn't enter a number
                    square = -1
        self.board[square] = player
        print("\n\n")

    def status(self):
        # check for a win
        for a, b, c in WINNING_LINES:
            if self.board[a] == self.board[b] and self.board[b] == self.board[c] and self.board[a] != ' ':
                return self.board[a]

        if not any(cell == ' ' for cell in self.board):
            return STALEMATE
        return ' '

def get_players():
    while True:
        text = input("1 or 2 players")
        try:
            value = int(text)
        except ValueError:
            pass
        else:
            if 0 < value < 3:
                return value




def play_again():
    return input("\nPress 0 and then ENTER to play again!") == '0'

GAME_OVER_MESSAGES = {
    'X' : 'X wins!',
    'O' : 'O wins!',
    '!' : 'The game is a draw!'
}

def main():

    while True:

        # create a list to denote whether
        # a player is AI
        ai_players = [False, False]
        if get_players() == 1:
            ai_players[1] = True


        tic_tac_toe = TicTacToe()
        player_turn = 0
        player_codes = 'XY'
        while tic_tac_toe.status() == ' ':
            tic_tac_toe.print()
            tic_tac_toe.move_square(player_codes[player_turn], ai_players[player_turn])
            player_turn = (player_turn + 1) % 2

        print(GAME_OVER_MESSAGES[tic_tac_toe.status()])

        if not play_again():
            break


if __name__ == '__main__':
    main()
\$\endgroup\$
1
\$\begingroup\$

Couple of comments on general style:

while(players != 1 and players != 2):
    players = eval(getPlayers())

I would expect getPlayers() to get the player information and do any error handling. Thus in the main loop this should just be a single call:

players = getPlayers()

You have lots of places where you do the same thing in two different ways:

    if(players == 1):
        moveSquare(2, 1)
    if(players == 2):
        moveSquare(2, 0)

You can simplify this as

    moveSquare(2, players - 1)

Same sort of thing here:

if(win == 1):
    print("\nX Wins!")
if(win == 2):
    print("\nO Wins!")
if(win == 3):
    print("\n The game is a draw!")

Can be much neater like this:

print(result[win])   # Now just define the result array.

Your problem with printing the wrong thing on a draw is here:

    if(game > 0):
        win = game
        break

Notice this is different from your second check here

    game = checkForWin()
    if(game > 0):
        win = game + 1
        break

So checkForWin() returns 0 for no winner 1 for winner and 2 for a draw. The first check above will result in win being 1 or 2 even for a draw, while the second check will result in 2 or 3 (which is probably correct).

What you really want to do is make checkForWin() return the result you are looking for.

0:  for no winner
1:  for player one
2:  for player two
3:  for a draw.

You can even optimize checkForWin() by passing in the player that made the last move and the position they played in (as the winning move will include their last move).

#!/usr/bin/env python3

import random


# Put the ID of the square in each square for easy printing.
# We will put 'X' or 'O' in the square as we make each move
def resetBoard(board):
    for index, i in enumerate(board):
        board[index] = index

# Print the board
def printBoard(board):
    s = ""
    for index, i in enumerate(board):
        s += "|  %s  " % i
        if((index % 3) == 2):
            print("|-----|-----|-----|\n%s|" % s)
            s = ""
    print("|-----|-----|-----|")

# Function to get the human mve
def humanMove(playerToMove, board):

    print "Human Move (%s)" % playerToMove
    return eval(raw_input("Choose a square to place %s: " % playerToMove))

# Function to get the AI move
def aiMove(playerToMove, board):

    print "AI Move (%s)" % playerToMove
    return random.randint(0, 8)

def moveSquare(playerToMove, players, board):
    # Get the move from the cuttrny player
    # Note this passes weather they are an 'X' or an 'O' 
    # as a parameter.
    FirstTry    = False
    square      = 0
    while(board[square] == 'X' or board[square] == 'O' or FirstTry):
        square  = players[playerToMove](playerToMove, board)
        FirstTry    = False;
    board[square] = playerToMove            # Puts an 'X' or an 'O' on the board
    print("\n\n")

#
# This function returns a dictionary with move move functions.
# So on 'X' move call the result{'X'} function to get a move
# So we have to decide how to fill the dictionary based on user input.
def getPlayers():
    players = 0;
    while players != 1 and players != 2:
        players = input("1 or 2 Players?")

    if players == 2:
       # Two players so both elements call the humanMove function
       return {'X': humanMove, 'O': humanMove}

    if players == 1:
        human      = '?'
        while human != 'X' and human != 'O':
            human      = raw_input("Do you want to play X or O");
        if human == 'X':
            # Human is playing X ai is playing O so functions set appropriately.
            return {'X': humanMove, 'O': aiMove}
        else:
            return {'X': aiMove, 'O': humanMove}


#
# Return 'X' for winner is X
# Return 'O' for winner is O
# Return 'D' for a draw
# Return 'R' for game is still running.
def checkForWin(board):
    if(board[0] == board[1] and board[1] == board[2] and board[2] != 0):
        return board[0]
    if(board[3] == board[4] and board[4] == board[5] and board[5] != 0):
        return board[3]
    if(board[6] == board[7] and board[7] == board[8] and board[8] != 0):
        return board[6]
    if(board[0] == board[3] and board[3] == board[6] and board[6] != 0):
        return board[0]
    if(board[1] == board[4] and board[4] == board[7] and board[7] != 0):
        return board[1]
    if(board[2] == board[5] and board[5] == board[8] and board[8] != 0):
        return board[2]
    if(board[0] == board[4] and board[4] == board[8] and board[8] != 0):
        return board[0]
    if(board[2] == board[4] and board[4] == board[6] and board[6] != 0):
        return board[2]

    # No winner yet
    # decide if the game is still running.
    for index, i in enumerate(board):
        if(i  != 'X' or i != 'O'):
            return 'R'

    # No empty squares so it must be a draw.
    return 'D'

def gameOver():
    return raw_input("\nPress 0 and then ENTER to play again!")

# Main
random.seed()
quit = "0"
board = [0]*9

while(quit == "0"):
    resetBoard(board)
    winner          = 'R'
    players         = getPlayers()
    playerOrder     = ['X', 'O']
    currentPlayer   = 0;
    while True:
        printBoard(board)

        moveSquare(playerOrder[currentPlayer], players, board)
        winner = checkForWin(board)
        if winner != 'R':
            break

        currentPlayer   = (currentPlayer + 1) % 2

    result  = { 'X': "X Wins!",'O': "O Wins!",  'D': "The game is a draw!"}
    print "\n%s\n" % result[winner];

    quit = gameOver()
\$\endgroup\$
1
  • 1
    \$\begingroup\$ I think you've misunderstood the second argument to moveSquare. The first argument is the player number. The second number indicates whether or not this is an AI player. \$\endgroup\$ Oct 16 '11 at 18:35

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.