5
\$\begingroup\$

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.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

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?

\$\endgroup\$
1
  • \$\begingroup\$ 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... \$\endgroup\$
    – Peilonrayz
    Nov 28, 2015 at 22:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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