Python tic-tac-toe AI

Question

I just finished my Tic-tac-toe AI, and would be very grateful for opinions and comments on that project. I would like to add it to my 'entry level Python developer portfolio'.

README:

Simple AI that plays tic-tac-toe with itself. At the beginning it choses moves to play randomly, every possible move has the same probability of being played. Sequences of moves that lead to victory get points, thus there will be a higher chance of choosing those moves in the future games. Accordingly moves that ended in a losing state will be chosen less frequently. After about 200 000 games there will be more than 99% of draws - both players will not make single mistake in more than 99% of games.

Main Class:

import rules

#main game loop. Number of repetition defines how many games are played

for i in range (1):

    #these two lists are used to store moves used in each game
    moves_X = []
    moves_O = []

    #creation of an empty board, flags move_X and move_O are used to define whether it is Xs or Os move at the moment
    board = [{'number': 100000, 'board': [' ', ' ', ' ', ' ', ' ', ' ', ' ', ' ', ' '], 'value': 100}]
    move_X = True
    move_O = False
    #change flag is the cap that prevents the game to be played for longher than 9 moves
    change = 0

    #this loop goes on until Os or Xs wins or it is a draw
    while True:


        if move_X and change <= 4:

            #to define new board after Xs move we use function called draw() defined in the rules module
            board = rules.draw(0, change, board)
            moves_X.append(board)

            #to visualize the game we use draw_board() function defined in rules module
            print(rules.draw_board(board))



            move_X = False
            move_O = True

            #using iswin function from rules() we check if Xs have alredy won
            if (rules.iswin(board[0]['board'], 'X')):

                print(rules.draw_board(board))
                print('X won!')



                #if Xs won we add points to all of Xs moves from that game
                #and take points from Os moves
                for move in moves_X:
                    move[0]['value'] += 1

                for move in moves_O:
                    if move[0]['value'] > 1:
                        move[0]['value'] -= 1

                break

        #next O play its move
        elif move_O and change<4:
            board = rules.draw(1, change, board)

            print(rules.draw_board(board))

            moves_O.append(board)
            move_X = True
            move_O = False

            change += 1
            if (rules.iswin(board[0]['board'], 'O')):

                print(rules.draw_board(board))
                print('O won!')



                for move in moves_O:
                        move[0]['value'] += 1

                for move in moves_X:
                    if move[0]['value'] > 1:
                        move[0]['value'] -= 1

                break

        #if the board is full and neither of the players won
        else:
            print('Draw!')
            break


#after compliton of the main game loop we save the results
rules.save()

all_data Class:

import itertools

#used to create list of all combinations of ' ', X and O. Each combination have its number and value
#that list will be filtered and sorted in the data_base module

def board_init():

    move_number = []

    for b,i in enumerate(itertools.product(' XO', repeat=9)):

        if b==100000:
            break
        else:
            move_n_number = {'number': b, 'board': i, 'value': 100}
            move_number.append(move_n_number)

    return move_number

data_base Class:

import all_data
import json

#board_sort module sorts and filters list created with board_init() in the all_data module

def board_sort(move_n):

    #move_list_X and move_list_O are used to sort moves by sign and number, from first to fifth in Xs and fourth in Os.
    move_list_X=[[], [], [], [], []]
    move_list_O=[[], [], [], []]

    move_list = (move_list_X, move_list_O)



    for i in move_n:
        if i.get('board').count('X')==1 and i.get('board').count('O')==0:
            move_list_X[0].append(i)


        elif i.get('board').count('X')==1 and i.get('board').count('O')==1:
            move_list_O[0].append(i)

        elif i.get('board').count('X')==2 and i.get('board').count('O')==1:
            move_list_X[1].append(i)

        elif i.get('board').count('X')==2 and i.get('board').count('O')==2:
            move_list_O[1].append(i)

        elif i.get('board').count('X')==3 and i.get('board').count('O')==2:
            move_list_X[2].append(i)

        elif i.get('board').count('X')==3 and i.get('board').count('O')==3:
            move_list_O[2].append(i)

        elif i.get('board').count('X')==4 and i.get('board').count('O')==3:
            move_list_X[3].append(i)

        elif i.get('board').count('X')==4 and i.get('board').count('O')==4:
            move_list_O[3].append(i)

        elif i.get('board').count('X')==5 and i.get('board').count('O')==4:
            move_list_X[4].append(i)

        else:
            continue

    return move_list

