Beefy Boxes and Bandwidth Generously Provided by pair Networks vroom
There's more than one way to do things
 
PerlMonks  

BWTDI: Better Way To Do It?

by coreolyn (Parson)
on Jan 14, 2001 at 20:43 UTC ( #51764=perlquestion: print w/ replies, xml ) Need Help??
coreolyn has asked for the wisdom of the Perl Monks concerning the following question:

I can tell by looking at this sub that this it does not look like the type of elegant solutions I've seen on this site. What I'm trying to accomplish here is the following:

Call an object as Dice->grab('3d6s') Where the number before 'd' is one argument, the number after 'd' is and argument and the 's' is an optional argument. I've gotten it to work, but I can tell by looking at it that there is probably a more efficient way to accomplish it.

sub grab { my $init = shift; my $class = ref($init) || $init; if ( shift =~ m/(^\d+)d(\d+)(.*)/i ) { my ( $type, $face, $qty, @dice ); $qty = $1; $type = $2; $face = $3; for ( my $i=1; $i <= $qty; $i++ ) { if ( $face =~ m/s$/i ) { push( @dice, Die->get($type,1) ); } else { push( @dice, Die->get($type) ); } } return bless { dice => \@dice }, $class; } else { croak "Invalid parameters to Dice::Dice->grab: $!"; } }

coreolyn

Comment on BWTDI: Better Way To Do It?
Download Code
Re: BWTDI: Better Way To Do It?
by mirod (Canon) on Jan 14, 2001 at 21:17 UTC

    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.

Re: BWTDI: Better Way To Do It?
by I0 (Priest) on Jan 14, 2001 at 21:39 UTC
    could you show the code of Die->get?
Re: BWTDI: Better Way To Do It?
by Kanji (Parson) on Jan 14, 2001 at 22:12 UTC

    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--;

    ... 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

Re: BWTDI: Better Way To Do It?
by I0 (Priest) on Jan 14, 2001 at 22:34 UTC
    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;

      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

      Ok so it works and I've been playing with

      $face &&= 1;
      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";
      afterwords it does pop an unitialized warning, and I would have expected it to print a '0'.

      coreolyn -- arm tired from head scratchin :)

        Die->get checks if ( $faced && $faced == 1 ) {
      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.)

Re: BWTDI: Better Way To Do It?
by dkubb (Deacon) on Jan 15, 2001 at 12:48 UTC
    I think I may have found a way that could be more efficient:
    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; }

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

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://51764]
Approved by root
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others scrutinizing the Monastery: (14)
As of 2014-04-18 14:22 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    April first is:







    Results (469 votes), past polls