Tic Tac Toe program that includes reports

Question

I'm working to use everything I'm learning in my reading and former code reviews to keep making this program better. I'm getting stuck on the following issue: the play_many_games and game_is_over methods in the GamesEngine class both call reports. I don't like this because the method names don't at all lead one to believe that reports are being called. I'd rather have functions do only one thing: so... basically I can't figure out the right way to call reports which need to be called after games end. Note that the main function in the top level module (tic_tac_toe.py) is doing the "one thing" of initializing reports and starting the games (i.e. there, in the main function, I feel justified touching "game" and "report" even though they are two different things). But once the games are started I find it harder to justify calling reports within the GamesEngine class which should just be about games.

Here is what I'm wondering: do I just suck more and more out of the GamesEngine class and dump it into the main function so that main knows when games are ending and can therefore handle the actual report calls? Or is there some design pattern I can use that watches for games to end and then calls reports (if yes, I don't know how to do it just yet)?

Basically I'm confused about how the GamesEngine class can somehow communicate with the reports.py module in a clean way that follows good clean programming principles.

Here are the three modules and their contents:

tic_tac_toe.py

"""Submit jobs for the requested number of games. Request game-reports.
Usage:
 python3 tic_tac_toe.py <number of games>
"""
import sys
from game import GamesEngine
from reports import (GameReport, ManyGamesReport)
def main(number_of_games):
 """Request report instructions. Request game initiation.
 Args:
 number_of_games: Number of games to play
 """
 report_requests = prepare_reports(number_of_games)
 games_engine = GamesEngine(*report_requests)
 play_games(games_engine, number_of_games)
def prepare_reports(number_of_games):
 """Create a report request based on the number of games. If 100 games or less requested, order printout for every game.
 Statistics on wins/ties for all games are always ordered.
 Args:
 number_of_games: Number of games to play
 """
 report_requests = []
 if number_of_games <= 100:
 report_on_game = GameReport()
 report_requests.append(report_on_game)
 report_on_many_games = ManyGamesReport()
 report_requests.append(report_on_many_games)
 return report_requests
def play_games(games_engine, number_of_games):
 """Initiate the games.
 Args:
 games_engine: An instance of GamesEngine that drives all games and knows when reports should be called.
 number_of_games: Number of games to play
 """
 games_engine.play_many_games(number_of_games)
if __name__ == '__main__':
 main(int(sys.argv[1]))

game.py

import random
class GamesEngine:
 def __init__(self, *reports_requested):
 self.game_result_as_string = '' # For unit tests this needs to remain an instance variable
 self.state_of_game = GameState()
 self.find_game_outcome = GameOutcome()
 self.row_index_for_move = None
 self.column_index_for_move = None
 self.reports_requested = reports_requested
 def __repr__(self):
 return "{}(*{})".format(self.__class__.__name__, self.reports_requested)
 def play_many_games(self, num_games_to_play):
 """Request report instructions. Request game initiation.
 Args:
 num_games_to_play: Number of games to play
 """
 while num_games_to_play > 0:
 self.play_one_game()
 num_games_to_play -= 1
 report_on_many_games = list(self.reports_requested).pop()
 report_on_many_games.report_outcome_statistics()
 def play_one_game(self):
 self.state_of_game = GameState()
 game_over = False
 while not game_over:
 self.make_move()
 self.update_who_moves_next()
 game_over = self.game_is_over()
 def make_move(self):
 # randomly select an available square for next move
 square_for_next_move_as_list_of_list = random.sample(self.state_of_game.available_squares, 1)
 self.row_index_for_move = square_for_next_move_as_list_of_list[0][0]
 self.column_index_for_move = square_for_next_move_as_list_of_list[0][1]
 # remove, from the available_squares list, the square we will use
 self.state_of_game.available_squares.remove([self.row_index_for_move, self.column_index_for_move])
 # make move
 self.state_of_game.board[self.row_index_for_move][self.column_index_for_move] = self.state_of_game.next_move
 def update_who_moves_next(self):
 temp = self.state_of_game.next_move
 self.state_of_game.next_move = self.state_of_game.previous_move
 self.state_of_game.previous_move = temp
 def game_is_over(self):
 self.game_result_as_string = ''
 if self.row_index_for_move is not None:
 self.game_result_as_string = self.find_game_outcome.find_winner_or_tie(self.state_of_game, self.row_index_for_move, self.column_index_for_move)
 if self.game_result_as_string:
 for report_calls in self.reports_requested:
 report_calls(self.state_of_game, self.game_result_as_string)
 return True
 else:
 return False
