Gyepi,
Sorry about the options code, I apparently hadn't finished that. Thought I
had :( If you look not very closely you can see where I stole part of it...
:)
I din't quite follow the bless stuff. You're welcome to tell me more but
I'll hit the refs in a bit and figure out what you meant.
I've eliminated the unused destructor per your and Sam's comments. From
C++, I habitually add destructors, since I always end up using them anyway.
Looks like it wasn't a good call here. Also I don't use mod_perl so I'm not
sure what types of things one would do to make code mod_perl safe :(
Dave
----- Original Message -----
From: "Gyepi SAM" <[EMAIL PROTECTED]>
To: "David Ferrance" <[EMAIL PROTECTED]>
Cc: "'htmltmpl'" <[EMAIL PROTECTED]>
Sent: Tuesday, September 04, 2001 7:01 PM
Subject: Re: [htmltmpl] helper module: grouper
> On Tue, Sep 04, 2001 at 06:19:34PM -0400, David Ferrance wrote:
>
> > BTW a lot of you know perl better than I do. I love constructive
criticism. Fire away.
>
> Well, since you ask, here are a few things that jumped out at me during a
> quick read of the code.
>
>
>
> # load in options supplied to new()
> for (my $x = 0; $x <= $#_; $x += 2)
>
> $x <= $#_ is wrong. $#_ contains the index of the last item. Since you are
> stepping by twos, $x < $#_ is what you want.
>
> $options->{lc($_[$x])} = $_[($x + 1)];
>
> Where is $options defined?
>
>
> bless($self);
>
> This should either use pass the C<$class> variable, or get rid of it and
the
> and the C<$proto> variable. As it stands, you set it's value, but do not
use it anywhere else.
> I usually say C<$self = bless {}, ref($self) || $self> somewhere near the
top of the sub.
> As written, this package cannot be easily subclassed.
>
> return $self;
> }
>
> sub DESTROY
> {
> my $self = shift;
>
> eh what? Are you trying to keep this thing in memory forever?
> If you run this under mod_perl, you'll only reclaim the memory when you re
start the httpd server.
>
> }
>
> sub define_total
> {
> my ($self,$col) = @_;
>
> # should verify that col hasn't already been added
>
> You could keep a hash of column names to do the verification. Question is,
> what do you do in the event of a duplicate? Die or replace original column
with new value I suppose.
> # OK, add it
> push @{$self->{SUM_COLS}}, $col;
> }
>
>
> -Gyepi
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
