2
\$\begingroup\$

In the below GameOutcome class I'm checking for wins/ties in Tic Tac Toe. I'm pretty set on breaking the checks into 5 different functions because I'm specifically practicing making functions short and single-purposed. But I'm not at all set on my 5 if statements in the find_winner_or_tie method. Does somebody know a more elegant solution for the find_winner_or_tie method so it doesn't have 5 if statements in it? I sort of which I could just call each of the "check functions" in a loop but I don't know how to do this without hardcoding the names of all 5 of the "check functions".

class GameOutcome:

    def __init__(self):
        self.letter_dict = {'X': -1, 'O': 1, ' ': 0}
        self.state_of_game = None
        self.row_index_of_move = None
        self.column_index_of_move = None
        self.game_outcome = None

    def find_winner_or_tie(self, state_of_game, row_index_of_move, column_index_of_move):

        self.set_board_and_move(state_of_game, row_index_of_move, column_index_of_move)

        if self.check_row_containing_move_for_win():
            return self.game_outcome
        if self.check_column_containing_move_for_win():
            return self.game_outcome
        if self.check_main_diagonal_for_win_iff_it_contains_move():
            return self.game_outcome
        if self.check_off_diagonal_for_win_iff_it_contains_move():
            return self.game_outcome
        if self.check_for_tie():
            return self.game_outcome

        return ''

    def set_board_and_move(self, state_of_game, row_index_of_move, column_index_of_move):
        self.state_of_game = state_of_game
        self.row_index_of_move = row_index_of_move
        self.column_index_of_move = column_index_of_move

    def check_row_containing_move_for_win(self):
        total = 0
        for column in range(3):
            total = total + int(self.letter_dict[self.state_of_game.board[self.row_index_of_move][column]])
            if abs(total) == 3:
                winning_letter = self.state_of_game.board[self.row_index_of_move][self.column_index_of_move]
                self.game_outcome = winning_letter
                return True
        return False

    def check_column_containing_move_for_win(self):
        total = 0
        for row in range(3):
            total = total + int(self.letter_dict[self.state_of_game.board[row][self.column_index_of_move]])
            if abs(total) == 3:
                winning_letter = self.state_of_game.board[self.row_index_of_move][self.column_index_of_move]
                self.game_outcome = winning_letter
                return True
        return False

    def check_main_diagonal_for_win_iff_it_contains_move(self):
        if self.row_index_of_move == self.column_index_of_move:
            total = 0
            for diagonal_indexing in range(3):
                total = total + int(self.letter_dict[self.state_of_game.board[diagonal_indexing][diagonal_indexing]])
                if abs(total) == 3:
                    winning_letter = self.state_of_game.board[self.row_index_of_move][self.column_index_of_move]
                    self.game_outcome = winning_letter
                    return True
        return False

    def check_off_diagonal_for_win_iff_it_contains_move(self):
        if self.row_index_of_move + self.column_index_of_move == 2:
            total = 0
            for off_diagonal_indexing in range(3):
                total = total + int(self.letter_dict[self.state_of_game.board[off_diagonal_indexing][2 - off_diagonal_indexing]])
                if abs(total) == 3:
                    winning_letter = self.state_of_game.board[self.row_index_of_move][self.column_index_of_move]
                    self.game_outcome = winning_letter
                    return True
        return False

    def check_for_tie(self):
        if len(self.state_of_game.available_squares) == 0:
            self.game_outcome = 'Tie'
            return True
        return False
\$\endgroup\$
1
\$\begingroup\$
def check_row_containing_move_for_win(self):
    total = 0
    for column in range(3):
        total = total + int(self.letter_dict[self.state_of_game.board[self.row_index_of_move][column]])
        if abs(total) == 3:
            winning_letter = self.state_of_game.board[self.row_index_of_move][self.column_index_of_move]
            self.game_outcome = winning_letter
            return True
    return False

There are a few things wrong with the above code, and a few things which can be greatly improved.

  • self.letter_dict[ ] is a dictionary containing the values 1, -1, and 0. Since these are already integer values, you don’t need the int( ... ) call to convert the values to integers.
  • total = total + ... is almost always written total += ...
  • Since total is initialized to 0, and can only be incremented or decremented by 1 each time through the loop, you can only get to +3 or -3 after all 3 passes through the loop. Having the if abs(total) == 3: test inside the loop is inefficient; it should be moved outside of, just after the loop.
  • Since you are summing up values in a loop, you can use the Python sum( ) function, with list comprehension instead of a loop.

With the above changes, this function could become:

