my ( $qty, $type, $face ) = shift =~ m/(^\d+)d(\d+)(.*s$)?/i or croak 
+"Invalid parameters to Dice::Dice->grab: $!";
$face &&= 1;
my @dice = map Die->get($type,$face), 1..$qty;
return bless { dice => \@dice }, $class;
[download]

Ok so it works and I've been playing with

$face &&= 1;
[download]

and I think finally get how that's working. (Never used && outside of a conditional before -- cool). But I don't see how it's not popping a 'uninitialized warning' on $face in the map statement when I supply '3d6' as an arg as opposed to 3d6s. If I pop a debugging

print "$face\n";
[download]

afterwords it does pop an unitialized warning, and I would have expected it to print a '0'.

coreolyn -- arm tired from head scratchin :)

"boolean" context is different from "number" context with respect to warnings about "not being a number".

-- Randal L. Schwartz, Perl hacker

Die->get checks if ( $faced && $faced == 1 ) {

Damn! Now that's efficiency! I was trying to get that (.s$)? forever! I knew that dot star was bad but couldn't get it to work any other way. Thanks :)

I'm still worried that it'll blow under -w for an undef'd $face, but I'm off to play.

coreolyn

Er... I don't think you want to include $! in that error message... :)

($! should only be used after an error return that indicates a system error.)

I don't see the need for declaring your variables on one line and assigning to them on the next, or the if/else as you could do ...

    my( $qty, $type, $face, @dice ) = ( $1, $2, $3 ? 1 : undef );
    push @dice, Die->get( $type, $face ) while $qty--;
[download]

... to reduce 9 lines of code to just 2.

Granted, that destroys the value of $qty in the process, but it's easy to preserve by substituting for 1 .. $qty (as Mirod suggests) for the while $qty--.

You may also want to tighten up your regular expression (the .*-thing has been beaten to death), especially if the only optional argument following the second number is an 's'.

As an alternative to the use of a regexp to pass arguments though, I'd personally be more inclined to pass them seperately rather (ie, grab( -dice => 3, -sides => 6, -face => 0 ); or even grab( qw( 3 d 6 s ) );), possibly coupling them with defaults if they aren't set.

    --k.

++! I'm definately going to play with all your suggestions, with the exception of the passing of the arguments. In this case the only purpose the class serves is to simplify coding with dice As int(rand($num)) is so much more CPU efficient. In otherwords the main audience for the class is a casual Perl user that wants to whip out a quick application that requires manipulating many sets of Dice. The 3d6 method of labeling Dice sets is the most common nomenclature, so I'm trying to make the object as 'natural' to use as possible.

This brings about a deeper question where should efficiency end and ease of use begin? Ahh so much to ponder :)

Thanks again, I should have Dice::Dice updated later.

coreolyn

I don't see much improvement to be made here, except maybe replace for ( my $i=1; $i <= $qty; $i++ ) by a simple for (1..$qty) as you don't use the $i in the loop.

sub grab {
  my $class = shift;
  my $dice  = shift;
  my $self  = ref($class) || $class;

  unless ($dice =~ m/^(\d+)[Dd](\d+)([Ss])?$/ ) {
    croak "Invalid dice format to $self->grab: $dice";
  }

  #$1 - Quantity
  #$2 - Type
  #$3 - Face  
  my @dice = map { Die->get( $2, $3 ? 1 : 0 ) } 1..$1;
  
  return bless { dice => \@dice }, $self;
}
[download]

Here's a summary of what I did:

I changed the code to be more in-line, as there is no need for a logic branch if the regex fails to return a match. Inside the croak command, I figured it was better to report back the passed in $dice, rather than $!, which is generaly set by system-level errors.

The regex was optimized. The /i modifier wasn't necessary because you were wanting to match d and D, using a character class is faster anyhow, as in: [Dd]. Same with the [Ss] on the end, which was made an optional match, rather than use the (.*) or (.*s). The Mastering Regex book cautions against using the /i when a character class will do just as well.

Instead of creating temporary variables, I used the $1, $2, $3 variables directly. Of course, I compensated for the loss in readability, by using comments =).

I could have pushed onto an array inside a loop, but map was used and the values were copied directly onto the array. I benchmarked using for, foreach and map, with map coming out as a winner in my tests. The others won when I experimented with making $1 equal to 10000 or so, but this will most likely not get this high, in the real-world.

Also, you'll notice that $3 ? 1 : 0 was used, it's not possible to do something like $3 &&= 1, since $3 is a read-only variable.

Great post! Thought you might want to see some results:

Dice Version .04 IO

  OneDiceNotStored:  3 wallclock secs ( 4.43 usr +  0.00 sys =  4.43 C
+PU)
OneFacedDiceStored:  5 wallclock secs ( 5.32 usr +  0.00 sys =  5.32 C
+PU)
[download]

Dice Version .05 dkubb

  OneDiceNotStored:  4 wallclock secs ( 4.39 usr +  0.00 sys =  4.39 C
+PU)
OneFacedDiceStored:  6 wallclock secs ( 5.24 usr +  0.00 sys =  5.24 C
+PU)
[download]

Now to go back and update Dice::Dice, but with all these examples I think I'm going to revisit Dice::Die again too.

coreolyn

See Dice::Die.