class GameOutcome:
 def __init__(self):
 self._letter_dict = {'X': -1, 'O': 1, ' ': 0}
 self._game_state = None
 self._row = None
 self._col = None
 self._game_outcome = None
 def __repr__(self):
 return "{}".format(self.__class__.__name__)
 def find_winner_or_tie(self, game_state, row, col):
 self._set_board_and_move(game_state, row, col)
 if self._check_row() or self._check_column() or self._check_main_diagonal() or self._check_off_diagonal() or self._check_tie():
 return self._game_outcome
 return ''
 def _set_board_and_move(self, state_of_game, row_index_of_move, column_index_of_move):
 self._game_state = state_of_game
 self._row = row_index_of_move
 self._col = column_index_of_move
 def _check_row(self):
 """Checks the row containing the most recent move to see if there is a win"""
 total = sum([self._letter_dict[self._game_state.board[self._row][column]] for column in range(3)])
 if abs(total) == 3:
 winning_letter = self._game_state.board[self._row][self._col]
 self._game_outcome = winning_letter
 return True
 return False
 def _check_column(self):
 """Checks the column containing the most recent move to see if there is a win"""
 total = sum([self._letter_dict[self._game_state.board[row][self._col]] for row in range(3)])
 if abs(total) == 3:
 winning_letter = self._game_state.board[self._row][self._col]
 self._game_outcome = winning_letter
 return True
 return False
 def _check_main_diagonal(self):
 """If most recent move is on the main diagonal, checks the main diagonal to see if there is a win"""
 if self._row == self._col:
 total = sum([self._letter_dict[self._game_state.board[diagonal_indexing][diagonal_indexing]] for diagonal_indexing in range(3)])
 if abs(total) == 3:
 winning_letter = self._game_state.board[self._row][self._col]
 self._game_outcome = winning_letter
 return True
 return False
 def _check_off_diagonal(self):
 """If most recent move is on the off diagonal, checks the off diagonal to see if there is a win"""
 if self._row + self._col == 2:
 total = sum([self._letter_dict[self._game_state.board[off_diagonal_indexing][2 - off_diagonal_indexing]] for off_diagonal_indexing in range(3)])
 if abs(total) == 3:
 winning_letter = self._game_state.board[self._row][self._col]
 self._game_outcome = winning_letter
 return True
 return False
 def _check_tie(self):
 if len(self._game_state.available_squares) == 0:
 self._game_outcome = 'Tie'
 return True
 return False
class GameState:
 def __init__(self):
 self.board = None
 self.available_squares = None
 self.initialize_board_and_available_squares()
 self.next_move = 'X'
 self.previous_move = 'O'
 def initialize_board_and_available_squares(self):
 self.board = [[' ' for i in range(3)] for j in range(3)]
 self.available_squares = [[i, j] for i in range(3) for j in range(3)]
 # Printing an instance of the class will display a standard
 # tic tac toe game board.
 def __str__(self):
 return "\n" + self.board[0][0] + "|" + self.board[0][1] + "|" + self.board[0][2] + "\n" + self.board[1][0] + "|" + self.board[1][1] + "|" + self.board[1][2] + "\n" + self.board[2][0] + "|" + self.board[2][1] + "|" + self.board[2][2]

reports.py

class ManyGamesReport:
 def __init__(self):
 self.count_wins_and_ties = {'X': 0, 'O': 0, 'Tie': 0}
 def __repr__(self):
 return "{}".format(self.__class__.__name__)
 def __call__(self, board, win_result):
 self.count_wins_and_ties[win_result] += 1
 def report_outcome_statistics(self):
 total_games = sum(self.count_wins_and_ties.values())
 print('Proportion of X wins: ' + str(self.count_wins_and_ties['X'] / total_games))
 print('Proportion of O wins: ' + str(self.count_wins_and_ties['O'] / total_games))
 print('Proportion of ties: ' + str(self.count_wins_and_ties['Tie'] / total_games))
