8
\$\begingroup\$

I have written this code for fun. I'd like to hear your suggestions about how to make it more compact and pythonic.

from random import shuffle

class Pool(object):
    """
    Simple object that allows to sort
    """

    def __init__(self):
        """
        During initalization the final grid containing
        ordered balls is created
        1 is placeholder for Solid balls
        0 is placeholder for Striped balls
        """
        self.grid = [[1],[0,0],[1,0,1],[0,1,0,0],[1,0,1,0,1]]

    def create_ball_set(self):
        """
        This function returns a list containing
        a shuffled pool balls set
        """
        balls = []
        [balls.append([number]) for number in range(1, 16)]
        for ball in balls:
            if ball[0] < 9:
                ball.append("Solid")
            else:
                ball.append("Striped")
        shuffle(balls)
        return balls

    def sort_ball_set(self, unsorted_balls):
        """
        This function returns a
        list of sorted balls from a list of
        shuffled balls
        """
        # Ball 1 always goes in 1st place
        self.grid[0][0] = unsorted_balls.pop(unsorted_balls.index([1, 'Solid']))
        # Ball 8 always goes in the 2nd row, in the middle
        self.grid[2][1] = unsorted_balls.pop(unsorted_balls.index([8, 'Solid']))
        # Creating an empty list for solid balls
        unsorted_solid_balls = []
        # Same thing but for striped balls
        unsorted_striped_balls = []

        # Now it is time to divide solid balls from striped ones
        for ball in unsorted_balls:
            if ball[1] == 'Solid':
                unsorted_solid_balls.append(ball)
            elif ball[1] == 'Striped':
                unsorted_striped_balls.append(ball)

        # Once the balls are divided it is time to put them in the grid
        for grid_row_index, grid_row in enumerate(self.grid):
            for grid_col_index, grid_col_value in enumerate(grid_row):
                # In their corresponding placeholder spot
                if grid_col_value == 1:
                    # 1 is for solid balls
                    self.grid[grid_row_index][grid_col_index] = unsorted_solid_balls.pop()
                elif grid_col_value == 0:
                    # 0 is for striped balls
                    self.grid[grid_row_index][grid_col_index] = unsorted_striped_balls.pop()
        return self.grid

pool_table = Pool()
print "Getting new set of balls from the pool table:"
unsorted_balls = pool_table.create_ball_set()
print unsorted_balls
print "Sorted balls:"
sorted_balls = pool_table.sort_ball_set(unsorted_balls)
# Pretty printing sorted balls
for row in sorted_balls:
    print row
\$\endgroup\$
8
\$\begingroup\$

There are several aspects of your self.grid that I don't like:

  • It is constant, so it should be in ALL_CAPS.
  • It can be shared by all instances of Pool, so it should be an attribute of the Pool class, not of each Pool instance.
  • The convention of using 1 and 0 to represent solids and stripes is unnecessarily cryptic and necessitates comments like 1 is placeholder for Solid balls and # 1 is for solid balls. Everywhere else in your class, you are using strings 'Solid' and 'Striped', which is more obvious.
  • It neglects to specify that the ① ball always goes first and the ⑧ ball always goes in the middle. These two balls then have to be handled as exceptional cases in sort_ball_set().

