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

Question

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!

Answer 1

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.