HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

TicTacToe (again!), but with difficulty levels, undo, and hints

Submitted by: @import:stackexchange-codereview··
0
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

Solution

You wrap your public variables and functions into another class, called Game or TicTacToe. This way you can instantiate your game by just importing the class, then doing

game = 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 USER


It 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 USER
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))
for cell in cells:
    cell._reset()

Context

StackExchange Code Review Q#160365, answer score: 3

Revisions (0)

No revisions yet.