It's hard to say what your Pool class represents. Each instance doesn't really have any object state. (As mentioned above, self.grid doesn't count.) You're basically just using it as a kind of namespace. I would repurpose it to represent a list of balls.


I don't think that it is a good idea to represent each ball as a list of length 2:

  • Lists are most suited for storing homogeneous items of arbitrary length. An object with a fixed number of attributes, each of a different type, would be better represented using a tuple.
  • The task already necessitates dealing with two-dimensional lists. Introducing more indexing makes the code more cumbersome and less readable.
  • For even better readability, you should use a namedtuple, so that you can refer to ball.n and ball.coloration instead of magic indices like ball[0] and ball[1].

In create_ball_set(), this is an abuse of a list comprehension solely for its side-effects:

balls = []
[balls.append([number]) for number in range(1, 16)]

The weirdness is more apparent if you run it in the REPL:

>>> balls = []
>>> [balls.append([number]) for number in range(1, 16)]
[None, None, None, None, None, None, None, None, None, None, None, None, None, None, None]
>>> balls
[[1], [2], [3], [4], [5], [6], [7], [8], [9], [10], [11], [12], [13], [14], [15]]

The conventional expression should be balls = [[n] for n in range(1, 16)].


Suggested solution

I would put more work into defining a TEMPLATE that fully describes what the triangle has to look like.

from collections import namedtuple
import random

PoolBall = namedtuple('PoolBall', 'n coloration')

class PoolBalls(object):
    def filter(criterion):
        return (lambda ball: ball.n == criterion) if criterion in (1, 8) else \
               (lambda ball: ball.n not in (1, 8) and ball.coloration == criterion)

    TEMPLATE = [[filter(criterion) for criterion in row] for row in [
                                  [1],
                         ['Striped', 'Striped'],
                     ['Solid',     8,      'Solid'],
               ['Striped', 'Solid', 'Striped', 'Striped'],
            ['Solid', 'Striped', 'Solid', 'Striped', 'Solid'],
        ]
    ]

    def __init__(self):
        self.balls = [PoolBall(n, 'Solid') for n in range(1, 9)] + \
                     [PoolBall(n, 'Striped') for n in range(9, 16)]

    def shuffle(self):
        random.shuffle(self.balls)

    def arrange(self):
        balls = self.balls[::-1]
        def pick(filter):
            for i in range(len(balls) - 1, -1, -1):
                if filter(balls[i]):
                    return balls.pop(i)
        return [[pick(filter) for filter in row] for row in self.TEMPLATE]

if __name__ == '__main__':
    print "Getting new set of balls from the pool table:"
    pool_table = PoolBalls()
    pool_table.shuffle()
    print pool_table.balls
    print "Arranged balls:"
    for row in pool_table.arrange():
        print row
\$\endgroup\$
  • \$\begingroup\$ Simply wow. Another amazing input and fuel to learn and program for countless time. I deeply thank you! \$\endgroup\$ – Pitto Apr 13 '17 at 7:54
10
\$\begingroup\$

I think it is usually better to take advantage of the built-in functions. Python lists already have a sort function, so you would expect a list of pool balls to be sortable by the start arrangement (a list of tuples/lists is already sortable, Python compares them element wise).

One way to achieve that is with a custom key function:

order = [1, 13, 11, 7, 8, 6, 9, 5, 14, 15, 2, 12, 3, 10, 4]
# Simplified balls list, as the fill style is not really needed for this
balls = [[n] for n in range(1, 16)]
balls.sort(key=lambda ball: order.index(ball[0]))

This is very succinct, but you could also promote the lambda key function to a proper function:

def pool_start_order(ball):
    order = [1, 13, 11, 7, 8, 6, 9, 5, 14, 15, 2, 12, 3, 10, 4]
    return order.index(ball[0])

balls = [[n] for n in range(1, 16)]
balls.sort(key=pool_start_order)

Another alternative is to make a PoolBall class, that is sortable, something like this:

class PoolBall(object):
    order = [1, 13, 11, 7, 8, 6, 9, 5, 14, 15, 2, 12, 3, 10, 4]

    def __init__(self, number):
        assert 1 <= number <= 15
        self.number = number
        self.fill = 'Solid' if number <= 8 else 'Striped'
        self.order = PoolBall.order.index(number)

    def __lt__(self, other):
        return self.order < other.order

    def __eq__(self, other):
        return self.number == other.number

    def __str__(self):
        return "{self.number} {self.fill}".format(self=self)

    def __repr__(self):
        return "{self.__class__.__name__}({self.number})".format(self=self)


    balls = [PoolBall(i) for i in range(1, 16)]
    balls.sort()

Here, the __str__ and __repr__ are just there for pretty printing.

Whichever way you go, once you have a correctly sorted list of balls, you can use a function that successively takes one more ball, something like:

def pyramid_arrangement(balls):
    """
    Given a list of balls, arranges them in a pyramid shape.
    Yields each row.

    Leaves the balls list empty.
    """
    take = 1
    while balls:
        yield [balls.pop() for _ in range(take)]
        take += 1

Which you can use like this:

if __name__ == "__main__":
    balls = [PoolBall(i) for i in range(1, 16)]
    balls.sort(reverse=True)
    for row in pyramid_arrangement(balls):
        print row
\$\endgroup\$
  • 1
    \$\begingroup\$ This is simply great. I've quickly read it twice and I had the chance to stare ad the abyss of my ignorance. Once recovered I'll put serious time into understanding your code. Thanks for your time, it is highly appreciated. \$\endgroup\$ – Pitto Apr 12 '17 at 13:39
  • \$\begingroup\$ I don't understand what you are doing with order = [1, 13, 11, …, 4]. Won't that result in exactly the same specific arrangement every time? \$\endgroup\$ – 200_success Apr 12 '17 at 18:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.