11
\$\begingroup\$

I did this Tic-Tac-Toe as an exercise for OOP.

I would like to know possible improvements (the method main loop is too rock and roll, if you have suggestion to reduce it and improve readability).

Code:

import numpy as np

class Game():
    """Core of the game, contains the loop and victory checking"""
    
    def __init__(self): 
        """Initialization of the matrix and game launch"""
        self.__init_matrix() 
        self.__init_launch_game() 
       
    def __init_matrix(self):
        """Fill matrix 3 by 3 of nan"""
        self.matrix = np.empty((3,3,))
        self.matrix[:] = np.nan
        
    def __init_launch_game(self):
        """Instantiate the two players and launch the loop"""
        self.player1 = Player(1)
        self.player2 = Player(2)
        self.__loop_game()
        
    def __loop_game(self):  
        """Ask the player to play as long as there is no victory or no full matrix"""         
        while not(self.__is_victory()) and not(self.__is_full()):
            row, col = 5, 5
            while not(self.__is_case_empty(row, col)): 
                row,col = self.player1.tick_case()
                self.__is_case_empty(row, col)
                print(self.__is_case_empty(row, col))
            self.__set_case(row, col, 1)
            
            if not(self.__is_victory()) and not(self.__is_full()):
                row, col = 5, 5
                while not(self.__is_case_empty(row, col)): 
                    row,col = self.player2.tick_case()
                    self.__is_case_empty(row, col)
                self.__set_case(row, col, 2)
                if self.__is_victory():
                    print('player2 wins')
            else:
                print('player1 wins')
        print(not(self.__is_victory()) and self.__is_full())        
        if not(self.__is_victory()) and self.__is_full():
            print('no winner')
            
    def __is_case_empty(self, row, col):
        """Check if the case has a nan"""
        if row == 5 and col == 5:
            return False 
        elif np.isnan(self.matrix[row, col]):
            return True
        else:
            return False 
            
    def __is_victory(self):
        """check if the victory condition is met"""
        for nb in range(3):
                if np.isnan(np.min(self.matrix[nb,:])) != True: # Check row
                    if np.sum(self.matrix[nb,:]) % 3 == 0: 
                            return True
                if np.isnan(np.min(self.matrix[:,nb])) != True:# Check col 
                    if np.sum(self.matrix[:,nb]) % 3 == 0:
                        return True  
        diag1 = self.matrix[0,0]+self.matrix[1,1]+self.matrix[2,2] # Check diag1 
        if np.isnan(np.min(diag1)) != True:
            if np.sum(diag1) % 3 == 0:                              
                return True
        
        diag2 = self.matrix[0,2]+self.matrix[1,1]+self.matrix[2,0] # Check diag2
        if np.isnan(np.min(diag2)) != True:
            if np.sum(diag2) % 3 == 0:                                
                return True   
        return False 
    
    def __is_full(self):
        """check if the matrix is full"""
       if np.isnan(self.matrix).any():
           return False
       else:
           return True 
    
    def __set_case(self, row, col, player):
        """Change the value of a case in the matrix"""
        if player == 1: 
            self.matrix[row , col] = 0
        elif player == 2:
            self.matrix[row, col] = 1
            
        print(self.matrix)                   

class Player():
    """Player role, which ask what case shall be ticked and if the entered number is valid"""
    
    def __init__(self, player_id): 
        """init player id"""
        self.player_id = str(player_id)
    
    def tick_case(self):
        """ask the player a case to tick"""
        row, col = 10, 10
        while not(self.__is_case_valid(row, col)):
            print('Player ' + self.player_id + ' , this is your turn')
            row = input('which row do you want to enter?' ) 
            col = input('which row do you want to enter?' ) 
        return(int(row), int(col))
    
    def __is_case_valid(self, row, col):      
        """check the validity of the ticked case"""
        if int(row) < 0 or int(row)>3 or int(col) < 0 or int(col)>3:
            print('value must be between 0 and 3')
            return False
        else:
            return True 
        
A = Game()
\$\endgroup\$
0
12
\$\begingroup\$