def check_row_containing_move_for_win(self):
    total = 0
    for column in range(3):
        total += self.letter_dict[self.state_of_game.board[self.row_index_of_move][column]]
    if abs(total) == 3:
        winning_letter = self.state_of_game.board[self.row_index_of_move][self.column_index_of_move]
        self.game_outcome = winning_letter
        return True
    return False

Or simply:

def check_row_containing_move_for_win(self):
    total = sum(self.letter_dict[self.state_of_game.board[self.row_index_of_move][column]]
                for column in range(3))
    if abs(total) == 3:
        winning_letter = self.state_of_game.board[self.row_index_of_move][self.column_index_of_move]
        self.game_outcome = winning_letter
        return True
    return False

Similar comments for your column and diagonal win tests.


Your 5 if statements ...

    if self.check_row_containing_move_for_win():
        return self.game_outcome
    if self.check_column_containing_move_for_win():
        return self.game_outcome
    if self.check_main_diagonal_for_win_iff_it_contains_move():
        return self.game_outcome
    if self.check_off_diagonal_for_win_iff_it_contains_move():
        return self.game_outcome
    if self.check_for_tie():
        return self.game_outcome

can be simplified by combining into a single if statement:

    if (  self.check_row_containing_move_for_win() or
          self.check_column_containing_move_for_win() or
          self.check_main_diagonal_for_win_iff_it_contains_move() or
          self.check_off_diagonal_for_win_iff_it_contains_move() or           
          self.check_for_tie()):
        return self.game_outcome
\$\endgroup\$
  • \$\begingroup\$ These changes all look good. Since you didn't mention the find_winner_or_tie method, does this mean that the five if statements in that method are the best way to call each of the functions that you helped cleanup? (I got it in my head that when a function has a ton of if statements then it isn't as clean as it could be). \$\endgroup\$ – okcapp Feb 20 at 22:50
  • \$\begingroup\$ I just noticed your edit and accepted the answer... that singe if statement with the or between each call works perfectly. \$\endgroup\$ – okcapp Feb 27 at 13:22
4
\$\begingroup\$

(Note: no code in this post has been run- beware errors.)

I have a couple of issues with your design, and with your review request.

First, you say that you are committed to using five different functions for the checking. That's unfortunate, since this problem is very amenable to being solved with iteration.

Next, you have a class GameOutcome that is being asked to identify a winner or tie. I disagree with this- this is something the game itself should do. The game outcome, IMO, would be a data class just collecting results. The name GameOutcome suggest that this class would have no knowledge of the sequence of moves, or the state of the game board.

With those out of the way, let's talk about your code.

Those names!

I hate your naming style. You have fallen into the trap of making excessively long names for no good purpose. I strongly suggest you review code written by other people (either here on CodeReview or look through github or the pypi repositories) to get a sense of how names should be chosen.

Consider this line:

    if self.check_row_containing_move_for_win():

The method check_row_containing_move_for_win is an internal method. It is not intended for use by arbitrary external callers, it is only a method in order to package up its code behind a name.

For an internal function, why not just call it _check_row.

Similarly, you have an externally callable method named find_winner_or_tie. The declaration:

def find_winner_or_tie(self, state_of_game, row_index_of_move, column_index_of_move):

Here's a better approach:

def find_winner_or_tie(self, state, row, col):
    """ Check if either player has won, or if a tie has been forced. Return 'X' or 'O' 
        for a win, 'Tie' for a tie, and None when the game is not over.

        Parameters:
          state:  state of the game
          row:    row of the last move
          col:    column of the last move
    """

Providing in-depth explanation is the function of comments, not names. Your names should be as long as they need to be, but no longer.

Builtins any() and all()

You are writing code that checks for various conditions. Python supplies the any() and all() built-ins for situations like this. The trick is to learn to construct iterables in line, and that means using generator expressions.

A generator expression is an inside-out for loop in parentheses, like:

(abs(x) for x in somelist)

They resemble, and are related to, list, dictionary, and set comprehensions. In all cases there is an iteration keyword (for ... in ...) and some kind of leading expression. With a generator expression, you are constructing a generator in-line and storing it, evaluating it, or passing it as parameters.

How does this help you? Well, suppose you have a group of functions you want to evaluate for success. Say ... five of them? You might decide that you were going to return if any of those conditions were true. Well ...

def find_winner_or_tie(self, state, row, column):
    """ ... """
    self.set_board_and_move(state, row, column)

    checks = (self.check_row, self.check_column, self.check_diag_lr, self.check_diag_rl, self.check_tie)

    if any(check() for check in checks):   # If any check is true, we know the outcome
        return self.game_outcome

    return None

