2

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?

Alex Lopatin
  • 682
  • 4
  • 8
  • What do you mean by "[the original version] has to be broken"? Why isn't it acceptable in this form? – meowgoesthedog Apr 29 '19 at 10:21
  • 1
    It is not *more pythonic* if it looks bad. Pythons philosophy is to keep it readable and simple. If regular `if` statement looks better that ternary `if else` - use regular `if`. **Simple is better than complex**. – Yevhen Kuzmovych Apr 29 '19 at 10:23
  • 1
    You could extract the array generation `[index for index, square in enumerate(self._board) if square == TicTacToe.square_free]` to a separate method and call that. – nitzel Apr 29 '19 at 10:23
  • @meowgoesthedog: Limit all lines to a maximum of 79 characters. https://www.python.org/dev/peps/pep-0008/#maximum-line-length – Alex Lopatin Apr 29 '19 at 11:00
  • 1
    also a small detail, but I'd name the function `possible_moves()` or `list_possible_moves()` since the current name makes me think that it's a variable. –  Apr 29 '19 at 11:08
  • Interesting. I used it as a verb, but I think you are right. I will rename it. Thank you. – Alex Lopatin Apr 29 '19 at 11:17

4 Answers4

5

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
    ]
AKX
  • 152,115
  • 15
  • 115
  • 172
  • 1
    this is very nice, the `return []` also makes the function skip the rest if `self._state` is not `Game.play` –  Apr 29 '19 at 10:30
  • I got three answers. The rest two are similar to your answer. I will accept yours since it was the first, plus bonus: black code formatter :-) Thank you! – Alex Lopatin Apr 29 '19 at 10:45
  • 1
    Tooting my own horn a little here – if you want to run Black without installing, check out my https://nicen.pw/ site :) – AKX Apr 29 '19 at 11:52
3

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.

Right leg
  • 16,080
  • 7
  • 48
  • 81
3

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
Netwave
  • 40,134
  • 6
  • 50
  • 93
3

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!).

Karl Knechtel
  • 62,466
  • 11
  • 102
  • 153
  • 1
    You are right. Great observation! Actually, I don't need to check `self._state` after you noticed it. I only call `list_of_possible_moves` in the code when `self._state == Game.play`.Thank you! – Alex Lopatin Apr 29 '19 at 10:55
  • Also gained 0.03 second in 10000 games :-) – Alex Lopatin Apr 29 '19 at 11:33