__ prefix

    while not(self.__is_victory()) and not(self.__is_full()):
            row, col = 5, 5
            while not(self.__is_case_empty(row, col)):
                row,col = self.player1.tick_case()
                self.__is_case_empty(row, col)
                print(self.__is_case_empty(row, col))
            self.__set_case(row, col, 1)

            if not(self.__is_victory()) and not(self.__is_full()):
                row, col = 5, 5
                while not(self.__is_case_empty(row, col)):
                    row,col = self.player2.tick_case()
                    self.__is_case_empty(row, col)
                self.__set_case(row, col, 2)
                if self.__is_victory():
                    print('player2 wins')
            else:
                print('player1 wins')
        print(not(self.__is_victory()) and self.__is_full())
        if not(self.__is_victory()) and self.__is_full():
            print('no winner')

Do you feel your code is very legible here? Not to me at least, the problem is that the member functions have a __ prefix which makes the code look convoluted for no good reason.
The exact same thing without the prefix

        while not(self.is_victory()) and not(self.is_full()):
            row, col = 5, 5
            while not(self.is_case_empty(row, col)):
                row,col = self.player1.tick_case()
                self.is_case_empty(row, col)
                print(self.is_case_empty(row, col))
            self.set_case(row, col, 1)

            if not(self.is_victory()) and not(self.is_full()):
                row, col = 5, 5
                while not(self.is_case_empty(row, col)):
                    row,col = self.player2.tick_case()
                    self.is_case_empty(row, col)
                self.set_case(row, col, 2)
                if self.is_victory():
                    print('player2 wins')
            else:
                print('player1 wins')
                
        print(not(self.is_victory()) and self.is_full())
        if not(self.is_victory()) and self.is_full():
            print('no winner')

Re-think Code structure

What I don't see is the need for a very huge Player What is the need for having two Player objects in your Game? Why can't you have just one variable that will either say Player1 or Player2 depending on whose turn it is to play? That way you can easily validate moves without the extra convoluted code in your current Player class.

What you need to do is create an attribute that your Game class can hold, I would prefer an Enum.

from enum import Enum

class Player(Enum):
    one = 'x'
    two = 'o'

Now in your __init__ method of Game, you can simply do

    def __init__(self):
        self.player = Player.one

Each time a player makes a move, you can simply update it this way

    def switch_player(self):
        if self.player == Player.one: 
            self.player = Player.two
        else: 
            self.player = Player.one

And whenever you need to value of Player-1 or 2, that is if you want to print the board, you can always do

self.player.value

Catch bad input

        row, col = 10, 10
        while not(self.__is_case_valid(row, col)):
            print('Player ' + self.player_id + ' , this is your turn')
            row = input('which row do you want to enter?' ) 
            col = input('which row do you want to enter?' ) 
        return(int(row), int(col))

There is a problem here. Imagine this scenario

which row do you want to enter? Code review
which row do you want to enter? 2

I have three problems here

  1. The second message should ask which column do you want to enter?
  2. After entering bad input the first time, the code happily accepts it and lets me enter the second one.
  3. After the code discovers that I entered invalid input, it doesn't ask me for new input, it gives me `ValueError: invalid literal for int() with base 10: 'Code review'

you need to try and except and catch the ValueError and ask the user to enter valid input again. I would make a simple function

def input_field(prompt, err_msg):
    while True:
        try:
            field = int(input(prompt))
        except ValueError:
            print(err_msg)
            continue
        break 
    return field 

and then do the validation process separately for row and column so the user doesn't have to repeat the same input if he messes up.

     while True:
         row = input_field("Enter row: ", "Error row...")
         if(not (is_row_valid(row))): 
             print("error row...")
             continue
         break
         
     while True:
         col = input_field("Enter col", "Error col...")
         ...

Print a nicer board

When you print the current state of the board, you are printing

[[nan nan 1]
 [ 0. nan nan]
 [nan nan nan]]

How about printing this

   . | . | o
  -----------
   x | . | .
  -----------
   . | . | .


Make case a class

How about you represent any move with a class? A class that can hold three things, a row, a col, and the player

You can use a data class

@dataclass
class Move:
    row : int
    col : int
    player : Player

