Feedback, question about my module and if i should change anything ?

Question

package My::Module;

# $Id$

use strict;
use Carp;
use Data::Dumper;
use DBI;

$My::Module::VERSION  = '0.1';

sub new {
    my ($class, %opt) = @_;
    my $opt_count = keys %opt;

    $class->set_error('');
    #return $class->set_error("Too many arguments to initialize.") if ($opt_count > 5);
    #return $class->set_error("Missing arguments to initialize.") if ($opt_count < 2);

    my $self = bless {
                      _DRIVER_OPTIONS  => $opt{'mysql'},
                     },$class;

    if (not defined $self) {
        return $class->set_error( "new() failed: " . $class->errstr );
    }

    if ($self->{_DRIVER_OPTIONS}->{Host} ne '') {
        $self->{_DRIVER_OPTIONS}->{DataSource} = 'DBI:mysql:database=' . $self->{_DRIVER_OPTIONS}->{Database} . ';host=' . $self->{_DRIVER_OPTIONS}->{Host};
    } else {
        $self->{_DRIVER_OPTIONS}->{DataSource} = 'DBI:mysql:database=' . $self->{_DRIVER_OPTIONS}->{Database} . ';';
    }
    $self->{Handle} = DBI->connect($self->{_DRIVER_OPTIONS}->{DataSource},
                                   $self->{_DRIVER_OPTIONS}->{Username},
                                   $self->{_DRIVER_OPTIONS}->{Password},
                                   { RaiseError=>1, PrintError=>1, AutoCommit=>1 }
                                  );
    return $self->set_error("new(): couldn't connect to database: " . DBI->errstr) unless ($self->{Handle});
    $self->{_disconnect} = 1;

    print Dumper \$self;

    return $self;
}

sub database {
    my $self = shift;
    if (@_) { $self->{Handle} = shift }
    return $self->{Handle};
}

sub set_error {
    my $class   = shift;
    my $message = shift;
    $class = ref($class) || $class;
    no strict 'refs';
    ${ "$class\::errstr" } = sprintf($message || "", @_);
    return;
}

*error = \&errstr;
sub errstr {
    my $class = shift;
    $class = ref( $class ) || $class;

    no strict 'refs';
    return ${ "$class\::errstr" } || '';
}

sub DESTROY {
    my $self = shift;

    unless (defined $self->{Handle} && $self->{Handle}->ping) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
    }

    unless ($self->{Handle}->{AutoCommit}) {
        $self->{Handle}->commit;
    }

    if ($self->{_disconnect}) {
        $self->{Handle}->disconnect;
    }
}

1;

1. Is that the right way so i can re-use the database on my code instead of having to open a new connection or that will aswell open a new connection every time i use it ?

2. Should i change anything on the module ? or anything i did wrong ?

Currently i am just learning and thinked of doing my own engine module so i began with this.

Simple test code (the bellow code is not to be reviewed just a sample on how to use the module):

#!/usr/bin/perl

use warnings;
use strict;
use Data::Dumper;
use lib 'path to module';
use My::Module;

my $session = My::Module->new(mysql     => {
                                            Database =>'module',
                                            Host     =>'10.0.0.2',
                                            Username =>'module',
                                            Password =>'module'
                                           }) or die My::Module->errstr;

my $dbh = $session->database();
my $sth = $dbh->prepare(q{
             SELECT session_id
             FROM sessions
          });
   $sth->execute() || die print($dbh->errstr);
my $ref = $sth->fetchall_arrayref({});
$sth->finish;

print Dumper \$ref;

Answer 1

I would suggest using an existing database interface rather than rolling your own, as there are many secret gotchas that others have spent years figuring out and solving for you. DBIx::Connector is excellent, and with its fixup mode, will let you reuse database connections, even across process forks.

Additionally, if you use Moose you will never have to write your own object constructors or object fields again. :)

DBIx::Class combined with Moose would be even better, but not necessary until you find yourself needing more ORM-ish features.

Answer 2

Other than using a CPAN module to accomplish this task, here are my practical recommendations:

1. Don't return an error value from the constructor. Instead, throw an exception.
2. Access the internals of your class using accessors rather than using direct hash access.

3. If the user of your class did not enable AutoCommit, she chose not to enable AutoCommit for a reason. Therefore don't do:

   unless ($self->{Handle}->{AutoCommit}) {
       $self->{Handle}->commit;
   }
   

   in DESTROY.

4. Note, bless cannot fail.

Answer 3

Your way of exposing errors is very, very oldfashioned. If something exceptional happens, why not raise a proper exception? You seem to have modelled your error handling after the DBI module. Note that DBI also has a RaiseError option. Using that is almost always more reasonable than using the oldfashioned errorstr version. Unfortunately DBI can't change it's default anymore now, but for new code I entirely don't see the reason to copy this flawed idea.

You're also constructing a DBI connection within your code, based on parameters the user provided from the outside. Do you have a good reason for doing that? Allowing the user to pass in the DBI::dh he constructed himself would be more flexible. Yes, that requires slightly more code on the outside to set up objects and wire them together, but it will also result in a cleaner design. If wiring up your objects manually bothers you too much, you might want to have a look at Bread::Board to do the wiring for you instead of compromising on the design of your module.

Also, I second Ether's suggestion of using DBIx::Connector. It really takes a lot of pain out of managing database handles, which can be very error-prone.