Less code, more data!

The thing about most games is that they are based on lots of data. And if you write your code correctly, you will find more and more data, with less and less code. That's the case here. Let's take a look at one of your check-functions:

def check_row_containing_move_for_win(self):
    total = 0
    for column in range(3):
        total = total + int(self.letter_dict[self.state_of_game.board[self.row_index_of_move][column]])
        if abs(total) == 3:
            winning_letter = self.state_of_game.board[self.row_index_of_move][self.column_index_of_move]
            self.game_outcome = winning_letter
            return True
    return False

Honestly, that's so long that it's unreadable. So let's refactor out the names:

def check_row(self):
    """Check row containing latest move for a win."""
    bias = self.letter_dict
    board = self.state_of_game.board
    row = self.row_index_of_move

    total = 0
    for column in range(3):
        total = total + bias[board[row][column]]

    if abs(total) == 3:
        self.game_outcome = board[row][0]
        return True
    return False

I think some people have already pointed out sum and some other options. But let's actually spell out what the row and column choices are:

ROWS = [ ((0, 0), (0, 1), (0, 2)),
         ((1, 0), (1, 1), (1, 2)),
         ((2, 0), (2, 1), (2, 2)),
]

Now, you could write code to generate that. In one line, even! But that's not important. What's important is that we know that there's a ROWS variable that can be indexed by row-number and which has a sequence of (row, col) tuples identifying the cells to look at.

Now, how does that help? Well, it helps because you can write a helper function that does the checking for you given a sequence of cells. Like this:

def check_cells(self, indices):
    """ Return True if all gameboard[][] cells specified by indices have same value,
        else False.
    """
    board = self.state_of_game.board
    values = [board[row][col] for row,col in indices]
    v0 = values[0]
    return all(v == v0 for v in values)

Then you can write the check_ functions in terms of that:

def check_row(self):
    """Check row containing latest move for a win."""
    indices = ROWS[self.row_index_of_move]
    return self.check_cells(indices)

You'll have to put in some validation on the diagonals, so you only call the check_cells if the move really was on a diagonal. The key here is that this uses the check_ functions to provide more data to the underlying check_cells, and so the bottom level code where the work is getting done is mainly data-driven.

Extra credit: detect forced ties

Your tie-detector only triggers at the last minute. But there are plenty of circumstances where a tie is inevitable. Why not code your tie-checker to recognize them?