def save(move_list):
    with open('all_moves.txt', 'w') as f:
        json.dump(move_list, f)

rules Class:

import random
import json
import all_data
import data_base



#if there is no file 'all_moves.txt' it is is created 
try:

    with open('all_moves.txt') as json_file:
        move_list = json.load(json_file)

except IOError:
    move_n = all_data.board_init()
    move_list = data_base.board_sort(move_n)



#iswin function is used to check if Xs or Os have alredy won

def iswin(board, letter='X'):

    if board[0]==letter and board[1]==letter and board[2]==letter:
        return True
    elif board[3]==letter and  board[4]==letter and board[5]==letter:
        return True
    elif board[6] == letter and board[7] == letter and board[8] == letter:
        return True
    elif board[0] == letter and board[3] == letter and board[6] == letter:
        return True
    elif board[1] == letter and board[4] == letter and board[7] == letter:
        return True
    elif board[2] == letter and board[5] == letter and board[8] == letter:
        return True
    elif board[0] == letter and board[4] == letter and board[8] == letter:
        return True
    elif board[6] == letter and board[4] == letter and board[2] == letter:
        return True
    else:
        return False





#draw function is used to draw a move from a list of possible moves
#the probability of each choice is dependent on previous results

def draw(letter, number, board):
    possible_moves=[]
    move = move_list[letter][number]

    for i in move:

        sum=0

        for z, g in zip(i['board'], board[0]['board']):

            if z==g:
                continue
            else:
                sum += 1
        if sum==1:
            possible_moves.append(i)
        else:
            continue

    return(random.choices(possible_moves, weights=[i['value'] for i in possible_moves], k = 1))



#saves changes made in the 'all_moves.txt' file

def save():
    with open('all_moves.txt', 'w') as f:
        json.dump(move_list, f)

#function used to visualize present state of the board

def draw_board(board):
    return(
    '''
    {}
    {}
    {}
    '''.format(board[0]['board'][:3], board[0]['board'][3:6], board[0]['board'][6:])
    )

Github link

Answer 1

Thanks Jacek for your post. Let's make a quick summary of the most obvious issues and we'll go over them:

• number of files
• the functions in certain files belong in other files due to their naming
• clear dependencies for functions in other files
• redundant comments
• code formatting (empty lines, not using PEP8, using doc-string for multi-line print, not using f-strings)
• many functions are too long and can be refactored
• use of break and continue (spaghetti code)
• no entry point, the script runs as soon as it's imported
• lots of hard-coding of strings, numbers, structure indexes, etc.

Number of files

I can understand breaking up the files, but some of them only have one or two functions. If you were writing the operating system for the Space Shuttle, there is a good reason to do so, but the domain problem is playing tic-tac-toe - not enough to warrant splitting them so. This splitting leads into the next issue:

Function Dependencies

As described above, each file needed to import a few functions from other files, which means each file is dependent on another in the same domain.

Let me describe the loading, hopefully you see why it's unnecessary:

run main -> opens rules -> loads random, json, all_data -> 
opens all_data -> loads intertools -> returns to rules -> 
opens data_base -> opens all_data -> loads itertools/cached -> 
returns to data_base -> loads json/cached -> returns to rules -> 
loads data_base/cached.

In large projects, files are split along modules/activities, losing a file/folder would break specific functionality, but the program "could" still run (i.e. "module not found, cannot access accounting function"). Everything, however, in your tic-tac-toe project is required and needed to run. Doesn't make sense to split them up.

There is also an obvious case of duplicating the save() function. You have the exact same code in 2 different files. This would be a clear bug if everything was in a single file.

Function Naming

When other programmers look at your code, they don't know/understand the coded solution, but will, through how you express the intent of variables by their name.

Part of proper naming of variables is explaining to the reader what your function does. In all_data, the board_init() function name is noun-verb. initialise_playing_board() is verb-noun, longer, and so descriptive that it leaves no other interpretation to what it does.
However, looking into the comment at the top of all_data, it doesn't do that at all. It builds 100,000 variants of possible combinations of tic-tac-toe board layouts. So board_init() doesn't initialise a board at all, and all_data isn't data, but derived possibilities of a tic-tac-toe board.

