5
\$\begingroup\$

Follow-up from: Back to Basics - Tic Tac Toe

Again, any optimizations and critique is welcome! I'm particularly interested if it involves numpy, scipy | bitwise operators.

import numpy as np
from random import randint


def print_board(board):
    print("""
  +---+---+---+
c | {}   {}   {} |
  +   +   +   +
b | {}   {}   {} |
  +   +   +   +
a | {}   {}   {} |
  +---+---+---+
    1   2   3
""".format(*list(x if x != '0' else ' ' for x in board)))


def check_for_win(board):

    def check_diagonal(dia):
        return '0' not in dia and len(set(dia)) == 1

    a = np.array(board).reshape(3, 3)

    # check lr diagonal
    if check_diagonal(a.diagonal()):
        return True

    # check rl diagonal
    if check_diagonal(np.fliplr(a).diagonal()):
        return True

    # check rows & cols
    for matr in (a, np.transpose(a)):
        for row in matr:
            if '0' not in row and len(set(row)) == 1:
                return True

    return False


if __name__ == "__main__":
    board = ['0'] * 9
    codes = ('c1', 'c2', 'c3', 'b1', 'b2', 'b3', 'a1', 'a2', 'a3')
    turn, user = 1, 'X' if bool(randint(0, 1)) else 'O'
    print(f'Welcome! First to play is {user}!')
    print_board(board)

    while True:
        print(f'Player {user}: ')
        code = input().strip().lower()

        if code in codes:
            idx = codes.index(code)

            if board[idx] == '0':
                board[idx] = user
                print_board(board)

                if turn >= 5:
                    if check_for_win(board):
                        print(f'{user} won in {turn} turns! Congratulations!')
                        break
                    elif '0' not in board:
                        print("Aw, it's a draw!")
                        break

                user = 'X' if user == 'O' else 'O'
                turn = turn + 1
            else:
                print('Woops! You need to pick an empty space.')
        else:
            print("Hmm, that's not a valid input.")

\$\endgroup\$
3
\$\begingroup\$

I see you've removed the bugs. Good job. Let's clean it up some more.

Zero -vs- Naught

It is a little disconcerting to read the code and see '0' not in board or if board[idx] == '0'. It looks like it is hard-coded checks for the "Naughts" player, when it is in fact, a check for an empty square. '0' and 'O' are not all that different in certain fonts.

It could be an improvement to change identifier of the empty squares. But what to change it to?

A space comes to mind. It looks empty, just like the square. More over,

print("""
    ... omitted ...
""".format(*list(x if x != '0' else ' ' for x in board)))

could be reduced to:

print("""
    ... omitted ...
""".format(*list(x for x in board)))

since the empty indicator would already be a space. And since board is already a list, this could further reduce to:

print("""
    ... omitted ...
""".format(*board))

which is a definite win.

Deconstructing Assignment

What does this statement do, in 7 words or less?

turn, user = 1, 'X' if bool(randint(0, 1)) else 'O'

I can't come up with one 7 word sentence, but perhaps you can. If so, same question for this statement:

board, codes, turn, user = ['0'] * 9, ('c1', 'c2', 'c3', 'b1', 'b2', 'b3',
    'a1', 'a2', 'a3'), turn, user = 1, 'X' if bool(randint(0, 1)) else 'O'

It is similar to the previous statement, in that a couple of variables on the left are being assigned values from the right. The problem is none of these variables are related to each other. If you have an XYZ coordinate, using x, y, z = 0, 1, 2 can be reasonable, but for unrelated concepts, use separate statements:

turn = 1
user = 'X' if bool(randint(0, 1)) else 'O'

Cohesive Code

There are 21 lines between code = input().strip().lower() and print("Hmm, that's not a valid input."). Between those lines, we have a check for a win by either player, a check for a draw game, code to switch players, and code to increment the turn number. Because the code for these things is only valid when the move is valid, 4 levels of indenting are needed.

The while True loop is doing double duty. It is looping once for each turn of the game, where the state of the game changes, and once for each invalid input, where the state of the game doesn't change.

Code is more cohesive when related lines of code near one another. So the code should be organized more like:

while True:
    # get and validate move
    # update board
    # display new board
    # check for win or draw game
    # switch players
    # increment turn number

This also reduces the amount of indentation you need in the code, which is sometimes called "left-leaning" code.

The "get and validate move" will have its own loop. It could be written in place, but it might be better moved to its own function:

