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.
You've got it all! It was quite fun writing them, actually.always have tests! One approach would be to use DBD::SQLite for the database, and skip the tests if that module isn't installed.
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 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.
Agreed. All in all, getting the documentation right seems to be about the hardest part.
Hope that's of use!
Very much so! You've helped me a lot in just a few days. You've made me aware of a number of shortcomings that I hadn't given much thought. But most importantly, you've given me the confidence that it's quite doable, though by no means trivial, to put a good package together.
And I very much enjoyed our conversation too :-)
Rhesa