class GameReport:
 def __repr__(self):
 return "{}".format(self.__class__.__name__)
 # call turn_on_end_of_game_reporting from main.py to run_random_tic_tac_toe_simulation this report method
 def __call__(self, board, win_result='Tie'):
 print(board)
 if win_result != 'Tie':
 print(win_result + ' won\n')
 else:
 print(win_result + '\n')

Answer 1

1. You follow PEP 8 pretty well. Just two minor things: the maximum line length should be 79 for characters, and 72 for docstrings. In game.py, for example, you have a line of 241 characters! Another thing is imports:

   Imports should be grouped in the following order:

   1. Standard library imports.
   2. Related third party imports.
   3. Local application/library specific imports.

   You should put a blank line between each group of imports.

   So, instead of this:

   import sys
   from game import GamesEngine
   from reports import (GameReport, ManyGamesReport)
   

   you should have:

   import sys
   from game import GamesEngine
   from reports import GameReport, ManyGamesReport
   

   Note that I removed redundant parenthesis.

2. In tic_tac_toe.py you have a hardcoded value 100 that defines the maximum number of games that will be printed. While it is clear what it does from the docstring, I advise you to make it an input parameter of the prepare_reports function.

3. In the same function you create and fill the report_requests list with one-two objects: ManyGamesReport() and/or GameReport(). And I see a problem here. The thing is that lists in Python are used only for homogeneous data. If you have heterogeneous data in a list then it means you are doing something wrong - you should rethink your design, think of other data structure, or simply use tuples, depending on the case. In fact, all this structure with reports seems too complicated for such a simple programming task. If I were you I would completely redesign the program.

4. When creating a board:

   self.board = [[' ' for i in range(3)] for j in range(3)]
   

   you don't need i and j. For throwaway variables you should use underscore _:

   self.board = [[' ' for _ in range(3)] for _ in range(3)]
   

5. Why are GameOutcome and GameState separate classes? For me this didn't make any sense... Also, in GameEngine you have:

   self.game_result_as_string = self.find_game_outcome.find_winner_or_tie(self.state_of_game, self.row_index_for_move, self.column_index_for_move)
   

   This just doesn't seem right. All this separation of logic to different classes is very confusing for me.

6. Function play_games is redundant. It just calls a play_many_games method of the games_engine object. You should take that call outside into main function.

7. play_many_games has the following lines:

   while num_games_to_play > 0:
    self.play_one_game()
    num_games_to_play -= 1
   

   This is not Pythonic. Use for _ in range(num_games_to_play) instead.

8. In GamesEngine you create empty GameState but then you rewrite it by empty GameState in play_one_game. This seems useless. Consider rewriting that part.

9. square_for_next_move_as_list_of_list is a very long name for a variable! Usually, we don't write a type or structure details of a variable in its name. So, at least, remove the as_list_of_list part.

10. In make_move you generate a move and then delete it from a list of available moves. But how about using random.sample(self.state_of_game.available_squares, 9) and just iterate over obtained random values? The code should look cleaner, and the performance shouldn't be a problem in this case.

11. The following function:

    def update_who_moves_next(self):
     temp = self.state_of_game.next_move
     self.state_of_game.next_move = self.state_of_game.previous_move
     self.state_of_game.previous_move = temp
    

    can be rewritten as a, b = b, a. This is a standard Python solution for swapping variables values.

---

Just for fun I fun I wrote the same program but using functional approach. Feel free to compare. The square field here can have a variable size. You can also specify a number of players and a number of consequent cells to win.

import collections
import itertools
import random
from typing import (Counter,
 Iterator,
 List,
 Tuple)
FieldType = List[List[str]]
def run_games(count: int,
 *,
 printable_games_count: int = 5,
 players: Tuple[str, ...] = ('X', 'O'),
 cells_in_line: int = 3,
 cells_to_win: int = 3) -> None:
 """Plays and prints results of Tic-Tac-Toe games"""
 wins_counter = collections.Counter()
 for _ in range(count):
 field, winner = game_result(size=cells_in_line,
 players=players,
 cells_to_win=cells_to_win)
 wins_counter[winner] += 1
 if count <= printable_games_count:
 print_game_result(field, winner)
 print_final_stats(players, wins_counter, count)