def get_player_move(board, user):

    print(f'Player {user}: ')

    while True:
        code = input().strip().lower()

        if code in CODES:
            idx = CODES.index(code)

            if board[idx] == ' ':
                return idx

            print('Whoops! You need to pick an empty space.')

        else:
            print("Hmm, that's not a valid input.")  

if turn >= 5:

What is this 5, and where did it come from?

Ok, in playing Naughts and Crosses, you win by getting 3 of your symbol in a row. Since you can only place one symbol per turn, and you take turns with another player, you can't possibly win before turn 5.

But do you really need to check that? The code would work perfectly without that check. The amount of work the check saves is minimal. More over, if you wanted to change this to a 3 player game, or play on a larger grid and get more than 3 in row, etc., the check would need to be changed.

Perhaps better is to remove the check altogether. It doesn't save enough work to call out as a special case.

Draw Game

Again, Naughts and Crosses is a game is played until someone wins, or until no moves are left. With 9 spaces to play in, there can be at most 9 turns. This leads to a different formulation of the "draw game" detection.

Instead of:

while True:
    ... omitted ...
    if check_for_win(board):
        print(f'{user} won in {turn} turns! Congratulations!')
        break
    elif '0' not in board:
        print("Aw, it's a draw!")
        break

you could use a for ... else loop:

for _ in range(9):
    ... omitted ...
    if check_for_win(board):
        print(f'{user} won in {turn} turns! Congratulations!')
        break
else:
    print("Aw, it's a draw!")

What is happening here?

If the for loop execution runs to completion, the else: clause at the end of the loop is executed. If the for loop execution is interrupt by a break, the loop terminates and the else: clause is not executed. This is pattern is usually used in a search, where you loop over something until you find the desired item, and if you don't find it you do something "else", but it works just as well here, where you play until a win, or there are no more moves.

The 9 isn't quite as magic as the 5 was, but we can still get rid of it. It is the number of moves which can be made, which is the size of the board. At the same time, we can absorb the turn variable into the for-loop.

for turn in range(1, len(board) + 1):
    ... omitted ...
    if check_for_win(board):
        print(f'{user} won in {turn} turns! Congratulations!')
        break
else:
    print("Aw, it's a draw!")

numpy, scipy, and bitwise operators

While you are hoping for optimizations utilizing numpy, scipy or bitwise operators, these are the wrong tools for the job.

I held my tongue in my previous review, but your code could be made much simpler without numpy.

First, consider what does the following condition do?

' ' not in row and len(set(row)) == 1

First, it ensures that the row doesn't contain any empty cells, and then it forms a set of the contents of the cells, removing duplicates, and if the row contained only 1 symbol (which is not the empty symbol), we have found a winning row.

That is a mouthful to describe. What is the winning condition for the game?

A row (or column or diagonal) with 3 of the same symbols.

That sounds way simpler. If you pass user to the check_for_win function, then the winning condition would be [user, user, user] found in a row (or column, or diagonal).

winning_pattern = [user] * 3
...
   if row == winning_pattern:
       return True
...

No expensive set() construction. No weird testing the length of the set.

And now to get rid of numpy. With 3 rows, 3 columns, and 2 diagonals, all we need are 8 slices of the board. And a slice is a first class object in Python:

ROWS = (slice(0, 3, 1), slice(3, 6, 1), slice(6, 9, 1),
        slice(0, 9, 3), slice(1, 10, 3), slice(2, 11, 3),
        slice(0, 12, 4), slice(3, 9, 2))

def check_for_win(board, user):

    winning_pattern = [user] * 3

    return any(board[row] == winning_pattern for row in ROWS)

Refactored Code

from random import randint


CODES = ('c1', 'c2', 'c3', 'b1', 'b2', 'b3', 'a1', 'a2', 'a3')

ROWS = (slice(0, 3, 1), slice(3, 6, 1), slice(6, 9, 1),
        slice(0, 9, 3), slice(1, 10, 3), slice(2, 11, 3),
        slice(0, 12, 4), slice(3, 9, 2))


def print_board(board):
    print("""
  +---+---+---+
c | {}   {}   {} |
  +   +   +   +
b | {}   {}   {} |
  +   +   +   +
a | {}   {}   {} |
  +---+---+---+
    1   2   3
""".format(*board))


def check_for_win(board, user):

    winning_pattern = [user] * 3

    return any(board[row] == winning_pattern for row in ROWS)