Does that explain the point? I've had to read your code, line by line, to understand the variables aren't the truth of the action. Which leads into the next section:

Redundant Comments

The comment at the top of all_data explained what the code does. Then I read the code and saw that the comment matched. So one of these two needs to be deleted, which should it be? Obviously the comment - otherwise the code wouldn't exist anymore.

Comments are used to explain the "why" of a function. If you had a function that did a very-specific formatting but appears it is never run, you'd go and ask a team member "why does this function exist?" Perhaps they know, perhaps they don't, so you ask another colleague and so on, until you find someone that says "Oh yeah, we had a client that wanted their report formatted that way." Do we have that client anymore? "No, they closed a few years ago." There you go, you can delete that code and push your changes to the code repository.
Why did I give you that example? Well, if there was a function comment """For Client X-Y-Z report format""" (in this example), then you could have searched the customer database to see if that client exists, learning the truth that it's time to delete that function.
Delete the code??? Shouldn't you keep it? Well, no, that's what the code repository is for, all the historical code changes are tracked there.
Comments lie. Comments cause bugs. Over time, the code changes, so what was the "how" 2 years ago, might not be the "how" today. If you get a new and eager-beaver developer, they might read your comment, see that the code doesn't do that, and rewrite the code so that it matches the comment (I've seen that happen).

Code formatting

The PEP8 formatting style is mentioned alot when you're coding Python, so many developers expect you to format your code to follow that standard. Many IDEs have automated style formatting (such as PyCharm (Menu item 'Code'-> 'Reformat Code')) which can save your time.
PEP257 discusses what a Docstring is and where you should use it. Whilst yes, it's possible to use it as a string representation, you should be using a string with "\n" to denote a line break, and instead of .format(var), using f-strings which have been available since Python 3.6.
Please research PEP8, PEP257 and f-strings to help improve your code formatting.
If your IDE doesn't help with all of this, you can install pylint (pip install pylint) to help locate issues in your code. Here is pylint in action:

************* Module all_data
all_data.py:9:8: C0103: Variable name "b" doesn't conform to snake_case naming style (invalid-name)
all_data.py:11:8: R1723: Unnecessary "else" after "break" (no-else-break)
************* Module data_base
data_base.py:7:0: C0301: Line too long (119/100) (line-too-long)
data_base.py:8:4: C0103: Variable name "move_list_X" doesn't conform to snake_case naming style (invalid-name)
data_base.py:9:4: C0103: Variable name "move_list_O" doesn't conform to snake_case naming style (invalid-name)
data_base.py:50:9: W1514: Using open without explicitly specifying an encoding (unspecified-encoding)
data_base.py:50:39: C0103: Variable name "f" doesn't conform to snake_case naming style (invalid-name)
data_base.py:1:0: W0611: Unused import all_data (unused-import)
data_base.py:2:0: C0411: standard import "import json" should be placed before "import all_data" (wrong-import-order)
************* Module main
main.py:12:0: C0301: Line too long (121/100) (line-too-long)
main.py:13:0: C0301: Line too long (106/100) (line-too-long)
main.py:25:0: C0301: Line too long (108/100) (line-too-long)
main.py:38:0: C0325: Unnecessary parens after 'if' keyword (superfluous-parens)
main.py:67:0: C0325: Unnecessary parens after 'if' keyword (superfluous-parens)
main.py:75:0: W0311: Bad indentation. Found 28 spaces, expected 24 (bad-indentation)
main.py:9:8: C0103: Variable name "moves_X" doesn't conform to snake_case naming style (invalid-name)
main.py:10:8: C0103: Variable name "moves_O" doesn't conform to snake_case naming style (invalid-name)
main.py:14:8: C0103: Variable name "move_X" doesn't conform to snake_case naming style (invalid-name)
main.py:15:8: C0103: Variable name "move_O" doesn't conform to snake_case naming style (invalid-name)
main.py:34:16: C0103: Variable name "move_X" doesn't conform to snake_case naming style (invalid-name)
main.py:35:16: C0103: Variable name "move_O" doesn't conform to snake_case naming style (invalid-name)
main.py:6:4: R1702: Too many nested blocks (6/5) (too-many-nested-blocks)
main.py:63:16: C0103: Variable name "move_X" doesn't conform to snake_case naming style (invalid-name)
main.py:64:16: C0103: Variable name "move_O" doesn't conform to snake_case naming style (invalid-name)
main.py:6:4: R1702: Too many nested blocks (6/5) (too-many-nested-blocks)
main.py:6:8: W0612: Unused variable 'i' (unused-variable)
main.py:4:0: R0912: Too many branches (13/12) (too-many-branches)
************* Module rules
rules.py:8:53: C0303: Trailing whitespace (trailing-whitespace)
rules.py:67:0: C0325: Unnecessary parens after 'return' keyword (superfluous-parens)
rules.py:11:9: W1514: Using open without explicitly specifying an encoding (unspecified-encoding)
rules.py:23:4: R1705: Unnecessary "elif" after "return" (no-else-return)
rules.py:21:0: R0911: Too many return statements (9/6) (too-many-return-statements)
rules.py:54:8: W0622: Redefining built-in 'sum' (redefined-builtin)
rules.py:56:12: C0103: Variable name "z" doesn't conform to snake_case naming style (invalid-name)
rules.py:56:15: C0103: Variable name "g" doesn't conform to snake_case naming style (invalid-name)
rules.py:58:12: R1724: Unnecessary "else" after "continue" (no-else-continue)
rules.py:73:9: W1514: Using open without explicitly specifying an encoding (unspecified-encoding)
rules.py:73:39: C0103: Variable name "f" doesn't conform to snake_case naming style (invalid-name)

