in reply to Dice::Dice

I think you should do some more thinking about your interfaces.

I know it is tempting when turning loose on the OO way of thinking to go overboard. For instance writing get/set methods for everything. Having dice that know everything about themselves. So on and so forth.

But such over-engineering is one of the major (IMHO) pitfalls of OO design. To avoid it, stop and walk through how you expect your stuff to be used. Do it by hand if need be. (An interesting extreme programming exercise is to have people stand in a room, each one being another object, and walk through the algorithm, watching as people make requests.)

What you want to do is export a simple interface that can be implemented internally efficiently. For instance most of your get/set routines can be replaced by hash interfaces that you declare cannot be accessed externally. In one stroke you throw away a large amount of your interface and get a huge performance win.

For instance why should a die keep track of its last roll? Almost never is that needed. OTOH you are constantly going to want to roll a particular type of die many times in a row. Why require a ton of objects for that? I maintain that in practice the simpler interface of saying that a die has a size and can be rolled to be rolled many times is just as flexible and far more efficient: 

package Dice::Die;
$VERSION = 0.04;

use strict;

sub get_size {
  (shift)->{size};
}

sub new {
  my $init = shift;
  my $class = ref($init) || $init;
  my $size = shift;
  return bless {size => $size}, $class;
}

sub roll {
  my $self = shift;
  my $count = shift || 1;
  my $size = $self->{size};
  return map {int(rand($size) + 1)} 1..$count;
}

1;

