Infinite patterned ASCII dice

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)
additio

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:

# 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?

StackExchange Code Review Q#111445, answer score: 2