4
\$\begingroup\$

I'm looking for feedback on the readability of my code and opinions on the pick_best algorithm. I do this stuff for fun when I have time, and never get a chance to have anyone else look at it. Hopefully, I can use this site to improve my coding.

from itertools import permutations 
from random import randint
class TTT:    
    def __init__(self):
        self.game_grid = [0 for x in range(9)]  
        self.winning_matrix = ((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 game_state(self, grid):
        '''Prints current game state'''
        print "\n\n\n0 1 2 \n3 4 5 \n6 7 8 \n"
        for square in range(9):
            if square%3==0: print
            print ['.', 'O', 'X'][grid[square]],
        print

    def check_game(self, grid):
        '''Checks game state for a winner or tie'''
        for row in self.winning_matrix:
            total = sum([grid[x] for x in row])
            if abs(total) == 3: 
                return self.winner_is(total)
        return self.winner_is(total)

    def winner_is(self, total):
        '''Establishes which player wins in current node'''
        return total/3 if abs(total)==3 else 0

    def pick_best(self):
        '''A non-recursive minimax style function utilizing itertools.permutations'''
        avail_squares = [pos for pos, x in enumerate(self.game_grid) if x==0] 
        score = {x:0 for x in avail_squares}    #keeps minimax score for current game state  
        for perm in permutations(avail_squares): #thank you itertools
            player = -1 #start with computer player
            node = list(self.game_grid) #node is a copy of the current game state
            for square in perm:
                node[square]=player #makes a move in the permutation
                state_score = self.check_game(node) #checks to see if anyone won
                if state_score: 
                    ''' if there is a winning play in this node,
                        a score based off the depth of the play is added
                        to the total score of the possible move.
                        Then we break out of the current permutation'''
                    score[perm[0]] += (10**(len(perm)-perm.index(square)))*-1*state_score 
                    break 
                player *= -1 #swaps to the next player in the node
        return max(score, key=lambda x: score[x]) #returns square with highest score      

    def player_choice(self):
        start = raw_input("Would you like to start? (Y/N) ")
        print start
        if start.upper() == 'Y':
            return False, 1
        elif start.upper() == 'N':
            return False, -1
        else:
            print "Please choose (Y)es or (N)o"
            return True, 0

    def pick_a_spot(self):
        move = raw_input("Where would you like to move 0-8? ")
        if move.isdigit() and 0<=int(move)<9:
            m = int(move)
            if self.game_grid[m] == 0:
                return False, m
        return True, 9


    def play(self):
        '''A very basic game play interface to test algorithm.'''
        invalid = True
        while invalid:
            invalid, player = self.player_choice()
        if player == -1: 
            self.game_grid[randint(0,8)]=-1
            player = 1
        self.game_state(self.game_grid)
        while 0 in self.game_grid and self.check_game(self.game_grid)==0:
            if player == 1:
                invalid = True
                while invalid:
                    invalid, move = self.pick_a_spot()
                self.game_grid[move]=1
                player = -1
            else:
                move = self.pick_best()
                player = 1
                self.game_grid[move]=-1
            self.game_state(self.game_grid)


if __name__=='__main__':
    TTT().play()
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

TTT is not a clear name, TicTacToe is only 6 characters more, and much clearer.

You should have winning_matrix be a constant, defined just inside the class declaration:

class TicTacToe:
    WINNING_MATRIX = ((0,1,2), (3,4,5), (6,7,8), (0,3,6), (1,4,7), (2,5,8), (0,4,8), (2,4,6))

Indexing a literal is odd to see, you should declare the constant ICONS and index that.

ICONS = ['.', 'O', 'X']

...

print ICONS[grid[square]],

I personally prefer using print('\n') to make it more clear what's happening as it can look funny to see just a print. You also shouldn't use an inline if as they're very easy to misread.

if square % 3 == 0: print
print ['.', 'O', 'X'][grid[square]],

The above looks like the if is meant to apply to the print call beneath it, but has an indentation error. Even when you spot the print, it seems like that was put there accidentally because it doesn't seem to print anything.

if square % 3 == 0:
    print('\n')
print(ICONS[grid[square]])

Also, instead of iterating over numbers and using those to index you should iterate over grid itself. You need the index of the values to detect when to print a new line, but you can still do this using enumerate.

for index, square in enumerate(grid):
    if index % 3 == 0:
        print('\n')
    print(ICONS[square])

This is much simpler than a double index. Plus, if you were to change the size of grid for any reason, you'd then be able to handle it just fine. The one last change, particularly in that case, could be to replace the 3 with a constant like ROW_WIDTH.

You could also make functions a lot clearer, like this:

def winner_is(self, total):
    '''Establishes which player wins in current node'''
    return total/3 if abs(total)==3 else 0

Your docstring is vague, and more abstract than it should be. Tell me what the function does, not what it's for.

def winner_is(self, total):
    '''Returns -1 or 1 to indicate which player won, or 0 for a tie.'''

However what's odd is that you're doubling down on your check for if the total is 3? You're calling self.winner_is when you've already determined that the total is three, or when it's definitely a tie. I'd combine the functions into one:

def check_game(self, grid):
    '''Returns -1 or 1 to indicate which player won, or 0 for a tie.'''

    for row in self.winning_matrix:
        total = sum([grid[x] for x in row])
        if abs(total) == 3: 
            return total / 3
    return 0

Now it's much simpler and contained, but still does exactly the same. I'd also consider renaming this, as it's unclear what a check would do, or what aspect of the game it applies to. eval_winner could improve the clarity.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the feedback. You're completely right about the winner_is() function being useless. I'm not sure how that got past my refactoring. I thought indexing the literal [',','O','X'] was pretty cool but I like your suggestion better. Same with making the list winning_matrix a constant. I never would have come up with that one on my own. Any thoughts on the function pick_best()? \$\endgroup\$ Commented Jun 10, 2016 at 11:37

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.