Python: does it make sense to refactor this check into your own method?
I am still learning python. I just wrote this method to determine if a player won the tic-tac-toe game given the state of the board:'[['o','x','x'],['x','o','-'],['x','o','o']]'
def hasWon(board):
players = ['x', 'o']
for player in players:
for row in board:
if row.count(player) == 3:
return player
top, mid, low = board
for i in range(3):
if [ top[i],mid[i],low[i] ].count(player) == 3:
return player
if [top[0],mid[1],low[2]].count(player) == 3:
return player
if [top[2],mid[1],low[0]].count(player) == 3:
return player
return None
It occurred to me that I was checking for 3-character lists multiple times and could refactor the check to my own method like this:
def check(list, player):
if list.count(player) == 3:
return player
... but then realized that all it really does is change lines like:
if [ top[i],mid[i],low[i] ].count(player) == 3:
return player
at
if check( [top[i],mid[i],low[i]], player ):
return player
... which, frankly, doesn't seem like an improvement. Do you see a better way to refactor this? Or even a more pythonic version? I would love to hear it!
a source to share
I could use
def check(somelist, player):
return somelist.count(player) == 3
Edit : like @Andrew suggested in the comment (tx @Andrew!) You can do even better, like this:
def check(somelist, player):
return somelist.count(player) == len(somelist)
without hardcoding 3
, which also offers another nice alternative:
def check(somelist, player):
return all(x==player for x in somelist)
which reads very directly "all items in the list are equal player
". The general point is that by refactoring to a single method, you can play with the implementation of that method - now, of course, the code here is very simple, so the advantage is similarly modest, but this is a great point to keep in mind when moving to more complex code.
As you noticed, you need a bool anyway, so this allows a much simpler approach - just return the bool expression instead of doing it if
on it. It is important to never use a built-in name, for example list
for your own identifiers - the "attractive nuisance" of the language ...; -).
By this I mean that Python uses a lot of nice, attractive names for its builtins like list, bool, sum, etc., so it's easy to find yourself by accident using one of these names for a variable of your own, and nothing bad happens ... until you need to turn, say, a tuple into a list, use a clearly better solution, x = list(thetuple)
... and end up wasting our efforts to figure out and solve errors is because you used list
to mean something other than the built-in type of this name.
So, just get in the habit of not using these nice built-in names for purposes other than the corresponding built-in functions, and you will save a lot of escalation time! -)
Back to your code, you might think of brevity, which you don't unpack board
(a tough decision since your code quite readable ... but might look a little verbose):
for i in range(3):
if check([row[i] for row in board], player):
return player
if check([row[i] for i, row in enumerate(board)], player):
return player
if check([row[2-i] for i, row in enumerate(board)], player):
return player
In the end, I think I'm sticking with your choice - more readable and slightly more verbose, if at all, but I'm pleased to know about the alternatives, I think - here, list comprehension and enumerate
for generating lists to be checked as an alternative to "manual coding "three possibilities.
a source to share
Just create your own iterator over board
.
def get_lines(board):
nums = range(3)
for i in nums:
yield (board[i][j] for j in nums) #cols
for j in nums:
yield (board[i][j] for i in nums) #rows
yield (board[i][i] for i in nums) #diag \
yield (board[i][2-i] for i in nums) #diag /
def get_winner(board): #a bit too indented
for line in get_lines(board): #more expensive, so go through it only once
for player in 'x', 'o':
if line == player, player, player: #other way to check victory condition
return player
return None
Obviously it really should be class methods board
:)
a source to share
Instead, check
you can use a better name that says little. Rule of thumb: If you can think of a good name for the world of code, then it might be helpful to move it into a separate function, even if it's just one line of code. allsame
could be an alternative here.
def winner(board):
main_diag = [row[i] for i, row in enumerate(board)]
aux_diag = [row[len(board) - i - 1] for i, row in enumerate(board)]
for triple in board + zip(*board) + [main_diag, aux_diag]:
if allsame(triple):
return triple[0]
def allsame(lst):
return all(x == lst[0] for x in lst)
a source to share
Personally, I think your best bet for readability is to bubble functions to give you lines (), columns () and diags () on a board like lists of lists. Then you can go through them and check evenly. You can even define allTriples (), which adds rows (), columns (), and diags () outputs so you can complete your validation in one concise loop. I'll probably also make the board an object so that these functions can become object methods.
a source to share
And now for something completely different:
Introducing the board as a list of nine elements. Each element can be -1 (X), 1 (O), or 0 (empty):
WIN_LINES = (
(0, 1, 2),
(3, 4, 5),
(6, 7, 8),
(0, 3, 6),
(1, 4, 7),
(2, 5, 8),
(2, 4, 6),
(0, 4, 8),
)
def test_for_win(board):
for line in WIN_LINES:
total = sum(board[point] for point in line)
if abs(total) == 3:
return total // 3
return None
Clarification:
WIN_LINES = (
0, 1, 2,
3, 4, 5,
6, 7, 8,
0, 3, 6,
1, 4, 7,
2, 5, 8,
2, 4, 6,
0, 4, 8,
)
def test_for_win(board):
wpos = 0
for _unused in xrange(8):
total = board[WIN_LINES[wpos]]; wpos += 1
total += board[WIN_LINES[wpos]]; wpos += 1
total += board[WIN_LINES[wpos]]; wpos += 1
if total == 3: return 1
if total == -3: return -1
return None
a source to share
Your solution is in order - correct, readable and understandable.
However, if you want to optimize for speed, I would use a 1-dimensional array of digits, not strings, and try to find each number as small as possible. Of course, there will be an extremely inconvenient solution in which you check each field only once. I don't want to build it now. :) This kind of thing can make a difference if you want to implement the AI against you, exploring the entire search tree for possible moves. There will need to be an effective win / loss check.
a source to share