This is a completely functional python script of the Tic-Tac-Toe game. I've used python's set container for almost all data structure and operation. The code is small and somewhat readable. I would very much appreciate your opinion and suggestions to improve it in terms of speed, readability and refactoring wherever necessary. Please post in your answer any modifications needed in the algorithm used.

  1. The grid is marked from 1 to 9 horizontally.
  2. The TTT class handles all the tasks.
  3. self.ploys is a dictionary containing all the solution combinations.

    {1: [{1,2,3}, {1,4,7}, {1,5,9}], 2:[{1,2,3}, {2,5,8}], ...}
  4. The Main Loop displays the output, cycles through each player's turn and checks the result.

  5. self.PlayUser and self.PlayCom plays respective turns.

#!/usr/bin/env python3

import os

erase = 'cls' if os.name == 'nt' else 'clear'
# 3x3 grid and 8 solution sets
grid = set(range(1,10))
sols = ({1,2,3}, {4,5,6},
        {7,8,9}, {1,4,7},
        {2,5,8}, {3,6,9},
        {1,5,9}, {3,5,7})

class TTT(object):

    def __init__(self):
        # Valid combinations (shortens during game)
        self.ploys = {k:[s for s in sols if k in s] for k in grid}
        # Holds two sets; user moves and com moves
        self.umoves = set()
        self.cmoves = set()

    def GameOver(self, moves):
        """Returns True if the puzzle is solved, False if it's a tie."""
        if any(s.issubset(moves) for s in sols):
            return True
        elif (self.umoves|self.cmoves) == grid:
            return False

    def PlayUser(self):
        """Plays the user's turn"""
        umove = int(input())
        # Filter invalid moves
        while umove not in grid or (umove in self.umoves or
            umove in self.cmoves):
            umove = int(input("Invalid Move "))
        # Modify ploys
        del self.ploys[umove]
        for p, s in self.ploys.items():
            for i in [i for i,n in enumerate(s) if umove in n]:
                del self.ploys[p][i]

    def PlayCom(self, cmove=None, best=None):
        """Plays the com's turn"""
        for p, s in self.ploys.items():
            # Check if com can win
            if any(n.issubset({p}|self.cmoves) for n in s):
                cmove = p
            # Check if user can win
            elif any(n.issubset({p}|self.umoves) for n in sols if p in n):
                cmove = p
            # Get the next best move
            elif best is None or len(s) > len(self.ploys[best]):
                best = p
        cmove = cmove or best
        del self.ploys[cmove]

    def Show(self, cols=3, com="X", usr="O", fill=" "):
        """Prints the grid in the command-line."""
        for g in grid:
            if g in self.cmoves:
                print(com, fill, end="") if g % cols else print(com)
            elif g in self.umoves:
                print(usr, fill, end="") if g % cols else print(usr)
                # Replace g with fill for clean grid
                print(g, fill, end="") if g % cols else print(g)

    def Main(self):
        """Stars the game."""
        from itertools import cycle

        text = ("You Won", "You Lost")
        move = (self.umoves, self.cmoves)
        play = (self.PlayUser, self.PlayCom)

        for t, m, p in cycle(zip(text,move,play)):
            status = self.GameOver(m)
            if status is not None:
                print(t) if status is True else print("Game Over.")

if __name__ == "__main__":
    game = TTT()

The logic behind this section seems to be a little off.

for p, s in self.ploys.items():
    for i in [i for i,n in enumerate(s) if umove in n]:
        del self.ploys[p][i]

When you delete the value at self.ploys[p][i] this essentially means that:

self.ploys[p][i + 1] is now at position self.ploys[p][i]
self.ploys[p][i + 2] is now at position self.ploys[p][i + 1]

Because the comprehension in the second line finds the indices to the original array, all the proceeding deletes will delete the element 1 index ahead of their target, the next delete will be 2 ahead, and so on.

To fix this you can just iterate through the loop backwards.

for p, s in self.ploys.items():
    for i in [i for i,n in enumerate(s) if umove in n][::-1]:
        del self.ploys[p][i]

Along with that, I think it would help readability to give variables in your loop meaningful names.

It's much less cryptic as:

for ploy_index, soln_list in self.ploys.items():
    for soln_index in [i for i, soln in enumerate(soln_list)
                      if umove in soln][::-1]:
        del self.ploys[ploy_index][soln_index]

I notice is that in PlayUser you write the following.

while umove not in grid or (umove in self.umoves or
    umove in self.cmoves):

These brackets are a little confusing. When line length is a concern, you should put the entire expression in brackets or use the \ continuation. And although not explicitly in PEP8, I think it also looks better to add extra indentation to easily identify the condition from the logic.

while (umove not in grid or 
       umove in self.umoves or
       umove in self.cmoves):
  • \$\begingroup\$ The list comprehension of indices is created before iterating over it. All of these indices are deleted at once without looping on the actual container, so I think this is not a case of "deleting while looping" mistake. \$\endgroup\$ Sep 15 '14 at 20:30
  • \$\begingroup\$ @user3058846 I believe Calpratt is right. Deleting an item from a list moves the following items back. You should loop backwards when deleting multiple items by index. \$\endgroup\$ Sep 16 '14 at 8:57
  • \$\begingroup\$ @JanneKarila Then I'm puzzled as to why the code works without it. \$\endgroup\$ Sep 16 '14 at 14:16
  • \$\begingroup\$ @user3058846 Does the list comprehension ever return more than one item? If not you're good, but the fact is not clear to someone looking at the function alone. \$\endgroup\$ Sep 17 '14 at 15:23

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.