Infinite patterned ASCII dice

Question

After seeing Pretty print dice faces from multiple rolls of multi-sided dices, I decided to make an infinite ASCII dice generator.

There was one requirement, it to follow a normal dice face. And so I decided on making two classes, the Dice and Rings. Dice outputs the ASCII dice, holds the rings, and tells the rings how many dots to display. The rings on the other hand are just one ring of the die. For example a normal 6 sided dice has two 'rings', the outer 8 and the inner 1. So Ring decide where to put the dots.

This method allows to easily extend the dice, but keep the same base layout for the dice. That is no matter the size we will always see the same first 6 faces. (If all 6 dot's go on the same ring)

After completing the program, some bits to me look messy for example display. But I can't think of a way to make them easier to understand. Or nicer to look at.

As far as I know it works with all sized dice. But I made the algorithms with odd sized dice in mind.

---

from math import ceil
class Ring:
 def __init__(self, ring):
 self.ring = ring
 self.size = self._size(ring)
 self.index = self._build_index(self.size)
 def build(self, amount):
 indexes = set(self.index(number) for number in range(amount))
 return [index in indexes for index in range(self.size)]
 def fill(self):
 return [True for _ in range(self.size)]
 @staticmethod
 def _size(ring):
 # Alternate way to think about this:
 # max(ring ** 2 - (ring - 2) ** 2, 1)
 return max((ring - 1) * 4, 1)
 @staticmethod
 def _build_index(size):
 increment = size // 2
 columns = size // 4
 columns_increment = size // 8
 def column_addition(column):
 return (column * columns_increment + column // 2) % columns
 if not columns:
 column_addition = lambda x: x
 def get_index(number):
 addition = column_addition(number // 4)
 addition += (number % 4 > 1) * columns
 return (number * increment + addition) % size
 return get_index
class AsciiDice:
 def __init__(self, size, icons=' O'):
 self._display = self.build_display(icons)
 largest_ring = ceil(size ** 0.5)
 start = 1 if largest_ring % 2 else 2
 self._ring = largest_ring
 self.rings = list(map(
 Ring,
 reversed(range(start, largest_ring + 1, 2))
 ))
 # To be overwritten in a subclass.
 # The amount is _always_ even.
 # This is as the odd dot is added to the last result.
 def _spread_amount(self, amount):
 pass
 def _build(self, amount):
 if amount % 2:
 spread = list(self._spread_amount(amount - 1))
 spread[-1] += 1
 else:
 spread = list(self._spread_amount(amount))
 for amount, dice in zip(spread, self.rings):
 if amount == dice.size:
 yield dice.fill()
 else:
 yield dice.build(amount)
 def display(self, amount):
 array = [[None for _ in range(self._ring)] for _ in range(self._ring)]
 rings = self._build(amount)
 for r, ring in enumerate(rings):
 groups = len(ring) // 4
 x = r
 y = r
 for i, value in enumerate(ring):
 array[y][x] = value
 if i < groups:
 y += 1
 elif i < groups * 2:
 x += 1
 elif i < groups * 3:
 y -= 1
 else:
 x -= 1
 case = ['+' + '-'*self._ring + '+']
 return case + ['|' + self._display(i) + '|' for i in array] + case
 @staticmethod
 def build_display(icons):
 def change_icon(value):
 return icons[value]
 def inner(array):
 return ''.join(map(change_icon, array))
 return inner
class SpreadoutDice(AsciiDice):
 def _spread_amount(self, amount):
 rings_amount = list(map(lambda x: x.size, self.rings[:-1]))
 rings_slice = len(rings_amount)
 spread = [0 for _ in range(rings_slice)]
 while rings_slice:
 each = amount // rings_slice
 if each < 4:
 rings_slice -= 1
 continue
 each = each // 4 * 4
 for ring in range(rings_slice):
 spread[ring] += each
 amount -= each
 if spread[ring] > rings_amount[ring]:
 amount += spread[ring] - rings_amount[ring]
 spread[ring] = rings_amount[ring]
 rings_slice -= 1
 spread[0] += amount
 amount = 0
 if spread[0] > rings_amount[0]:
 amount += spread[0] - rings_amount[0]
 spread[0] = rings_amount[0]
 return spread + [amount]
class OuterDice(AsciiDice):
 def _spread_amount(self, amount):
 for ring in self.rings:
 if amount > ring.size:
 yield ring.size
 elif amount > 0:
 yield amount
 else:
 yield 0
 amount -= ring.size
# From here down, doesn't really need reviewed.
class PrintSideBySide:
 def __init__(self, amount_dice, dice_height, separator=' '):
 self.amount = amount_dice
 self.height = dice_height
 self.clear_buff()
 self.separator = separator
 def print(self):
 if all(self.buff):
 print('\n'.join(self.buff))
 def clear_buff(self):
 self.buff = ['' for _ in range(self.height)]
 self._size = 0
 def add(self, dice):
 for index, value in enumerate(dice):
 self.buff[index] += self.separator + value
 self._size += 1
 if self._size == self.amount:
 self.print()
 self.clear_buff()
if __name__ == '__main__':
 size = 3
 buff = PrintSideBySide(9, size + 2)
 dice = OuterDice(size ** 2)
 for i in range(1, size ** 2 + 1):
 buff.add(dice.display(i))
 buff.print()
 size = 5
 buff = PrintSideBySide(5, size + 2)
 dice = OuterDice(size ** 2)
 for i in range(1, size ** 2 + 1):
 buff.add(dice.display(i))
 buff.print()
 buff.clear_buff()
 dice = SpreadoutDice(size ** 2)
 for i in range(1, size ** 2 + 1):
 buff.add(dice.display(i))
 buff.print()

The output for size = 3, from the above, is:

+---+ +---+ +---+ +---+ +---+ +---+ +---+ +---+ +---+
| | |O | |O | |O O| |O O| |O O| |O O| |OOO| |OOO|
| O | | | | O | | | | O | |O O| |OOO| |O O| |OOO|
| | | O| | O| |O O| |O O| |O O| |O O| |OOO| |OOO|
+---+ +---+ +---+ +---+ +---+ +---+ +---+ +---+ +---+

In the above size = 5 shows how the different classes change the pattern, but is too long to put here.

Answer 1

A couple of high-level suggestions:

• No docstrings or comments! This generally makes code harder to read, review and maintain – you should get into the habit of writing them.

  (Trying to debug your classes without knowing what arguments they took was quite tricky.)

• Your classes should have a __repr__(). This can be really helpful for debugging. Compare and contrast:

  <__main__.Ring object at 0x10125bcf8> # default repr()
  Ring(3) # repr() thrown together quickly
  

Ring class

• Setting size to a constant in your constructor is risky, because a caller could swap out the ring attribute midway through operation. (Why is another question – just trust that people will do weird things with your classes, and code defensively.)

  Since the size attribute is easy to compute on the fly, I’d make it a method or perhaps a property instead. That way it won’t get out of date if the class is modified.

• Likewise, the index() method is liable to get out-of-date. The optimisation from precomputing it is small – just make it a proper method.]

• The variable names in the _build_index method aren’t very clear – it’s not immediately apparent what any of these variables represent. It’s also complicated by the number of functions and lambdas littering it up, which are only used once – just drop the code in directly.

• The fill() method could be written more concisely:

  return [True] * self.size
  

• I haven’t quite worked out what the size attribute is for. It might be more appropriate to use the __len__ magic method, which means I can call len(Ring(foo)) and get a meaningful result. Depends on what it’s for – just a thought.

AsciiDice class

• As before, you seem to be creating methods and then assigning them to attributes – for example, the self._display attribute. Just make them methods!

• Using an attribute named _ring just after declaring a Ring class threw me a little – I expected this attribute to be an instance of Ring, but it seems to be just a number. That could be better named.

• The list(map( to construct the rings attribute is quite hard to read; a list comprehension would be better:

  self.rings = [Ring(r) for r in reversed(range(start, largest_ring + 1, 2))]
  

  You can likewise use a list comprehension to tidy up the inner() function in the _build_display method:

  return ''.join(change_icon(elem) for elem in array)
  

• Within the _build_display method, the name of the change_icons() function is misleading – it just gets a value, whereas it sounds like it’s going to cause some changes! It should be renamed, or better, thrown away – it’s so simple that you don’t need it. Now inner() becomes:

  return ''.join(icons[value] for value in array)
  

• If the _spread_amount method is to be defined in a subclass, it should raise a NotImplementedError in the base class – this ensures a subclasser cannot forget to implement this method.

• The display() method is very confusing. You can tidy up the array initialiser:

  array = [[None] * self._ring] * self._ring
  

  although pick a better name; array is hopelessly generic.

  Once again we have the name ring being used for structures that don’t appear to be Ring instances – for example, we have a len() call on a class that doesn’t define __len__.

SpreadoutDice class

• The list, map and lambda on the same line is unnecessarily obfuscated and confusing. Consider instead:

  rings_amount = [x.size for x in self.rings[:-1]]
  

  Isn’t that cleaner?

• Peilonrayz: Thank you for the review! Since [[None] * self._ring] * self.ring gives wrong output, you could change it to [[None] * self._ring for _ in range(self._ring)]. This is why I prefer list comprehensions...