in your Game class, you can simplify it into

# After receiving row and col from user

move = Move(row, col, self.player)

Now you can pass around this Move object wherever you need it, for example, if you wanna validate this move

def validate_move(self, move):
    if move.row < 0 or move.row > 3:
        return False
    if move.col < 0 or move.col > 3:
        return False

    return not(cell_is_occupied(move.row, move.col))

Anti-pattern

        if np.isnan(self.matrix).any():
            return False
        else:
            return True 

This basically translates to

        if True:
            return False
        else:
            return True

Why not just

        return not (True):

So basically

        return not(np.isnan(self.matrix).any())

Possible code structure

I wrote out a very basic design of how this game can be re-structured. Most of the big functions are left our for your implementation, I have filled in for some minor ones

from enum import Enum
from dataclasses import dataclass


class Player(Enum):
    one = 'o'
    two = 'x'
    none = '.'

    
@dataclass
class Move:
    row : int
    col : int 
    player : Player 

class Game:
    def __init__(self):
        self.player = Player.one 
        self.board = ...
        self.number_of_turns = 0

    def switch_player(self):
        # Switch players. If player is 1, player becomes 2 and vice-versa

        
    def print_board(self):
        # print a neat representation of the game board 

        
    def victory(self):
        # return the Player that won, if nobody won the game return None 

        
    def draw(self):
        return (self.victory() is None) and (number_of_turns == size_of_board):
            
    def input_move(self):
        # handle move input and valdation, you can create more functions if you want
        # receive row, col from user
        return Move(row, col, self.player)

    def perform_move(self, move):
        # make the move changes using move.row, move.col and move.player, and then self.switch_player()
        # also self.number_of_turns += 1
    def game_loop(self):
        while True:
            self.print_board()
            move = input_move()
            self.perform_move(move)
            if self.victory():
                 ...
            else if self.draw():
                 ...
\$\endgroup\$
2
  • \$\begingroup\$ Thanks a lot, I like the elegance of having a player class and you switch player with attributes. I am not familiar with enum and dataclass but will study. Will definitly rewrite with your advices along with advices of the other people. \$\endgroup\$ – Kerdiorp Nov 11 '20 at 19:15
  • \$\begingroup\$ @Kerdiorp Well, I can't impose anything on you. it's your program :) \$\endgroup\$ – Aryan Parekh Nov 11 '20 at 19:37
5
\$\begingroup\$

I'll just add some additional points that aren't brought up by the other users.

Initialize all your variables in __init__

When looking at a class, you should be able to tell all its attributes by looking in the __init__ method. Here it's not clear why you have self.matrix in some methods until you manually follow through all the calls in the initializer. Considering you never use __init_launch_game or __init_matrix anywhere, I'd suggest removing them and keep everything in your initializer.

def __init__(self): 
    """Initialization of the matrix"""
    self.matrix = np.empty((3,3,))
    self.matrix[:] = np.nan
    self.player1 = Player(1)
    self.player2 = Player(2)

If you want a setup or restart function, still declare the attributes with bogus values in the initializer.

def __init__(self): 
    """Initialization of the matrix"""
    self.matrix  = None
    self.player1 = None
    self.player2 = None
    self.setup()

Remove magic numbers

