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()
4 Answers 4
__
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
- The second message should ask
which column do you want to enter?
- After entering bad input the first time, the code happily accepts it and lets me enter the second one.
- 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():
...
-
\$\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– Kerdiorp2020年11月11日 19:15:56 +00:00Commented Nov 11, 2020 at 19:15
-
\$\begingroup\$ @Kerdiorp Well, I can't impose anything on you. it's your program :) \$\endgroup\$user228914– user2289142020年11月11日 19:37:02 +00:00Commented Nov 11, 2020 at 19:37
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.
-
1\$\begingroup\$ Welcome to the Code Review Community, nice first answer. \$\endgroup\$2020年11月11日 14:06:37 +00:00Commented Nov 11, 2020 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– Kerdiorp2020年11月11日 19:34:30 +00:00Commented Nov 11, 2020 at 19:34
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()
-
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– Kerdiorp2020年11月10日 19:51:48 +00:00Commented Nov 10, 2020 at 19:51
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, the10
made me waste a couple of seconds wondering why10
? ... 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 likeif 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,))
, defineself.size = 3
and use it in place of the literal3
, this way, we immediately understand what you mean (see my previous point aboutrow, col = 10, 10
) and it can be programmatically accessed from elsewhere, instead of repeating3
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,
NaN
s could easily be replaced byNone
andself.matrix[i][j]
is really not that much harder to type thanself.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 writereturn int(row), int(col)
) andwhile not self.__is_case_valid(row, col):
.Drop the
()
fromclass 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!
-
1\$\begingroup\$ This is an awesome first answer, keep it up! \$\endgroup\$user228914– user2289142020年11月11日 17:51:35 +00:00Commented Nov 11, 2020 at 17:51
-
\$\begingroup\$ @AryanParekh Thank you! \$\endgroup\$PiCTo– PiCTo2020年11月11日 19:43:19 +00:00Commented Nov 11, 2020 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– Kerdiorp2020年11月11日 20:19:07 +00:00Commented Nov 11, 2020 at 20:19
Explore related questions
See similar questions with these tags.