[RFC] Text::Echelon v0.01

Andrew Wilson andrew at rivendale.net
Sun Dec 16 09:13:29 CST 2001


On Sun, Dec 16, 2001 at 12:13:24PM +0000, Russell Matbouli wrote:
> Hello,
> I've written Text::Echelon after realising that my X-Echelon header
> program was very inefficient (shuffling an array instead of just taking
> a random element from the array). I wanted to get some feedback before
> putting it on CPAN, so please have a look at the code and tell me what
> I'm doing wrong. I'm not sure if my tests are doing the right thing
> even. Have I just got The Wrong Idea?
> 
> http://russell.matbouli.org/code/text-echelon/

Hi Russel

Mainly it doesn't work because you don't construct an object in your
constructor.  If you fix that the rest of the code works although
you do some things that I wouldn't do.

1) You construct the wordlist in your new function this would be better
moved into the pacakge initialisation.  Also the

while (<>) {
    @wordlist = <DATA>;
}

can be achieved with 

@wordlist = <DATA>;

2) You might as well just chomp the list and do your srand
initialisation at package level as well.  Wordlist is a package
variable, standard perlstyle says it should have initial caps (I
usually ignore this one, but you should be aware of what you're
ignoring ;-)

my @Wordlist = <DATA>;
chomp @Wordlist;
srand(time() ^ $$);

sub new {
    my $class = shift;
    my $self  = bless {}, $class;
    return $self;
}#new

3) You dont need a lot of the temporary variables that you scatter
about your code.

sub get {
    my $self = shift;
    $Wordlist[rand($#Wordlist)];
}#get

4) You should try to learn how to use perl's list operators, I much
prefer

join $delim, map $self->get (1 .. $num);

to 

my $str = '';
for(my $currnum = 1; $currnum <= $num; $currnum++)
{
    $str .= $self->get();
    $str .= $delim unless ($currnum == $num);
}
return $str;

I think it's more efficient too.  Just in case you can't follow it,
let's run through it with your default values.

(1 .. 3)  

this constructs the list (1, 2, 3)

map $self->get (1, 2, 3) 

this iterates over the list you've just made making $_ an alias to
each list member in turn and calling the function that you've asked it
to.  It calls $self->get three times, as you can see we've ignored $_
here we're just using it to make sure we call the funtion the correct
number of times.  The return value of the map is a list of phrases
returned by the get method.

join $delim, ("phrase one", "phrase two", "phrase three");

so you get 

"phrase one, phrase two, phrase three"

which is the last thing evaluated in the method, so it gets returned.

5) You keep redefining default values for things, then you pass in the
default value.  You only really need to define them in the method that
actually uses them and you don't need to pass in the default value.

I've modified the module and wriiten another test file for it, so you
can see what I'm talking about.  As I've said before when you fix the
constructor all the rest of it worked and these are jsut style points
really.  If you want to discuss any more of it give me a shout

cheers

Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: changed.tgz
Type: application/x-gtar
Size: 4972 bytes
Desc: not available
Url : http://mail.pm.org/archives/belfast-pm/attachments/20011216/18917ed8/changed.gtar


More information about the Belfast-pm mailing list