Rhesa Rozendaal writes:
> I'd be thrilled if you could check it out now that it's on CPAN.
OK, I've had a look, it seems like good Perl code to me -- and as a
bonus, from reading your code I learnt about the DBI prepare_cached
method (so that's a wheel that I can stop re-inventing on an ad hoc
basis in my own code).
One incredibly trivial thing -- in this code you're copying a bunch of
values from one hash to another using the same keys in both, which can
get a bit repetitive:
my $initial = shift if @_ % 2;
my %opts = @_;
my $self = {
countername => $countername,
dbh => $opts{dbh},
dsn => $opts{dsn} || $DSN,
login => $opts{login} || $LOGIN,
password => $opts{password} || $PASSWORD,
tablename => $opts{tablename} || $TABLENAME || 'counters',
initial => $initial || '0',
};
bless $self, $pkg;
One nice way of dealing with this that I hadn't thought of till I
recently spotted in Text::VimColor (though it's probably used in many
places) is simply to bless the options hash, so you'd end up with
something like:
unshift @_, 'initial' if @_ % 2;
my %opts = (countername => $countername, @_);
$opts{dsn} ||= $DSN,
$opts{login} ||= $LOGIN,
$opts{password} ||= $PASSWORD,
$opts{tablename} ||= $TABLENAME || 'counters',
$opts{initial} ||= '0',
bless \%opts, $pkg;
But supplying the default values for each individual option means this
technique doesn't avoid having a line listing each option -- it just saves
having to mention the keyname twice on each line -- so may not be worth
doing.
> > always have tests! One approach would be to use DBD::SQLite for the
> > database, and skip the tests if that module isn't installed.
>
> You've got it all! It was quite fun writing them, actually.
Wow -- I'm impressed, not least because I wrote the suggestion above
without actually knowing how to implement it myself!
> > > Connection settings can be set in the constructor, or by using
> > > the package variables $DSN, $LOGIN and $PASSWORD and $TABLENAME.
> >
> > I don't see the advantage of the package variables, given you can set
> > all of these things in the constructor in a nice clean way.
>
> Having the package vars available makes it easier to instantiate
> multiple counters:
Yeah, I can see they would be useful for that, but I'm still not
convinced that overrides the general badness of global variables. I
think I'd object to them less if they weren't given so prominent in the
documentation -- for example the login parameter is defined in terms of
$LOGIN, forcing the reader to learn about both of them and making the
former seem somehow inferior.
If the global variables are mainly intended only for people with
multiple counters then the documentation could describe how to use a
counter without mentioning them at all, then you could have a section
'Multiple Counters' which explains what use the globals have and how to
use them to provide defaults for the constructor -- thereby reversing
the prominence, defining the globals in terms of the params, and meaning
that people who only need one counter don't have to learn about the
globals.
That would also enable you to have a specific example of using the
globals, where you could local-ize them and help to promote best
practice in use of global variables in general. (The main objection to
them is that if you set them without local then they affect unrelated
bits of code, perhaps in other modules out of your direct control.)
> $c_in = DBIx::Counter('gauge one');
> $c_out = DBIx::Counter('gauge two');
But ... I still don't like having the globals! I suspect the OO way of
dealing with this would be to have a 'counter factory' class which takes
parameters and then can create counters using them:
my $counter_factory = DBIx::Counter->factory(dsn => $whatever);
my $c_in = $counter_factory->create('gauge one');
my $c_out = $counter_factory->create('gauge two');
That could be overkill. An alternative would be just to add a clone
method (which could be considered to be making counters be their own
factories):
my $c_in = DBIx::Counter('gauge one', dsn => $whatever);
my $c_out = $c_in->clone('gauge two');
(But some people may object that the word 'clone' is inappropriate,
since it doesn't clone the counter's value but resets it to the initial
value.)
Finally, in the doc here:
http://search.cpan.org/~rhesa/DBIx-Counter-0.01/lib/DBIx/Counter.pm#new
I think it'd be less confusing if the big code block only contained
things that are actually Perl code. My pod knowledge isn't very good,
but I believe there is some kind of list-markup you could use for the
list, rather than having to format it in a fixed-width block; and the
word "Examples" can be a 'proper' paragraph.
Hope that's of use!
Smylers
--
May God bless us with enough foolishness to believe that we can make a
difference in this world, so that we can do what others claim cannot be done.
