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
Graipher
41.6k7 gold badges70 silver badges134 bronze badges
asked Apr 12, 2017 at 11:12
\$\endgroup\$

2 Answers 2

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 1 ball always goes first and the 8 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
answered Apr 12, 2017 at 19:22
\$\endgroup\$
1
  • \$\begingroup\$ Simply wow. Another amazing input and fuel to learn and program for countless time. I deeply thank you! \$\endgroup\$ Commented Apr 13, 2017 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
answered Apr 12, 2017 at 13:17
\$\endgroup\$
2
  • 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\$ Commented Apr 12, 2017 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\$ Commented Apr 12, 2017 at 18:55

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.