Smylers wrote:
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).
You mean stuff like $sth ||= $dbh-prepare... ? ;^)
I was surprised to see that both connect_cached and prepare_cached were already introduced in DBI 1.06. It wasn't until I started using
Class::DBI that I heard of them. And I haven't seen it used much elsewhere, even though it's incredibly useful.
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:
[...]
One nice way of dealing with this
[...]
is simply to bless the options hash, so you'd end up with
something like:
unshift @_, 'initial' if @_ % 2;
I like that line: one less temporary variable.
my %opts = (countername = $countername, @_);
[...]
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.
That's one reason; the other is that the way I do it now doesn't introduce unwanted data in my object. Any unknown key-value pairs are
silently discarded. Blessing the options hash means you ought to check you only have the accepted keys, which leads to more mentioning of
the keys.
Then again, that's all rather philosophical: if I'd really wanted to be strict, I wouldn't use a hashref as base. Something with closures
and private variables would be a better way of keeping instance data private. But I think that would be overkill in this case.
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!
Hehehe. I copied some stuff from tests of CGI::Session (g4_sqlite.t to be
precise) and went from there.
[...] 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.
Fair enough. There's one situation where I really like having them available, but I agree with your point. I will tone down their prominence
in the documentation, and put them in a special section (with appropriate caveats and an emphasis on local).
[...]
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.)
Yes, that is a very valid concern. On the one hand, it's nice to be able to set those globals once in a configuration file (which is why
they're in there in the first place: I use them that way in a larger application). On the other hand, the risk of affecting unrelated code
is real.
I ran into it once when installing two applications for different customers on the same mod_perl server: the second started trampling all
over the first's data! The config files had the proper values for each custoemr, but since they were package globals, only the first set
loaded was used. I guess you have to make this mistake once before you're properly convinced of the dangers...
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');
Or do something like Class::DBI: set the configuration once in a base class (derived from D::C), and use that in the rest of your
application. All I'd need is add a setup method, so you can do:
package My::Counter;
use base qw/DBIx::Counter/;
__PACKAGE__-setup(dsn=$bar); # this would set vars internally (with
closures perhaps?)
1;
and elsewhere:
my $c = My::Counter-new('gauge three');
That could be overkill.
For this little module, sure!
But I'm thrilled that a class with only four methods and very little
functionality can generate this discussion. I really like this :)
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
