Based on this Code Review answer, I have created a noughts and crosses game in Python, which asks for an index (1 to 9) and updates the grid, and displays it on the screen. I have done some very untidy programming in places, and would please like some help on how to make it more readable, and avoid repeating statements, such as print_grid(). Also I would like to make the 'Change player' part more simple. Any other constructive criticism or improvements also gratefully recieved.

winning_lines = [(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 get_turn(player):
    return int(input("Player %d: " % player).strip()) - 1

def format_player(num):
    return ' OX'[num]

def print_grid(board):
    """Formats and prints the grid."""


def check_for_win(board, player):
    """Returns True for win, anything else, False"""
    for winning_line in winning_lines:  # Take all possible winning lines one by one
        for pos in winning_line: # Test each index of the winning line
            if board[pos] != format_player(player):
        else: return True
    return False

def play_game():
    player = 1
    board = [" "] * 9

    while True:
        place = get_turn(player)
        if place in range(9) and board[place] == " ":
            board[place] = format_player(player)

            if check_for_win(board, player):
                return player

            # Change player

            if player == 1:
                player = 2
                player = 1

winner = play_game()

print("Player %d wins!" % winner)

1 Answer 1

  1. The functions get_turn, format_player and play_game have no docstrings. What do they do? What do they return?

  2. In check_for_win, the value of format_player(player) does not change and so you could remember it in a local variable to avoid recomputing it on each time around the loop.

  3. check_for_win could be simplified using the built-in function all, like this:

    def check_for_win(board, player):
        """Returns True if player has won, otherwise False"""
        player = format_player(player)
        for winning_line in winning_lines:
            if all(board[pos] == player for pos in winning_line):
                return True
        return False

    and then that could be futher simplified using the built-in function any, like this:

    def check_for_win(board, player):
        """Returns True if player has won, otherwise False"""
        player = format_player(player)
        return any(all(board[pos] == player for pos in winning_line)
                   for winning_line in winning_lines)
  4. If the player's input is incorrect, there is no message explaining what the problem was.

  5. Repeatedly switching between player 1 and player 2 can be done using itertools.cycle, like this:

    from itertools import cycle
    for player in cycle((1, 2)):
        # etc.
  6. I think that printing the winner should be the responsibility of the play_game function, not of the top-level code.

  7. There is no check to see if the game is tied. If the board is full but there is no winner then the game continues to prompt player 2 for a move.


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.