Code Refactoring

Pieces of code like:

                for move in moves_X:
                    move[0]['value'] += 1

                for move in moves_O:
                    if move[0]['value'] > 1:
                        move[0]['value'] -= 1
                        

and

                for move in moves_O:
                    move[0]['value'] += 1

                for move in moves_X:
                    if move[0]['value'] > 1:
                        move[0]['value'] -= 1
                        

are identical except for the iterator. This could be refactored into:

            def calculate_points(first, second):
                for move in first:
                    move[0]['value'] += 1

                for move in second:
                    if move[0]['value'] > 1:
                        move[0]['value'] -= 1

and complete the process by issuing 2 statements in their place:

        calculate_points(moves_X, moves_O)
        calculate_points(moves_O, moves_X)          

Use of break and continue

Spaghetti code is the name given to code that doesn't complete the block fully, jumping out into another section of code. Ways to avoid this:

• ensuring you limit the number of iterations a loop performs by having the iterator as a function, or
• having a variable that is set to False inside the block, allowing the loop to exit when it returns to be evaluated before the next loop.
• refactoring the code into a function, and using return to leave the function when a condition is satisfied

As an example:

playing = True
while playing:
    ...
    if player.is_dead:
        playing = False
    else:
        ...do regular stuff...
    

or

while all_players_are_alive():
    ...do regular stuff...
    
def all_players_are_alive():
    for player in game_engine.players():
        if player.is_dead:
            return False
    return True
    

Entry point

As we're writing a script for execution, and not as a library, the entry point if __name__ == "__main__": should be used.
For example, if you pushed your script into the code repository, and a code documentor such as Sphinx ran over it, it would freeze because it would start playing the game. This is because the code begins running as soon as Python loads it, when the goal of the documentor was just to analyse the code.

Here is the process in action:

