First Python program: Tic-Tac-Toe (Followup)

Question

I am unsure if I followed the the suggestions on my previous post. I have only been working on programming for about 3 weeks so any more suggestions or insight would be more than welcomed.

import sys
from textwrap import dedent
import os
import random
os.system('CLS')
# board number setup
board = [0, 1, 2,
 3, 4, 5,
 6, 7, 8]
# Defines the board layout printed to the console
def board_layout():
 print(dedent(f''' 
 *************
 * {board[0]} | {board[1]} | {board[2]} *
 *-----------*
 * {board[3]} | {board[4]} | {board[5]} *
 *-----------*
 * {board[6]} | {board[7]} | {board[8]} *
 *************
 '''))
def main():
 players = ('Player','NPC')
 turn = 'Player'
 change_turn = 0
 for moves in range(9):
 if turn == 'Player': 
 while True:
 try:
 board_layout()
 player_move = int(input('Please select a spot: '))
 if board[player_move] != 'x' and board[player_move] != 'o':
 board[player_move] = 'x'
 check_winner()
 break
 except IndexError:
 print('please select valid spot')
 if turn == 'NPC':
 # npc move, chooses a random spot that is not taken 
 while True:
 npc = random.randint(0, 8)
 if board[npc] != 'o' and board[npc] != 'x':
 board[npc] = 'o'
 print('Computer chooses spot ', npc)
 check_winner()
 break 
 try:
 change_turn += 1
 turn = players[change_turn]
 except:
 change_turn = 0
 turn = players[change_turn]
 else:
 print('You Tied')
 end() 
def end():
 print('Thank you for playing')
 answer = input('Would you like to play again?: Y/N')
 if answer.lower() == 'n':
 quit()
 elif answer.lower() == 'y':
 clear_board()
 main()
 else:
 print('Please choose a valid option')
 end()
def clear_board():
 for i in range(9):
 board[i] = i
# checks for a winner when called at end of each turn 
def check_winner():
 # list of lists with all the winning combinations for from the tic tac toe board
 winning_list = [[board[0], board[1], board[2]], [board[3], board[4], board[5], ],
 [board[6], board[7], board[8]], [board[0], board[4], board[8]],
 [board[2], board[4], board[6]],
 [board[0], board[3], board[6]], [board[1], board[4], board[7]],
 [board[2], board[5], board[8]]]
 # iterates over the lists in winning_list
 for i, j, k in winning_list:
 # looks at the lists in winning_list to determine if a list has all x's for a win
 if i == 'x' and j == 'x' and k == 'x':
 print('X wins')
 end()
 # looks at the lists in winning_list to determine if a list has all o's for a win
 elif i == 'o' and j == 'o' and k == 'o':
 print('O wins')
 end()
if __name__ == "__main__":
 main()

• C. Harley: Many of your code improvements can be taken from this post, please have a look: codereview.stackexchange.com/questions/201506/…

Answer 1

You now have two methods of setting up the board. The first is direct initialization:

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

and the second is a resetting an already existing board:

def clear_board():
 for i in range(9):
 board[i] = i

I hate that there are two. It would be easy to make a mistake and change one (for example, changing to a 4x4 grid) but not the other.

---

board_layout() is an odd name for the function that prints the board. It is not laying out the board. I might call it print_board().

I dislike seeing the same thing over and over. In board_layout(), you have board[ ] appear 9 times in the format string. If you wanted to change the name of the game board, you'd have to edit the code in 9 places in this one function. You can eliminate these duplicates using the .format() command, instead of using a f-string. I know, seems like going backwards; f-strings are supposed to be an improvement!

''' 
*************
* {} | {} | {} *
*-----------*
* {} | {} | {} *
*-----------*
* {} | {} | {} *
*************
'''.format(*board)

*board takes the board list, takes the individual elements and "splats" them all as the arguments to the .format() call. Each argument, in turn, is substituted into the next {} format code.

---

Jumping ahead.

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

Again, here we have board[ ] repeated 24 times! There has got to be a better way. And there is. First, create as a global constant, a list of list of winning indices. Since these will never be modified, I've used tuples instead of lists.

WINNING_ROWS = ((0, 1, 2), (3, 4, 5), (6, 7, 8), # Rows
 (0, 3, 6), (1, 4, 7), (2, 5, 8), # Columns
 (0, 4, 8), (2, 4, 6)) # Diagonals

