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
2 Answers 2
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 thePool
class, not of eachPool
instance. - The convention of using
1
and0
to represent solids and stripes is unnecessarily cryptic and necessitates comments like1 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 toball.n
andball.coloration
instead of magic indices likeball[0]
andball[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
-
\$\begingroup\$ Simply wow. Another amazing input and fuel to learn and program for countless time. I deeply thank you! \$\endgroup\$Pitto– Pitto2017年04月13日 07:54:47 +00:00Commented Apr 13, 2017 at 7:54
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
-
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– Pitto2017年04月12日 13:39:17 +00:00Commented 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\$200_success– 200_success2017年04月12日 18:55:42 +00:00Commented Apr 12, 2017 at 18:55