5
\$\begingroup\$

I'm new to programming and this is the first thing I'm doing on my own. I would appreciate it if you could point me ways to optimize this RPG dice roller code in Python!

import random

def dice_reader():

##this should read messages like '2 d 6 + 3, 5 d 20 + 2 or just 3 d 8'##
    message = input('>')
    reader = message.split(' ')
    times = reader[0]
    sides = reader[2]
    output_with_modifier = []
    result = []

    if len(reader) == 5:
        modifier = reader[4]
    else:
        modifier = 0

    for output in range(int(times)):
        output = random.randint(1, int(sides))
        result.append(output)
        output_with_modifier = [(int(x) + int(modifier)) for x in result]
    print(f' Dice rolls: {tuple(result)}')
    if modifier != 0:
        print(f' With modifier: {tuple(output_with_modifier)}')


    end = False
    while end == False:
        dice_reader()
        end_message = input('Again? ')
        if end_message.lower() == 'no':
            end = True
    else:
        pass
\$\endgroup\$
  • \$\begingroup\$ I've taken the liberty of removing the performance tag, because nothing here will challenge your CPU or memory (even remotely). \$\endgroup\$ – Reinderien Jul 25 at 2:10
4
\$\begingroup\$

Mystery inputs

It's clear that there's an implied structure to this input:

message = input('>')
reader = message.split(' ')

requiring between four and five tokens. I have no idea what those are, and neither does your user. Replace '>' with an actual description of what's expected here.

Unpacking

times = reader[0]
sides = reader[2]

can be

times, _, sides = reader[:3]

though the discarded second item is suspicious. You do need to show what this is, and it probably shouldn't be there.

Looping and overwrites

This:

for output in range(int(times)):
    output_with_modifier = [(int(x) + int(modifier)) for x in result]

does not make sense. If you ask for 100 times, output_with_modifier will be calculated 100 times and thrown away 99 of them. Only the last value will be kept. You probably want to de-indent that last assignment so that it happens outside of the loop.

More looping

end = False
while end == False:
    dice_reader()
    end_message = input('Again? ')
    if end_message.lower() == 'no':
        end = True
else:
    pass

First, delete that else; pass - it isn't doing anything. Also, end == False should be not end; but you shouldn't be using a termination variable at all. If you find a no, simply break.

Suggested code

Some of this may challenge a beginner, but I like to think that CodeReview is for "aspiring advanced programmers". I've tried to comment it extensively, but feel free to ask questions in the comments.

import re
from random import randint
from re import Pattern
from typing import ClassVar, Iterable