Now we just need to use these indices to check for a win condition. We can even use chained comparisons to make the test more concise:

 for i, j, k in WINNING_ROWS:
 if board[i] == board[j] == board[k] == 'x':
 print('X wins')

That tests board[i] == board[j] AND board[j] == board[k] AND board[k] == 'x', but we haven't repeated any of the terms in the test.

 elif board[i] == board[j] == board[k] == 'o':
 print('O wins')

And now we have.

Whenever you repeat code, you should think "maybe a loop" or "maybe a function". A loop doesn't seem right here. Let's use a function:

def has_won(player):
 for i, j, k in WINNING_ROWS:
 if board[i] == board[j] == board[k] == player:
 return True
 return False

Now you can use if has_won('x'): to check in the 'x' player has won after they have made their move, and if has_won('o'): to check if the 'o' player has won after they have made theirs.

We can condense this function. The all() function will test each of its arguments, and return True only if all of the arguments are true. We can use list comprehension to extract each index in turn from the row tuples:

def has_won(player):
 for row in WINNING_ROWS:
 if all(board[idx] == player for idx in row):
 return True
 return False

Like the all() function, there is an any() function, which returns True if any of its arguments are true. Again, we'll use list comprehension to loop over each row in WINNING_ROWS:

def has_won(player):
 return any(all(board[idx] == player for idx in row) for row in WINNING_ROWS)

For any of the winning rows, if all of the board locations contain the player's symbol, True is returned. Pretty darn concise, if you want to use it.

---

Checking for a valid spot:

if board[player_move] != 'x' and board[player_move] != 'o':
if board[npc] != 'o' and board[npc] != 'x':

Effectively the same test, repeated twice. And board[pos] referenced twice in each test. Don't Repeat Yourself. DRY. As opposed to Write Everything Twice, or WET. You want DRY code, not WET code. The in operator will test if the item on the left of in is contained in the container on the right.

def is_valid_move(move):
 return board[move] not in ('x', 'o')

Not bad. But is 12 a valid move? How about -1? Note that -1 will not cause an IndexError, it will just return the last element of the list (board[8]).

def is_valid_move(move):
 return move in range(9) and board[move] not in ('x', 'o')

---

Elephant in the room.

You play the game, win, play again, loose, play again, tie, play again. What is the stack trace at this point?

main() -> check_winner() -> end() -> main() -> check_winner() -> end() -> main() -> check_winner() -> end() -> main() -> check_winner() -> end() ...

If you make a mistake and enter invalid input in the end() method, you could even have end() -> end() repeats in that stack trace.

Do not use recursion as a substitute for looping!

---

Here is a possible implementation, which doesn't use recursion. Note that there are no global variables, other than the constant WINNING_ROWS. Since board is no longer global, it can be created brand new each time a game is started.

import random
WINNING_ROWS = ((0, 1, 2), (3, 4, 5), (6, 7, 8),
 (0, 3, 6), (1, 4, 7), (2, 5, 8),
 (0, 4, 8), (2, 4, 6))
def print_board(board):
 row = " {} | {} | {}\n"
 line = " ---+---+---\n" 
 print(((row + line) * 2 + row).format(*board))
def has_won(player, board):
 return any(all(board[idx] == player for idx in row) for row in WINNING_ROWS)
def is_valid_move(move, board):
 return move in range(9) and board[move] not in ('x', 'o')
def player_move(board):
 print_board(board)
 while True:
 try:
 move = int(input('Please select as spot: '))
 if is_valid_move(move, board):
 return move
 except ValueError:
 pass
 print('Invalid input.', end=' ')
def npc_move(board):
 while True:
 move = random.randint(0, 8)
 if is_valid_move(move, board):
 return move
def play_game():
 board = list(range(9))
 player = 'x'
 for _ in range(9):
 if player == 'x':
 move = player_move(board)
 else:
 move = npc_move(board)
 board[move] = player
 if has_won(player, board):
 print_board(board)
 print(f"{player.upper()} wins!")
 return
 player = 'o' if player == 'x' else 'x'
 print("Tie game")
def main():
 answer = 'y'
 while answer == 'y':
 play_game()
 answer = ''
 while answer not in ('y', 'n'):
 answer = input('Play again? (Y/N): ').lower()
if __name__ == '__main__':
 main()