in reply to A small Deity AI class system

Hi there!

Almost too long to read without further explanation, however - let me provide brief feedback based on what I have seen in package HollyGameAI::RNG. Please note that I am writing this without knowledge of context.

Mistake in sub *rollDX*. 

Global symbol "$dx" requires explicit package name (did you forget to declare "my $dx"?) at test.pl line 22
Did you perhaps mean something like this? 
    sub rollDX { rand(shift->{dx}); }

[download]

Class constructor is usually named *new*. In your code, you are using constructor named RNG, which of course works, however this name is not something other people reading your code would expect to be. I would recommend sticking with the name *new* (minus the asterisk of course). 

    sub new { # this was named RNG
            my $class = shift;
            my $self = { dx => 0 };
            return bless $self, $class;
    }

[download]

The way how you pass parameters to your subs is broken.

Consider this code: 
    sub roll {
            my ($self, $dxx) = shift;

[download]
This leaves variable $dxx undefined. Did you mean this? 
    sub roll {
            my ($self, $dxx) = @_;

[download]

Rather complex logic in sub *roll* could be simplified.

Seemingly, the *given* statement in your code is not needed, your *rollDX* covers all scenarios which are defined in *rollD1*, *rollD3*, *rollD<whatever>* routines. However, it is possible that these are actual placeholders which will contain additional code... So it is possible this is actually valid.
Anyway, the *given* statement could be replaced by something more readable, for example: 
    package HollyGameAI::RNG;
    use strict;
    use warnings;
    use Carp;

    our $roll_dispatch = {
        0  => sub {0},
        1  => \&rollD1,
        3  => \&rollD3,
        6  => \&rollD6,
        10 => \&rollD10,
        20 => \&rollD20,
    };

    sub roll {
            my ($self, $dxx) = @_;
            $self->set($dxx);        
            return 0 unless defined $roll_dispatch->{$dxx};
            return $roll_dispatch->{$dxx}->();
    }

[download]

Final tip - it seems you are serious in that your code will be object oriented. If that's the case, I would recommend investing some time and learning one of Perl object oriented frameworks. As for myself, I have been using Moo for some time, and never looked back. Moo

Don't give up!

Regards, Jan

In Section Meditations