Python: does it make sense to refactor this check into its own method?

I'm still learning python. I just wrote this method to开发者_如何转开发 determine if a player has won a game of tic-tac-toe yet, given a board state like: '[['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 check lists of 3 chars several times and could refactor the checking to its own method like so:

def check(list, player):
  if list.count(player) == 3:
    return player

...but then realized that all that really does is change lines like:

 if [ top[i],mid[i],low[i] ].count(player) == 3:
    return player

to:

  if check( [top[i],mid[i],low[i]], player ):
    return player

...which frankly doesn't seem like much of an improvement. Do you see a better way to refactor this? Or in general a more Pythonic option? I'd love to hear it!

I might use

def check(somelist, player):
  return somelist.count(player) == 3

Edit: as @Andrew suggested in a comment (tx @Andrew!), you can do even better, e.g.:

def check(somelist, player):
  return somelist.count(player) == len(somelist)

without hardcoding the 3 -- which also suggests another nice alternative:

def check(somelist, player):
  return all(x==player for x in somelist)

which very directly reads "all items in the list equal player". The general point is that by refactoring to a separate method you can then play with that method's implementation -- now of course here the code is very simple so the advantage is similarly modest, but it's an excellent point to keep in mind as you move to more complicated code.

As you've noticed you only need a bool anyway, so this allows a much simpler approach -- just return the bool expression rather than doing an if on it. It's important to never use a built-in name like list for your own identifiers -- an "attractive nuisance" of the language...;-).

By which I mean, Python uses for its built-ins lots of nice, attractive names like list, bool, sum, and so on, so it's easy to find yourself accidentally using one of those names for a variable of your own, and nothing bad seems to happen... until the time you need to turn, say, a tuple into a list, use the obviously best solution, x = list(thetuple)... and end up spending our trying to understand and solve the errors that come because you've used list to mean anything else than the built-in type of that name.

So, just get into the habit of not using those nice built-in names for purposes other than indicating the respective builtins, and you'll save yourself much future aggravation!-)

Back to your code, you might consider the conciseness afforded by not unpacking board (a hard decision, since your code is quite readable... but may look a bit 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'd stick with your choice -- more readable and just marginally more verbose, if at all -- but it's nice to be aware of the alternatives, I think -- here, list comprehensions and enumerate to generate the lists to be checked as an alternative to "manually coding out" the three possibilities.

You could use a better name instead of check that doesn't tell much. The rule of thumb is: if you can think of a good name for a peace of code then it might be beneficial to move it into separate function even if it is just one line of code. allsame might be one of alternatives 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)

Simply make a custom 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 these really should be methods of a board class :)

Personally I think your best bet for readability is to bubble out functions to give you the rows(), columns(), and diags() of the board, as lists of lists. Then you can iterate through these and check uniformly. You can even then define allTriples(), which appends the output of rows(), columns(), and diags(), so that you can do your checking in a single concise loop. I'd probably also make the board an object, so that these functions could become object methods.

And now for something completely different:

Represent the board by 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

Refinement:

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

Just an idea

def hasWon(board):
  players = ['x', 'o']
  for player in players:
    top, mid, low = board
    game = board + [[ top[i],mid[i],low[i]] for i in range(3)] + [top[0],mid[1],low[2]] +[top[2],mid[1],low[0]]
    if 3 in [l.count(player) for l in game] :
      return player
  return None

Your solution is fine - correct, readable and understandable.

Still, if you'd want to optimize for speed, I'd use a 1-dimensional array of digits, not strings, and try to look up each number as few times as possible. There will certainly be an extremely awkward-looking solution in which you check every field only once. I don't want to construct this now. :) Things like that might matter if you wanted to implement an AI playing against you while exploring the entire search tree of possible moves. An efficient win/loss check would be necessary there.