patternpythonMinor
TicTacToe (again!), but with difficulty levels, undo, and hints
Viewed 0 times
withtictactoebutandhintslevelsdifficultyagainundo
Problem
I'm learning Python after a 30+ year break from programming, got bored with the usual TicTacToe exercise, so went a bit wild adding functionality to it to make something that's a bit more fun to play than losing to the computer every time.
Now reached the stage of aimlessly noodling around with it, which suggests to me that fresh, and experienced, eyes would be handy. I would very much welcome a review of any/all aspects of the code. Be brutal - I can take it!
A few points:
-
It is written as a module, as I'm rubbish at UIs and I want to use this to practise with. Would it help if I also posted something that used the module? I have a fairly scraggy tkinter thing that I am using as a testbed if that would help.
-
It works. Rather nicely I think - in fact I am quite proud of it. This of course is a Big Red Flag that a code review is needed ;)
-
Nevertheless, it is possible that I have grossly abused something-or-other, but I've tried hard to get PEP8/PEP257 stuff right, and to comment lightly and selectively. So it shouldn't be too hard to work out what I am driving at. I hope.
-
It's a bit big - runs to 8 pages printed, sorry about that.
```
"""Module to support playable versions of TicTacToe (Noughts and Crosses)
by phisheep 2017
VARIABLES (all should be treated as read-only)
- USER, COMPUTER, EMPTY - Player objects
- cells[] - list of Cell objects representing the playing board
- lines[] - list of Line objects ditto
- moves[] - list of Cell objects representing moves made in current game
- game_over - boolean
- winning_line - Line object, or None
- winning_player - Player object, or None
CLASSES
- Player() - One for each player, and EMPTY
player.symbol - arbitrary symbols for each player, default 'X', 'O' and ' '
player.opponent - the other (non-EMPTY) player
- Cell() - One for each cell on the board
cell.lines[] - list of Lines this cell is in
cell.set(player) - sets a cell and chec
Now reached the stage of aimlessly noodling around with it, which suggests to me that fresh, and experienced, eyes would be handy. I would very much welcome a review of any/all aspects of the code. Be brutal - I can take it!
A few points:
-
It is written as a module, as I'm rubbish at UIs and I want to use this to practise with. Would it help if I also posted something that used the module? I have a fairly scraggy tkinter thing that I am using as a testbed if that would help.
-
It works. Rather nicely I think - in fact I am quite proud of it. This of course is a Big Red Flag that a code review is needed ;)
-
Nevertheless, it is possible that I have grossly abused something-or-other, but I've tried hard to get PEP8/PEP257 stuff right, and to comment lightly and selectively. So it shouldn't be too hard to work out what I am driving at. I hope.
-
It's a bit big - runs to 8 pages printed, sorry about that.
```
"""Module to support playable versions of TicTacToe (Noughts and Crosses)
by phisheep 2017
VARIABLES (all should be treated as read-only)
- USER, COMPUTER, EMPTY - Player objects
- cells[] - list of Cell objects representing the playing board
- lines[] - list of Line objects ditto
- moves[] - list of Cell objects representing moves made in current game
- game_over - boolean
- winning_line - Line object, or None
- winning_player - Player object, or None
CLASSES
- Player() - One for each player, and EMPTY
player.symbol - arbitrary symbols for each player, default 'X', 'O' and ' '
player.opponent - the other (non-EMPTY) player
- Cell() - One for each cell on the board
cell.lines[] - list of Lines this cell is in
cell.set(player) - sets a cell and chec
Solution
You wrap your public variables and functions into another class, called
and having that class control the game.
Right now you are basically treating your global namespace like a class, with global functions as classes, and
The constructor of your
The
It returns the
Having classes for
This is hard to read and seems a bit like a math puzzle:
Rather write 6 lines (of code) instead of these 4 and write the values excplicitly. Using for loops here hardly gives you any value, neither concerning readability nor performance.
In
But remember that method names starting with underscores are used for private methods, and thus should not be called from outside of the class. Using a
Game or TicTacToe. This way you can instantiate your game by just importing the class, then doinggame = TicTacToe()
game.start()and having that class control the game.
Right now you are basically treating your global namespace like a class, with global functions as classes, and
__init__ as the constructor, that you call excplicitly. Functions/methods with leading and trailing double underscores should normally never be called explicitly.The constructor of your
Player class takes a parameter value. What is that value? Better use a name that tells you what kind of value it is (e. g. level, age, etc.).The
Player class has this property:@property
def opponent(self):
if self == USER:
return COMPUTER
return USERIt returns the
USER instance of Player in any case where it is not the USER itself. But what if you ask the EMPTY player that you are using for empty fields what its opponent is? Maybe you could instead use a variable to save the opponent and set it where you instantiate the players. It is not good practice to have the internal behaviour of classes depend on global state.Having classes for
Line and Cell seems a bit too much. Rather have a class Board which saves the positions of the players' symbols and checks when the winning-condition is fulfilled. If you want to use the Line and Cell classes to better organize your code, make them private classes of the Board, so that no user (even you) of your code creates instances of them where they are not needed.This is hard to read and seems a bit like a math puzzle:
for i in range(0, 9, 3): # rows
lines.append(Line(i, i + 1, i + 2))
for i in range(3): # columns
lines.append(Line(i, i + 3, i + 6))Rather write 6 lines (of code) instead of these 4 and write the values excplicitly. Using for loops here hardly gives you any value, neither concerning readability nor performance.
In
reset_board you call this:for cell in cells:
cell._reset()But remember that method names starting with underscores are used for private methods, and thus should not be called from outside of the class. Using a
Board class enables you to add a single reset() method to it that resets all cells and lines.Code Snippets
game = TicTacToe()
game.start()@property
def opponent(self):
if self == USER:
return COMPUTER
return USERfor i in range(0, 9, 3): # rows
lines.append(Line(i, i + 1, i + 2))
for i in range(3): # columns
lines.append(Line(i, i + 3, i + 6))for cell in cells:
cell._reset()Context
StackExchange Code Review Q#160365, answer score: 3
Revisions (0)
No revisions yet.