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!