[download]
(Note that while I provide an external accessor, I don't use it internally.)

Another point is be lazy about doing work. For instance in your Dice::Dice::roll method you do a tremendous amount of work keeping track of totals etc. I have to ask why. After all my experience with role-playing games suggests strongly that people will want to start rolling 5 and taking the top 3 very shortly. Just do a roll and return results. Allow the work of massaging that data to be done by someone else. (Do something simple, and do as little as possible.) After all that is why we have List::Util and friends.

Oh, also think twice before tracking stuff yourself. For instance if you kept your current design I would modify your roll method in Dice::Dice to look more like this: 

sub roll {
  my ($self) = shift;
  my $diRef  = $self->get_di;

  my ($total, @roll);
  foreach my $di ( @$diRef ) {
    my $rolled = $di->roll();
    push( @roll, $rolled );
    $total += $rolled;
  }
  $self->set_total($total);
  $self->set_dice(\@roll);
  return $total;
}

[download]
Oops, where did your quantity go? I don't need it because every array in Perl already knows how long it is. Besides which there is less room for me to make a mistake (and there is somewhat better performance) if I don't try to use explicit for loops.

Another warning is that the entire idea of a pseudo-hash is flawed. There is a real possibility that when Perl 5.8 comes out it will no longer be supported. It is virtually certain that it is an idea that will not survive into Perl 6.0 due to implementation issues. In other words code that depends on it will be in the acceptable 5% breakage. Which means that no matter what the performance benefit may be, it is unwise to start using it.

Unfortunately there is no good way to learn that other than by paying attention to development discussions. But after the first attempt at Perl 6 (the Topaz effort) crashed and burned on that issue, the value of supporting it has to be questioned. Oh, something else with the same idea likely will be introduced in time. But not with that interface.

Besides which the performance benefit is probably not that big for you. While they make accesses faster, they make construction more expensive. Given that your design results in constructing lots of objects that are accessed very few times each, this may well not be a net win. (Hey, as another benefit of not using them you get to do less typing! :-)

Replies are listed 'Best First'.
(ichimunki) Re (tilly) 1: Dice::Dice
by ichimunki (Priest) on Jan 08, 2001 at 02:05 UTC
    I don't see it as a problem to have a method for remembering how many sides the die has, especially since it's two real lines of code and the instance has to have that in order to useful (so it's persistent data anyway). Since he is doing this as a learning exercise, it's a good point to be made though.

    As far as dice objects go, I prefer to have a Face attribute, which does hold the most recent roll. Completely useless in many circumstances, but what if this is for a game of Yahtzee? Then the number on the top of the die is very important. Also, as an amusing trick, I'd like to see an OppositeFace () method.

    By carefully abusing list vs. scalar context, the same method can be made to return a single roll or a list of several rolls (of the same die). Also useful would be a sum method to total up several rolls into a single scalar result.

    Gee, I'm glad I have no idea what a pseudo-hash is given it's shaky future. I would have used "our" to create a default hash if it were really needed, but prefer to simply set the defaults in the new constructor.
[reply]
      Thank you for reminding me. Default values are something that is good to make a method from if you want them inherited in an override. His implementation of defaults tries to do that but will blow up because he will try to be using a symbolic reference and has strict on.

      As for the accessor method, I provided one accessor where he provided 2 setters and 2 getters. Then I didn't use mine internally, while he used his a lot. I think that the difference is substantial.

      You do have a good point about the needs of Yahtzee vs the needs of role-playing games. However if you wanted to reuse this module for Yahtzee, why not provide a subclass that adds the face attribute? Just because something is sometimes useful doesn't mean that you want to always do it. Instead be reasonably efficient by default but provide an interface that can be extended easily.

      As for a sum method, depending on your situation that might or might not fit in the class. In role-playing games you often see rolling a lot of dice and summing them up. But you also see a lot of rolling several dice and summing up some subselection. (Usually the top m of n.) In games such as backgammon you really don't want to sum them, but you do care about doubles. In short it doesn't need to happen by default though it may be convenient often enough to provide it.

      As for what a pseudo-hash is, it is an anonymous array whose first element is a hash saying what the following entries are. In short it acts a lot like a C struct. And if you do a lot of declarations Perl will actually resolve the lookup through the hash element at compile time and the accesses will be faster as well.

      Unfortunately it looks to be history. But you might as well remember this for reuse as a future trivia question.

      What feature was left out of Perl 6 because it caused serious performance problems in the first attempt to write Perl 6?

[reply]

        I have a question concerning this comment of yours,

        Default values are something that is good to make a method from if you want them inherited in an override. His implementation of defaults tries to do that but will blow up because he will try to be using a symbolic reference and has strict on.

        Inheritance is everything in this module so I'm trying to grok this. My english2perl isn't what it should be (yet). While I was working on this I made sure that Dice::di's roll method was inherited into Dice::Dice before I overroad it, and I had no problem. So obviously you are trying to warn me of something I don't know how to identify in the code.

        Please help in assisting the blind to see,

        coreolyn

[reply]

      By carefully abusing list vs. scalar context, the same method can be made to return a single roll or a list of several rolls

      Why would this be abusing context? That is exactly what wantarray is for ...

      Also, as an amusing trick, I'd like to see an OppositeFace () method. 

      sub opposite_face {
  my $self = shift;
  return $self->{size} - $self->{face} + 1;
}

      [download]

      Tony

      Update: DOH!

[reply]
[d/l]
        Re: Context: I know it's not really abuse. Poor choice of words.

        That should probably be  return $self->{size} + 1 - $self->{face};. Yours returns 13 on a six-sider with a six up. The answer is one. Of course, we also need a sanity check that it's an even number of sides not equal to 4 (since four sided dice are pyramid shaped).

        Note: I think I am a little too into dice. :) Update: the opposite face formula in the preceding post is correct at this time. I still maintain a need to sanity check whether the die has opposing face.
[reply]
[d/l]

      Call me dense but I'm not understanding the difference between a 'face' attribute and the array held in the 'Results' attribute.

      As for an OppositeFace() method I don't think I'll be chasing that one for a while :)

[reply]
        Untested, for illustration only.
        sub new {
    my $Di = { Sides => 6,
               Face => 1
             }; #default six sider, sitting with 1 up
    bless $Di, "Dice::Di";
}

sub Roll {
    my $self = shift;
    my $newface = int( rand( $self->{'Sides'} ) + 1);
    $self->{'Face'} = $newface;
    return $newface;
}

sub Rolls { #call with @results = $Di->Rolls( integer );
    my $self = shift;
    my $times = shift;
    my @faces;
    for (1..$times) {
        my $rolled = $self->Roll();
        push( @faces, $rolled );
    }
    return @faces;
}

sub CurrentFace {
    my $self = shift;
    return $self->{'Face'};
}

        [download]
        If you really want to be able to know every roll this particular Di has ever yielded you could add History => (), to the new method and push $newface onto that during Roll(). I would avoid this unless it were heavily restricted in the implementation. Also, using the wantarray function (or even detecting the presence of an argument) you can easily have the Roll() method detect whether or not it should act like the Rolls() function and return a list, or just roll once and return that as a scalar. Does this help?
[reply]
[d/l]

In Section Code Catacombs