Smylers wrote:
merely saying that you have this module you wish to
contribute is sufficient to get you a Pause account.  And it's OK to
upload something you aren't yet convinced about: just label it as such
clearly at the top of its pod, and give it a 0.01 version number.

Thanks. I got over my initial shyness, and applied for an account. You can find version 0.01 here:

http://search.cpan.org/~rhesa/DBIx-Counter-0.01/lib/DBIx/Counter.pm

- code review would be nice

Sorry, I couldn't be bothered to download the tarball, open it up, and look inside. (If you'd already uploaded this to Cpan and linked to it on the Cpan Search site then a handy link to view the source is provided -- but that could just be me being particularly lazy, and it doesn't necessarily follow that doing so will cause others to read your code.)

I'd be thrilled if you could check it out now that it's on CPAN.

- tests: for a module this simple?

Yes -- 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. This may well be the start of my test-driven development life :)

- exception handling: should I do this myself, or let DBI throw them?

Put your own in if you can provide useful information. But there isn't much point in catching an another module's exceptions if all you're going to do is throw them again unchanged.

I agree with that. I already do this when the connection is made. I'm not quite certain what to do on fetching or storing counter values. On the one hand, having DBI croaking for me is easy for me, and is closest to the truth. On the other hand, having to wrap your counter usage in an eval just to trap possible connection errors seems to go against the simplicity, IMO. I could catch the errors, and simply return undef, but that implies that the counters could receive incorrect data. Hm.


    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:

        $c_in  = DBIx::Counter('gauge one');
        $c_out = DBIx::Counter('gauge two');

It would be good to emphasize that stringification is happening, and
that the value can be interpolated into strings:

print "Item $c is being processed\n";

Quite so. I've incorporated this example in the pod.

Thanks for taking the time to respond!

Rhesa

Reply via email to