Don't use magic numbers. What does 3 mean? It very much depend on the context. But if I ask what MAXIMUM_ROWS means, then you could very easily figure out that it's a value for the maximum amount of rows. So try to avoid raw literals in your code (unless they're easily understandable in context, which is usually numbers as 0 or maybe 1) and declare them once. You can have them as either global variables, class variables or instance variables. For your use-case, I think it make sense to have them ass class variables.

class Game:
    MAXIMUM_ROWS    = 5
    MAXIMUM_COLUMNS = 5

    # ... And in later in the code you'll use it with:
    if row == Game.MAXIMUM_ROWS and col == Game.MAXIMUM_COLUMNS:

Remove unnecessary tokens

Some tokens (i.e. characters or sequence of characters that represent some semantic) are used unnecessarily. For example class Game():. The parenthesis are meant to contain a list of classes to inherit from. Since you don't inherit from any classes, simply remove it (class Game:).

The same goes for all your conditions. not(self.__is_victory()) and not(self.__is_full()) can be just not self.__is_victory() and not self.__is_full(). Since parenthesis is just for so much (logical grouping, method calls, tuples, mathematical precedence) removing them makes the code a whole lot cleaner and easier to read.

Be consistent!

Minor nitpick, but be consistent when you write. Sometimes you have spaces after colon (row, col = 5, 5) and sometimes not (row,col = self.player2.tick_case()). Sometimes you have spaces between operators and sometimes not (if int(row) < 0 or int(row)>3). This might seem arbitrary, but can help you when the code is getting bigger in size and you need to search for specific patterns in your code base. It also helps with readability.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Welcome to the Code Review Community, nice first answer. \$\endgroup\$ – pacmaninbw Nov 11 '20 at 14:06
  • \$\begingroup\$ Thanks, I will keep my init simple, improve consistency and I really like the idea to avoid magic numbers \$\endgroup\$ – Kerdiorp Nov 11 '20 at 19:34
5
\$\begingroup\$

Here are some ideas how you could improve your program which I hope will be helpful.

Documentation

Your use of docstrings is good to see. If you've never read it before, PEP 257 (Docstring Conventions) is useful so you can follow the norms in Python.

Name Mangling

I see you've used the double underscore for most methods, e.g. __is_full. This has the effect of mangling the names so it's not so easy to access the methods from outside the class (like private in many other languages). Python tends to have a different attitude to other languages though on private methods: generally you can default to leaving everything unmangled and public, which has the added bonus of making your uses of the method a bit less underscore-filled. Further discussion is available on Stack Overflow if you're interested.

Sentinel Values

It took me a couple of minutes to realise that you're using position (5, 5) to represent an impossible slot, and later you use (10, 10) to do the same. It might make more sense just to use None to represent an invalid value and check if row is None etc.

# Initialise row and column with invalid value before we take user input
row, col = None, None

Boolean Logic

You can save a few lines like so:

def __is_full(self):
   """check if the matrix is full"""
   return not np.isnan(self.matrix).any()

There are a few other spots where you could simplify your boolean logic, although it's up to you how readable you think this is. You also don't need some brackets here: not(self.__is_victory()). You may just write not self.__is_victory() and this is probably more conventional.

Constructors

It is unconventional that the constructor of Game actually launches the game. I'd expect it just to create the game object, and have a separate run method.

Main Guard

It is convention in Python to use a main guard to check whether to run your code as a script. So you might write

if __name__ == '__main__':
    game = Game()
    game.run()
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Thank you. Understood each points. Took me some time to get Constructors but with Main Guard it was ok. I was not aware that mangling was a bad practice \$\endgroup\$ – Kerdiorp Nov 10 '20 at 19:51
5
\$\begingroup\$

Many aspects have been addressed in the other answers so this answer will focus on a higher-level software engineering consideration but, first, a few nitpicks:

  • algorithmic considerations are important! In particular, avoid doing unnecessary work: when a player plays a move, they only ever touch a single row and a single column and, furthermore, might not even touch the diagonals. Yet, your implementation checks every single row, column, and diagonal every single time. This is a waste of effort and the implementation could be made more efficient by only checking a row/column/diagonal if it has been affected by the move.

  • Don't be afraid of while True: there are many instances where you need to check something as the condition of your loop but that something is only set inside of the loop and you therefore initialize it, before the loop, with garbage so that your program doesn't crash the first time it checks the condition. Don't do that!

    Instead, use while True, set that something, and check your condition to see if the loop is over. This is particularly powerful when the loop is the body of a function. For example (BTW, the 10 made me waste a couple of seconds wondering why 10? ... don't make your reader waste their time!):

    row, col = 10, 10
    while not(self.__is_case_valid(row, col)):
        print('Player ' + self.player_id + ' , this is your turn')
        row = input('which row do you want to enter?')
        col = input('which row do you want to enter?')
    return(int(row), int(col))
    

    becomes (ignoring issues in the rest of the code for now):

    while True:
        print('Player ' + self.player_id + ' , this is your turn')
        row = input('which row do you want to enter?')
        col = input('which row do you want to enter?')
        if self.__is_case_valid(row, col):
            return(int(row), int(col))
    
  • Writing high-quality code inevitably relies on well-named variables and functions: although your code is pretty good in that aspect, I will use cell in the following when referring to what you call a case and board instead of your matrix: this improves readability, IMO.

  • Implement your algorithms as intuitively as possible, instead of evolving them based of artificial constraints. For example, in __is_case_empty, you effectively check if a cell (see previous point) is empty or not, given its coordinates; I'm convinced that the algorithm that would naturally come to you would be something like "check if the coordinates or valid and, if so, check what value I have stored at the corresponding address" and nowhere would you think of checking "if the coordinates are (5,5)". Then why do it in your implementation? Instead, something like if not (0 <= row < 3 and 0 <= col < 3): would be more appropriate.

  • Don't rely on magic numbers: instead of self.matrix = np.empty((3,3,)), define self.size = 3 and use it in place of the literal 3, this way, we immediately understand what you mean (see my previous point about row, col = 10, 10) and it can be programmatically accessed from elsewhere, instead of repeating 3 throughout the code. What if you wanted to upgrade your game to a 4-by-4 board?

  • Think twice about dependencies: adding a dependency to your code is a trade-off as your code itself becomes simpler thanks to the functionality of the library but, at the same time, your project complexity increases. People will need to download that extra dependency, leading to potential issues with deployment and you are now wed to that library: bugs in your application might come from code you don't own, changes in functionality might break your code (remember that the library developers will certainly not care about your code) and what if the license of the code changes or if the library simply stops being maintained altogether? Of course, NumPy isn't going away but, in general, adding a dependency to your code should be seriously thought through. NumPy is an incredibly powerful library but, in your case, what do you really gain from the dependency you added to your project? In the way you use it, NaNs could easily be replaced by None and self.matrix[i][j] is really not that much harder to type than self.matrix[i,j] so are the pros really outweighing the cons?

  • not, return, ... are not functions: don't "call" them and, if parentheses are necessary (e.g. for operator priority), add a space before them, as you would do in English: return (int(row), int(col)) (BTW, the parentheses here are okay because the statement creates a tuple but I would personally write return int(row), int(col)) and while not self.__is_case_valid(row, col):.

  • Drop the () from class Class():


Now the main point of this answer:

Use proper encapsulation

OOP is a very powerful paradigm as it reflects our natural vision of the world. Things interact with each other and when representing this in code, it becomes more intuitive to follow. However, as with real-world objects, properties of a class should be contained in that class: that's called encapsulation.

The issue

In your case, Player knows details about the board (owned by an instance of Game) which are not supposed to be relevant to a player's actions. In particular, Player is the one checking is_case_valid: that doesn't makes sense as that check and those properties are those of the board. Furthermore, have you noticed how similar Player.__is_case_valid and Game.__is_case_empty are, conceptually? One of those has to be in the wrong place (actually, it's both; see below)!

This is even more obvious when you look at the name of your methods: tick_case and is_case_valid both have "case" in their names, the cell (case) should either be part of the object (which doesn't make sense for a Player) or be a parameter of the method but it's neither.

The fact that your Game class holds your board data (matrix) is itself another leaked encapsulation: the board is and object and should be treated as so. Just read your methods out loud: does Game().__is_full() really make sense? Is it really the game that is full? Yet another symptom of this missed opportunity is the fact that your code has to deal with so many unnecessary details that its actual purpose becomes unclear at first sight to the reader. Compare your

for nb in range(3):
        if np.isnan(np.min(self.matrix[nb,:])) != True: # Check row
            if np.sum(self.matrix[nb,:]) % 3 == 0:
                    return True
        if np.isnan(np.min(self.matrix[:,nb])) != True:# Check col
            if np.sum(self.matrix[:,nb]) % 3 == 0:
                return True
diag1 = self.matrix[0,0]+self.matrix[1,1]+self.matrix[2,2] # Check diag1
if np.isnan(np.min(diag1)) != True:
    if np.sum(diag1) % 3 == 0:
        return True

diag2 = self.matrix[0,2]+self.matrix[1,1]+self.matrix[2,0] # Check diag2
if np.isnan(np.min(diag2)) != True:
    if np.sum(diag2) % 3 == 0:
        return True
return False

with a more encapsulated example where all the details have been abstracted away into a Board class:

for i in range(self.board.size):
    found_winner = self.board.unique_value_in_row(i)
    if found_winner:
        return found_winner
    found_winner = self.board.unique_value_in_col(i)
    if found_winner:
        return found_winner

found_winner = self.board.unique_value_in_diag()
if found_winner:
    return found_winner
return self.board.unique_value_in_sec_diag()

you might believe that this is not really a win as you might now have to write more code: well it is better to have more lines of clearer code than fewer of hard-to-read code as it makes it easier to read and therefore easier to debug and, if your encapsulation is done right, it becomes by design harder to unknowingly add bugs because each layer of the problem is addressed in isolation. Also, notice something else: those comments explaining what the code does are gone as the code now documents itself thanks to our well-picked function and variable names!

Finally, for the previous method, we are still duplicating some code as the logic is basically "if this way of winning is verified, we've found the winner, else if this other way of winning is verified, we've found the winner, else ..." which still "smells" as we duplicate the condition checking. This can be simplified even further by using generators (a very powerful and central concept in Python):

def winning_candidates(self):
    yield from (self.board.unique_value_in_row(i)
                for i in range(self.board.size))
    yield from (self.board.unique_value_in_col(i)
                for i in range(self.board.size))
    yield self.board.unique_value_in_diag()
    yield self.board.unique_value_in_sec_diag()

def find_winner(self):
    return next(winner for winner in self.winning_candidates() if winner, None)

In general, a function should do one (and only one) action, expressed clearly through its name (usually a verb/object combination). This is another important aspect of proper class architecture: each method should do one thing e.g. initialize (part of) its state (note that, as this is very common, Python even provides the built-in __init__ method to do that!), check something, change something else, ...

class Game:

In the case of Game, __init__, the method supposed to initialize a new instance of the class, calls __init__launch_game, which does more initialization and then launches the game through __loop_game: these 2 (3?) steps are 2 actions and those 2 actions should be 2 function calls! Wouldn't it make more sense, for the calling code, to do as follows?

game = Game()  # obviously, this calls game.__init__()
game.play()

When you embrace this decomposition, you have more function calls and therefore get more return values (if a function is an action, a return value is the result of that action!), which can then be used to make your code even more readable. For example:

game = Game()
winner = Game.play()
if winner:
    print('Player {} wins!'.format(winner))
else:
    print('No winner.')

or, another possibility:

players = [Player(i) for i in range(2)]
game = Game(players)
winner = game.play()
if winner:
    print(f'{winner} wins!')
else:
    print('No winner.')

where Game.play() now returns a (reference to a) Player instance!

Notice the last print in the previous example: this is yet another instance of proper encapsulation! Although certainly not obvious the first time you see it, what happens there is that {winner} in the f-string is replaced by the result of str(winner) which itself returns winner.__str__(). The __str__ method is, similarly to __init__, a special method supported by Python which is intended for returning the nicely printable string representation of an object; see Python's data model for more (and notice how it also follows this "one action per function rule" very closely).

The reason why this implements encapsulation well is that, once you have your implementation of Player.__str__, you can use it throughout your code and, if at any point, you want to change the way players are represented in your text output, you only have to change one piece of code, inside Player (as opposed to throughout your code). This is exactly why encapsulation is powerful: by programming against an abstract interface, you don't care about the how but only about the what and even if the how changes, your calling code doesn't.

class Player:

To illustrate this, imagine you start with the following:

def Player:
    def __init__(self, player_id):
        self.id = player_id

    def __str__(self):
        return 'Player {}'.format(self.id)

and, whenever you need to print the player, you simply do:

player = Player(i)
# ...
print(f'{player} plays very well!')
# or: print('{} plays very well!'.format(player)) for Python pre-3.6

Now, imagine that you want to implement a new feature for referring to a player by their name. Then, with a properly encapsulated implementation, you only have to modify the code inside of Player. For example:

def Player:
    def __init__(self, player_id, name=None):
        self.id = player_id
        self.name = name

    def __str__(self):
        return self.name or 'Player {}'.format(self.id)

and those names having to come from somewhere, provide them to the Player constructor in the top-level script:

players = [Player(i, name=ask_name()) for _ in range(2)]
game = Game(players)
# ...

At this point, by only having changed a couple of lines of code, we've also updated the behaviour of all those print(f'{player} ... lines (potentially thousands of them); good luck doing this with your currently hard-coded if is_player1: print('Player 1 ...') else: print('Player 2 ...')!

class Board:

Now, let's move on to Board: obviously, when designing your classes, you'll define their methods to reflect what you expect to need from their instances. A board for a game of chess will certainly expose a completely different interface from what is expected here! The following seemed to me to be what your implementation required from the board:

class Board:
    def __init__(self, size):
        self.cells = [[None] * size for _ in range(size)]

    @property
    def size(self):
        return len(self.cells)

    def is_empty_cell(self, row, col):
        try:
            return self.cells[row][col] is None
        except:
            return False

    def is_full(self):
        cells = itertools.product(range(self.size))
        return not any(self.is_empty_cell(i, j) for i, j in cells)

    def unique_value_in_row(self, row):
        return unique_value(self.cells[row])

    def unique_value_in_col(self, col):
        return unique_value(row[col] for row in self.cells)

    def unique_value_in_diag(self):
        return unique_value(row[i] for i, row in enumerate(self.cells))

    def unique_value_in_sec_diag(self):
        return unique_value(row[~i] for i, row in enumerate(self.cells))

    def tick_cell_as(self, row, col, player):
        self.cells[row][col] = player.id

Based on this, you can then compose your Game instance from what makes up a game (a board and players) and because most actions are performed by the players and the board, implementing the game itself becomes trivial:

class Game:
    def __init__(self, players):
        self.board = Board(3)
        self.players = players

    def play(self):
        while not (self.is_won() or self.board.is_full()):
            for player in self.players:
                player.tick_one_cell(self.board)
        return self.find_winner()

    def is_won(self):
        return self.find_winner() is not None

    def find_winner(self):
        # see above

while Player in turn interacts with the board (as would happen in real life):

class Player:
    # ...
    def tick_one_cell(self, board):
        print(f'{self}, this is your turn')
        row, col = self.query_cell(board)
        board.tick_cell_as(row, col, self)

    def query_cell(self, board):
        while True:
            # ... get row and col from the user ...
            if board.is_empty_cell(row, col):
                return row, col

See how readable all of this is? You haven't written the code, yet you can easily understand (and therefore modify) it. This is exactly why readability is important in software engineering: the person maintaining the code might not have been the one writing it! Once again, intuitive abstractions help a lot with that.

Encapsulation and dependencies

Finally, regarding my previous point about dependencies, and on the topic of encapsulation, if you still decide to have them (or, in another project, if it's a wise decision to do so), it is recommended to try to contain those dependencies as much as possible in encapsulated entities e.g. by only having NumPy calls inside of Board so that, if something goes wrong with that dependency, you can swap it out without having to re-write your whole application. Instead, you would simply adapt those containing entities: once again the modularity of the code provided by the abstraction of encapsulation saves you!

\$\endgroup\$
3
  • 1
    \$\begingroup\$ This is an awesome first answer, keep it up! \$\endgroup\$ – Aryan Parekh Nov 11 '20 at 17:51
  • \$\begingroup\$ @AryanParekh Thank you! \$\endgroup\$ – PiCTo Nov 11 '20 at 19:43
  • \$\begingroup\$ Thank you, detailed, clear and interesting, will reread it several time to diggest. I really like when you say to read loud the methods to see if it makes sense. I have difficulties to say what belong to what. I will focus on this and apply it to simple examples. With the quality of your answer and also the other answers, I will also spend time reading some other code reviews. \$\endgroup\$ – Kerdiorp Nov 11 '20 at 20:19

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.