[RFC] Text::Echelon v0.01

[Tony Bowden]

On Sun, Dec 16, 2001 at 12:13:24PM +0000, Russell Matbouli wrote:
> 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?

Looks nice!

A few things I'd suggest:

1) I'd probably change some of the function names:  get doesn't really
   tell me what it's getting. Do these all even need to be public?

   What about:

   my $te = Text::Echelon->new;
   print $te->header;

   # or

   $te->set_delimiter(", ");
   $te->set_phrases(3);
   $te->set_prefix("X-My-Echelon");
   print $te->header;

2) You're using @wordlist in a strange way: every time you call new()
   you're setting a package global. You should either make this instance
   variable, which gets read every time (if you're going to
   allow instances of your class to change them in any way), or, probably,
   class data that only gets set once.

   To be honest, I'm not really sure why you've written it in an pseudo-OO
   style at all... 

3) I'd drop the 'use warnings'  - I don't see any other 5.6-isms, so no
   point in breaking backwards compatability...

3b) The srand() is the other lower-edge limit version of this. You haven't
    needed the explicit srand for quite some time...

4) while (<DATA>) { @wordlist = <DATA>; } 
   probably isn't doing what you think it's doing...

   Or maybe it is, but not the way you'd like it to.

   Or something.

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

   This is probably better, if I read it correctly, as:

   my @words; 
   push @words, $self->get for 1 .. $num;
   return join $delim, @words;

6) I'd suggest moving the test to Test::Simple! Saves mucking about with
   all that print OK / not OK stuff.

   Then it's easier to actually test that your code is doing what it
   says: e.g. instead of just testing that getmany returns a true value,
   you can do:

   ok scalar $te->getmany(4) == 4, "We got 4 phrases back";
   
   I'd also suggest that you at least test that when setting a custom
   header that you check something that contains the prefix and postfix
   that you set etc...


Hope this helps. If you want a hand tidying any of this up more, just
let me know!

Tony

-- 
--------------------------------------------------------------------------
 Tony Bowden | tony at tmtm.com | http://www.tmtm.com/
                     If I'm feigning coherence and calmness  Laugh with me
--------------------------------------------------------------------------