ch@ubuntu:~/PycharmProjects/testing/tic-tac-toe-main$ python
Python 3.8.3 (default, Sep  9 2020, 15:01:52) 
[GCC 7.5.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import main

[' ', ' ', ' ']
['X', ' ', ' ']
[' ', ' ', ' ']
...etc...


ch@ubuntu:~/PycharmProjects/testing/tic-tac-toe-main$ nano main.py 

import rules

def play_game():
    #main game loop. Number of repetition defines how many games are played
    for i in range (1):
....
....
    #after compliton of the main game loop we save the results
    rules.save()

if __name__ == "__main__":
    play_game()

and now, into Python again:

ch@ubuntu:~/PycharmProjects/testing/tic-tac-toe-main$ python
Python 3.8.3 (default, Sep  9 2020, 15:01:52) 
[GCC 7.5.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import main
>>> 

See? it didn't execute automatically. If you want to run it directly, with python main.py, that would begin the execution phase, playing the game.

Hard-coding of values/strings/indexes

When you hard-code strings, numbers, or the index numbers of lists, if something changes in your code, you need to find all the statements and change them. Take for instance, the example function above, in the section Code Refactoring.

If you had to find all instances of move[0]['value'] and change the value of 0 into 1, you'd spend quite some time to do it, and to verify every change was actually made, you'd need to look at every single line of code, or run the game quite a few times and look carefully for mistakes.

If, however, you replaced all those "magic" numbers and strings with a single variable, only a single change in a single location should be needed, and all the numbers or strings would be changed automatically.
If you need more clarification on this principle, look at Wikipedia for "Magic number (programming)".

Final Note

This was quite a challenge to review your code, and I hope you spend some time learning about all the items mentioned above. I wanted to also demonstrate code for a tic-tac-toe game - not exactly the same as yours - this is a single player version (no weighting or 100,000 loops). If you read the code carefully, it should also introduce you into some new ways of writing code.
The input system is very simple, using co-ordinates to select the cell.

I hope this review helps you with your code, good luck for any future programming.

import random
from enum import Enum
from itertools import product


class Tile(Enum):
    Empty = 0
    X = 1
    O = 2


class Board:
    def __init__(self):
        self._board = {f"{x},{y}": Tile.Empty for x, y in
                       product(range(3), range(3))}
        self._game_over = False
        self._winning_combinations = (
        ("0,0", "0,1", "0,2"), ("1,0", "1,1", "1,2"), ("2,0", "2,1", "2,2"),
        ("0,0", "1,0", "2,0"), ("0,1", "1,1", "2,1"), ("0,2", "1,2", "2,2"),
        ("0,0", "1,1", "2,2"), ("0,2", "1,1", "2,0"))
        self._winner = ""

    def pick_cell(self, address, tile, name):
        print(f"{name} has picked cell {address} for {tile.name}")
        current_tile = self.layout[address]
        if current_tile != Tile.Empty:
            print(f"Cannot change that cell ({address}). Already chosen: "
                  "{current_tile.name}")
        else:
            self.layout[address] = tile
            if self.check_for_winner():
                self._game_over = True
                print(f"Game is over! {self._winner} has won!")

    @property
    def game_over(self):
        return self._game_over

    @property
    def layout(self):
        return self._board

    def display_board(self):
        print("*" * 50)
        for counter, (address, status) in enumerate(self.layout.items()):
            char = "?" if status.name == "Empty" else status.name
            print(f"{char:5}  ", end="")
            if counter in (2, 5):
                print("")
        print("\n")

    def check_for_winner(self):
        for tile in (Tile.O, Tile.X):
            for group in self._winning_combinations:
                a = self.layout[group[0]] == tile
                b = self.layout[group[1]] == tile
                c = self.layout[group[2]] == tile
                if a == b == c and a:
                    self._winner = tile.name
                    return True

    @staticmethod
    def available_cells():
        return [f"{address}" for address, status in board.layout.items() if
                status == Tile.Empty]


def get_user_selection(params, display_params=""):
    is_a_valid_choice = False
    if display_params == "":
        display_params = params
    while not is_a_valid_choice:
        choice = input(f"\nPlease select a choice ({display_params}): ")
        if choice.isalpha():
            choice = choice.lower()
        if choice.isnumeric():
            choice = int(choice)
        if choice in params:
            return choice

        print(f"That's unfortunately not a selection you can make. Please "
              f"select one of these: {display_params}")


if __name__ == "__main__":
    board = Board()
    print("Who are you playing? Noughts or Crosses? (o/x)?")
    choice = get_user_selection(["x", "o"])
    player = Tile.X
    computer = Tile.O
    if choice == "o":
        player = Tile.O
        computer = Tile.X

    while not board.game_over and len(board.available_cells()):
        board.display_board()
        selection = get_user_selection(board.available_cells())
        board.pick_cell(selection, player, "Player")
        board.display_board()
        if not board.game_over and len(board.available_cells()):
            selection = random.choice(board.available_cells())
            board.pick_cell(selection, computer, "Computer")