def game_result(*,
 size: int,
 players: Tuple[str, ...],
 cells_to_win: int) -> Tuple[FieldType, str]:
 """
 Plays a Tic-Tac-Toe game of specified size
 and returns final field with a winner
 """
 field = [[' ' for _ in range(size)]
 for _ in range(size)]
 symbols = itertools.cycle(players)
 possible_indices = list(itertools.product(range(size), repeat=2))
 indices = random.sample(possible_indices, size ** 2)
 for (row, col), symbol in zip(indices, symbols):
 field[row][col] = symbol
 for line in corresponding_lines(field, row=row, col=col):
 if win_condition(line, symbol=symbol, cells_to_win=cells_to_win):
 return field, symbol
 return field, 'Tie'
def corresponding_lines(field: FieldType,
 *,
 row: int,
 col: int) -> Iterator[List[str]]:
 """
 Yields row, column and diagonals (if applicable)
 for the input indices
 """
 yield field[row]
 yield [field[index][col] for index in range(len(field))]
 if row == col:
 yield [field[index][index] for index in range(len(field))]
 if row + col + 1 == len(field):
 yield [field[index][-index - 1] for index in range(len(field))]
def win_condition(line: List[str],
 *,
 symbol: str,
 cells_to_win: int) -> bool:
 """Checks if a line has enough same symbols for a victory"""
 return sum(element == symbol for element in line) == cells_to_win
def print_game_result(field: FieldType, winner: str) -> None:
 """Prints field and a winner"""
 print(*prettify(field), sep='\n')
 winner_string = f'{winner} won' if winner != 'Tie' else 'Tie'
 print(winner_string, end='\n' * 2)
def prettify(field: FieldType) -> Iterator[str]:
 """Yields rows of the field for pretty printing"""
 yield from map('|'.join, field)
def print_final_stats(players: Tuple[str, ...],
 counter: Counter,
 count: int) -> None:
 """Prints proportions of victories for each player"""
 for player in players:
 print(f'Proportion of {player} wins: {counter[player] / count}')
 print(f'Proportion of ties: {counter["Tie"] / count}')
if __name__ == '__main__':
 run_games(3,
 printable_games_count=4,
 players=('a', 'b', 'c'),
 cells_in_line=5,
 cells_to_win=5)

Output:

c|c|c|c|c
b|b|b|a|b
b|b|b|c|c
b|a|a| |a
a|a|a|a|c
c won
a|b|c|b|c
a|b|c|c|a
b|a|b|c|a
b|c|b|b|a
a|a|a|c|c
Tie
b|a| |a| 
b|a|c|b| 
c|a|c| |c
 |a|a|c|b
c|a| |b|b
a won
Proportion of a wins: 0.3333333333333333
Proportion of b wins: 0.0
Proportion of c wins: 0.3333333333333333
Proportion of ties: 0.3333333333333333

• Georgy: AFAIK, you shouldn't add an improved version of the code as an edit. It would be better to ask a new question. Please, see What should I do when someone answers my question?

Answer 2

I think you should have a Game class, which the user would call like so:

Game(number_of_games=10)

The Game instance would then keep track of the games by itself.

Game would then call GameEngine which would handle only one game and would be run like so:

# somewhere in `Game`
result = GameEngine().run()
self.report(result)

I think all the report stuff should go in the Game class itself as methods. If I am not clear enough, maybe I should draw you a picture...

├── Game
│ │ with attributes to keep track of the score
│ ├── __init__(number_of_games)
│ ├── nextgame()
│ └── report(result)
│
└── GameEngine
 │ with attributes to keep track of the board
 ├── __init__()
 └── ... the game engine ...

Game would be kind of the "meta" version of GameEngine if you think of it like that. The user won't be exposed to the internals of the game but only to the "launcher".

What is the separation of concerns. Well Game doesn't care about the board.

And GameEngine doesn't care about the user, and the score.

You can test the GameEngine easily because it only includes the core stuff.

You can test Game easily by using Mocking. How can I do "mocking" ? Simple add this before doing unittesting:

class GameEngine:
 def run(): return 'Tie'

See unittest.mock for more info.