\$\endgroup\$
  • \$\begingroup\$ You have almost repeated my answer (: \$\endgroup\$ – outoftime Feb 21 at 0:17
  • \$\begingroup\$ @outoftime "Great minds think alike!" \$\endgroup\$ – Austin Hastings Feb 21 at 1:16
  • \$\begingroup\$ your check_cells can be even shorter if you use a set instead of a list: return len({board[row][col] for row, col in indices}) == 1 \$\endgroup\$ – Maarten Fabré Feb 21 at 11:23
  • \$\begingroup\$ @AJNeufeld according to the program, it's only called on the row containing a recent move. So the row should never be all blanks. \$\endgroup\$ – Austin Hastings Feb 22 at 1:08
2
\$\begingroup\$

Some point regarding the game

  • The game has 9 positions.
  • The order of moves does not affect the game result.
  • Player may not move over another.
  • There are 8 possible winning combinations of moves.
  • The game is symmetrically invariant so column, row need only be relative, and does not need to match actual board positions.

That means you can encode all the moves for both players in two 9 bit ints

self.moves = {"X" : 0, "O" : 0}

def playerMove(self, player, row, col):
    self.moves[player] |= 1 << (row * 3 + col)

You can use a mask to check for a winning move

self.wins = [7,56,448,273,84,292,146,73]  

def isWin(self, player):
    for win in self.wins:
        if (self.moves[player] & win) == win:
            self.game_outcome = player
            return True
    return False

You can check for a draw by or'ing (Same as adding because there is no overlap) both players moves

def isDraw(self):
    if (self.moves["X"] + self.moves["O"]) == 511:
        self.game_outcome = 'Tie'
        return True
    return False

To reset the game

def reset(self):
    self.moves["X"] = 0
    self.moves["O"] = 0

Thus the whole thing becomes

class GameOutcome:
    def __init__(self):
        self.moves = {'X' : 0, 'O' : 0}
        self.game_state = ''
        self.wins = [7, 56, 448, 273, 84, 292, 146, 73]  # each has 3bits set
        # bits 0 to 8 in order of above array
        # 000000111,000111000,111000000,100010001,001010100,100100100,010010010,001001001
        # fold them into a 3by3 to get the following patterns
        # 000  000  111  100  001  100  010  001
        # 000  111  000  010  010  100  010  001
        # 111, 000, 000, 001, 100, 100, 010, 001

    def get_game_state(self):
        if self.is_win('X') or self.is_win('O'):
            return self.game_state
        self.is_draw()
        return self.game_state

    def player_move(self, player, row, col):
        self.moves[player] |= 1 << (row * 3 + col)
        return None

    def is_win(self, player):
        for win in self.wins:
            if (self.moves[player] & win) == win:
                self.game_state = player
                return True
        return False

    def is_draw(self):
        if (self.moves["X"] + self.moves["O"]) == 511:
            self.game_state = 'Tie'
            return True
        return False

    def reset(self):
        self.moves["X"] = 0
        self.moves["O"] = 0
        self.game_state = ''
        return None
\$\endgroup\$
  • \$\begingroup\$ I have never worked with binary in my code before (I have yet to start my first software engineering position professionally). I can see your binary representations of winning boards and the conversions to decimal. Question: I'm reading "Code Complete" and "Clean Code" and they both go on and on about the importance of making code easily readable on the job. Do you know if using this lower level logic would be considered not easily readable? If yes, would you save it for cases where the increased efficiency would make a very large difference? \$\endgroup\$ – okcapp Feb 21 at 10:14
  • \$\begingroup\$ I don't like your representation of the winning codes. Why not use binary integer literals like 0b000_000_111 instead of 7, and a set would be a better data structure then a list \$\endgroup\$ – Maarten Fabré Feb 21 at 11:25
  • \$\begingroup\$ @MaartenFabré The list is "static" (in its use) and does not need protection from item duplication. For performance the list beats a set when iterating and that is its only use. Because many newbies don't understand that 0b10101010 and 170 are identical items. I wished to highlight that it is a list of ints not a list of (magic) binary numbers, hence the comment below it. \$\endgroup\$ – Blindman67 Feb 21 at 20:00
  • \$\begingroup\$ You're correct on the list, i read wrongly you used it to check whether a combination was in the win combination. Whether a comment or the binary representation USD Mort clear is open for debate \$\endgroup\$ – Maarten Fabré Feb 21 at 20:08
1
\$\begingroup\$

.. I'm pretty set on breaking the checks into 5 different functions because I'm specifically practicing making functions short and single-purposed ...

I can not agree that you can avoid sanity during study. If you do not need 5 functions - you should not use them. If you want to practice in making functions - find corresponding usage for it that will make sense.

.. But I'm not at all set on my 5 if statements in the find_winner_or_tie method. Does somebody know a more elegant solution for the find_winner_or_tie method so it doesn't have 5 if statements in it? ..

You have over complicated everything.

self.letter_dict

looks like it have to be used for presentation (in the view if we talk about MVC). You have no need to use it at all and can get rid of it simply.

self.row_index_of_move
self.column_index_of_move

again, it can help during presentation. During computation you have all data in the arguments so you can pass it deeper and didn't store useless data.

self.game_outcome

yet another class property you are using just to transfer data, in this case back to caller. Use return statement instead and return tuple of 2 objects if you needed.

.. Does somebody know a more elegant solution for the find_winner_or_tie method ..

You can try to think in terms of patterns and ask yourself "what presentation of the pattern will cover all cases at once?". I suggest that list of coordinates can describe any win case. It's creation will have same loops as you already have but they will be used for patterns generation.

patterns = []
for i in range(3):
    patterns.append( tuple(itertools.product(range(3), [i])) )
    patterns.append( tuple(itertools.product([i], range(3))) )
patterns.append( zip(range(3), range(3)) )
patterns.append( zip(range(3), range(2, -1, -1)) )

You can generate patterns during initialization and this is the only think I'd like to see as the field of GameOutcome class. Main purpose - caching, so readonly property will be awesome.

Function check_... will simply check every pattern, if all symbols are the same - victorious and return symbol from any pattern coordinates.

for pattern in patterns:
    a, b, c = [self.state_of_game.board[row][col] for row, col in pattern]
    if a == b == c != ' ':
        return (True, a)
return (False, None)
\$\endgroup\$
  • 1
    \$\begingroup\$ a == b and b == c can be written a == b == c. In either case, it repeats the error of detecting 3-blanks-in-a-row. You really want a == b == c != ' '. \$\endgroup\$ – AJNeufeld Feb 21 at 3:44

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.