def get_player_move(board, user):

    print(f'Player {user}: ')

    while True:
        code = input().strip().lower()

        if code in CODES:
            idx = CODES.index(code)

            if board[idx] == ' ':
                return idx

            print('Whoops! You need to pick an empty space.')

        else:
            print("Hmm, that's not a valid input.")  


def tic_tac_toe():

    board = [' '] * 9
    user = 'X' if bool(randint(0, 1)) else 'O'

    print(f'Welcome! First to play is {user}!')
    print_board(board)

    for turn in range(1, len(board) + 1):
        idx = get_player_move(board, user)

        board[idx] = user
        print_board(board)

        if check_for_win(board, user):
            print(f'{user} won in {turn} turns! Congratulations!')
            break

        user = 'X' if user == 'O' else 'O'

    else:
        print("Aw, it's a draw!")


if __name__ == '__main__':
    tic_tac_toe()

Despite adding 2 more functions, the code is 9 lines shorter (ignoring blank lines).

\$\endgroup\$
4
  • 1
    \$\begingroup\$ "Assign turn and the first moving player." :p \$\endgroup\$ – T145 Nov 6 '19 at 2:56
  • 1
    \$\begingroup\$ My English teacher would be appalled, but “good job.” And your seven words for the second part of the question? \$\endgroup\$ – AJNeufeld Nov 6 '19 at 3:01
  • 1
    \$\begingroup\$ "All the variables get their corresponding assignments." ;) \$\endgroup\$ – T145 Nov 6 '19 at 3:05
  • 1
    \$\begingroup\$ “I think you’ve omitted several salient details.” \$\endgroup\$ – AJNeufeld Nov 6 '19 at 3:11
5
\$\begingroup\$

Ways to improve:

  • simplifying list construction:

    *list(x if x != '0' else ' ' for x in board)) --> *[x if x != '0' else ' ' for x in board]

  • check_diagonal function. The function performs a logical check if the passed row is fully selected/marked with the same mark/code and returns boolean result respectively.
    Therefore, it's better to give it a more meaningful and unified name, say row_crossed (reflecting boolean purpose):

    def row_crossed(row):
        return '0' not in row and len(set(row)) == 1
    

    Besides, renaming it will serve beneficially for the next improvements (see below)

  • 2 consecutive conditions:

    if check_diagonal(a.diagonal()):
        return True
    
    # check rl diagonal
    if check_diagonal(np.fliplr(a).diagonal()):
        return True
    

    return the same boolean result. That's a sign for Consolidate conditional expression technique.
    Thus, the flow becomes:

    # check lr diagonal
    if row_crossed(a.diagonal()) or row_crossed(np.fliplr(a).diagonal()):
        return True
    
  • the for loop (in check_for_win function) which iterates through pair of matrices (initial and transposed one) duplicates the same logical check

     ...
     if '0' not in row and len(set(row)) == 1:
    

    as row_crossed (formerly check_diagonal) function does. Thus, replacing duplicated condition with function call.
    Eventually, the restructured check_for_win function would look as below:

    def check_for_win(board):
        def row_crossed(row):
            return '0' not in row and len(set(row)) == 1
    
        a = np.array(board).reshape(3, 3)
    
        # check lr diagonal
        if row_crossed(a.diagonal()) or row_crossed(np.fliplr(a).diagonal()):
            return True
    
        # check rows & cols
        for matr in (a, a.transpose()):
            for row in matr:
                if row_crossed(row):
                    return True
    
        return False
    

Actually, the above Consolidation of conditionals can be even more encompassing with applying np.concatenate and builtin any functions, though may seem less readable but quite viable for those who likes concise/compact code:

def check_for_win(board):
    def row_crossed(row):
        return '0' not in row and len(set(row)) == 1

    a = np.array(board).reshape(3, 3)

    if row_crossed(a.diagonal()) \
            or row_crossed(np.fliplr(a).diagonal()) \
            or any(row_crossed(row) for row in np.concatenate([a, a.transpose()])):
        return True

    return False

Minor "refinements":

  • for matr in (a, a.transpose()) looks a bit more "connected" than for matr in (a, np.transpose(a)):
  • optionally, to reduce typing misses of codes = ('c1', 'c2', 'c3', 'b1', 'b2', 'b3', 'a1', 'a2', 'a3') - a good alternative is:

    codes = tuple(map(''.join, itertools.product('cba', '123')))
    
\$\endgroup\$

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.