I am trying to be more Pythonic and less C++. However the limit of all lines to a maximum of 79 characters creates everyday struggle. Here is one example:

def list_of_possible_moves(self):  # version 1
    return [] if self._state != Game.play else [index for index, square in enumerate(self._board) if square == TicTacToe.square_free]

It is 138 characters long and has to be broken. (1) In three lines:

def list_of_possible_moves(self):  # version 2
    return [] if self._state != Game.play else\
        [index for index, square in enumerate(self._board)
         if square == TicTacToe.square_free]

(2) Possible in two lines (i, s instead index, square). Still 80, not 79:

def list_of_possible_moves(self):  # version 3
    return [] if self._state != Game.play else\
        [i for i, s in enumerate(self._board) if s == TicTacToe.square_free]

(3) In the end, I decided to keep that version:

def list_of_possible_moves(self): # version 4 (C++)
    if self._state is not Game.play:
        return []
    list_of_moves = []
    for index, square in enumerate(self._board):
        if square == TicTacToe.square_free:
            list_of_moves.append(index)
    return list_of_moves

What do I do wrong? Any better way to write this piece of code?

4 Answers

6
AKX On Best Solutions

This is a style/taste question; when in doubt, use the black code formatter on your code; if it looks ugly after Black, split it into multiple statements.

I'd "split the difference" and take care of the special case first, then use a list comprehension.

def list_of_possible_moves(self):
    if self._state is not Game.play:  # Not playing, no moves possible.
        return []
    return [
        index
        for index, square in enumerate(self._board)
        if square == TicTacToe.square_free
    ]
4
Right leg On

First, you can expand your if, else one-liner:

def list_of_possible_moves(self):
    if self._state != Game.play:
        return []
    else:
        return [index for index, square in enumerate(self._board) if square == TicTacToe.square_free]

But it's still a bit longer. A good way to break a comprehension is value - for - condition, as suggested by the Google styleguide:

def list_of_possible_moves(self):
    if self._state != Game.play:
        return []
    else:
        return [
            index 
            for index, square in enumerate(self._board) 
            if square == TicTacToe.square_free
        ]

But if you don't want to expand it, you can rename index into i, which is perfectly valid in 1. a comprehension, and 2. with enumerate. You can rename square as well:

def list_of_possible_moves(self):
    if self._state != Game.play:
        return []
    else:
        return [i for i, sq in enumerate(self._board) if sq == TicTacToe.square_free]

Here it's 85 character long, so just a little too long, but this can help reduce your lines in general.

4
Netwave On

The long version is the most redeable one, you can use comprehensions pipeline anyway:

def list_of_possible_moves(self): # version 4 (C++)
    if self._state is not Game.play:
        return []
    non_squares_indexes = [
        i for i, x in enumerate(self._board) if x == TicTacToe.square_free
    ]
    return non_squares_indexes
4
Karl Knechtel On

Thinking outside the box for a bit - consider whether the self._state check really belongs at this level of the code. If your class needs to check self._state in multiple places, that's a smell suggesting that you replace the conditional logic with polymorphism. Now you have a separate class for the main gameplay state, which determines the possible moves via the comprehension; and other classes for other game states that just give you the empty list - assuming that you really need this to be part of the interface (consider whether you really need to be able to check the list of available moves when the game is not being played!).