class Dice:
    """
    One specification for dice rolls in Dungeons & Dragons-like format.
    """

    def __init__(self, times: int, sides: int, modifier: int = 0):
        if times < 1:
            raise ValueError(f'times={times} is not a positive integer')
        if sides < 1:
            raise ValueError(f'sides={sides} is not a positive integer')

        self.times, self.sides, self.modifier = times, sides, modifier

    # This is a class variable (basically a "static") that only has one copy
    # for the entire class type, rather than a copy for every class instance
    # It is a regular expression pattern that will allow us to parse user
    # input.
    INPUT_PAT: ClassVar[Pattern] = re.compile(
        # From the start, maybe some whitespace, then a group named "times"
        # that contains one or more digits
        r'^\s*(?P<times>\d+)'    
        
        # Maybe some whitespace, then the letter "d"
        r'\s*d'
        
        # Maybe some whitespace, then a group named "sides" that contains one
        # or more digits
        r'\s*(?P<sides>\d+)'
        
        # The beginning of a group that we do not store.
        r'(?:'
        
            # Maybe some whitespace, then a "+" character
            r'\s*\+'
        
            # Maybe some whitespace, then a group named "modifier" that
            # contains one or more digits
            r'\s*(?P<modifier>\d+)'
        
        # End of the group that we do not store; mark it optional
        r')?'
        
        # Maybe some whitespace, then the end.
        r'\s*$',

        # We might use "d" or "D"
        re.IGNORECASE
    )

    # This can only be called on the class type, not a class instance. It
    # returns a new class instance, so it acts as a secondary constructor.
    @classmethod
    def parse(cls, message: str) -> 'Rolls':
        match = cls.INPUT_PAT.match(message)
        if match is None:
            raise ValueError(f'Invalid dice specification string "{message}"')

        # Make a new instance of this class based on the matched regular
        # expression.
        return cls(
            int(match['times']),
            int(match['sides']),
            # If there was no modifier specified, pass 0.
            0 if match['modifier'] is None else int(match['modifier']),
        )

    @classmethod
    def from_stdin(cls) -> 'Rolls':
        """
        Parse and return a new Rolls instance from stdin.
        """

        while True:
            try:
                message = input(
                    'Enter your dice specification, of the form\n'
                    '<times>d<sides> [+ modifier], e.g. 3d6 or 4d12 + 1:\n'
                )
                return cls.parse(message)
            except ValueError as v:
                print(v)
                print('Please try again.')

    def roll(self, with_modifier: bool = False) -> Iterable[int]:
        """
        Return a generator of rolls. This is "lazy" and will only execute the
        rolls that are consumed by the caller, because it returns a generator
        (not a list or a tuple).
        """
        mod = self.modifier if with_modifier else 0
        return (
            randint(1, self.sides) + mod
            for _ in range(self.times)
        )

    def print_roll(self):
        print(
            'Dice rolls:',
            ', '.join(str(x) for x in self.roll()),
        )

        if self.modifier != 0:
            print(
                'With modifier:',
                ', '.join(str(x) for x in self.roll(with_modifier=True)),
            )


def test():
    """
    This is an automated test method that does some sanity checks on the Dice
    implementation.
    """
    
    d = Dice.parse('3 d 6')
    assert d.times == 3
    assert d.sides == 6
    assert d.modifier == 0

    d = Dice.parse('3D6 + 2')
    assert d.times == 3
    assert d.sides == 6
    assert d.modifier == 2

    try:
        Dice.parse('nonsense')
        raise AssertionError()
    except ValueError as v:
        assert str(v) == 'Invalid dice specification string "nonsense"'

    try:
        Dice.parse('-2d5')
        raise AssertionError()
    except ValueError as v:
        assert str(v) == 'Invalid dice specification string "-2d5"'

    try:
        Dice.parse('0d6')
        raise AssertionError()
    except ValueError as v:
        assert str(v) == "times=0 is not a positive integer"

    d = Dice.parse('100 d 12+3')
    n = 0
    for x in d.roll(True):
        assert 4 <= x <= 15
        n += 1
    assert n == 100


def main():
    test()

    dice = Dice.from_stdin()
    dice.print_roll()


if __name__ == '__main__':
    main()
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I should've used comments to clarify what this is all about, my bad lol. This code is for reading messages like "2 d 10 + 3", for RPG dice rolling. Although "d" is completely useless, it makes for the classic virtual dice roller structure. It should print a message with the structure need for reading. Besides that, I didn't about anything you pointed me out, thanks for the help, I didn't expect any answers. \$\endgroup\$ – Lucas Melo Jul 25 at 13:06
  • \$\begingroup\$ Oh my. Thank you very much for your time and suggestions. I will definitely study that, as didn't expect such an answer. I would also like to extend it a bit and ask another question: I'm a little overwhelmed by the amount of stuff that comes along with Python (Atom, Pycharm, Anaconda, virtual environments) and I don't know where to start. What do I really need to play around with Python a little, but without limiting myself? \$\endgroup\$ – Lucas Melo Jul 25 at 20:33
  • 1
    \$\begingroup\$ Personally my favourite environment is PyCharm. It goes a long way to suggesting where things are wrong or non-standard. \$\endgroup\$ – Reinderien Jul 25 at 20:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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