Review and Suggestions - My First Module

awohld has asked for the wisdom of the Perl Monks concerning the following question:

I created my first module, really a class I guess. It's called My::DB and it stores the connection info for my various databses ( Postgres / Sqlite ). I hate typing the DB info into every program so I tried to make a module out of it.

Credited for helping me learn this:
Object Oriented Perl
Intermediate Perl

How does it look, what should I be doing differently?

My::DB

package My::DB;
use strict;
use DBI;

#  Postgres Username/Password
$My::DB::pg_username = 'my_username';
$My::DB::pg_password = 'my_password';

sub new {
    
    my $data = $_[1];
    
    my $dbh;
    
    if ( $data->{server} eq 'pg' ) {
        $dbh = DBI->connect("dbi:Pg:dbname=$data->{db};user=$My::DB::p
+g_username;password=$My::DB::pg_password")
            or die "Unable to connect: $DBI::errstr\n";
        $dbh->{PrintError} = 1;
        $dbh->{RaiseError} = 1;       
    }
    
    if ( $data->{server} eq 'sqlite' ) {
        $dbh = DBI->connect("dbi:SQLite:dbname=$data->{db}","","",{Pri
+ntError => 1,AutoCommit => 1});
    }
   
    return $dbh; 
  
}
[download]

Main Code

#!/usr/bin/perl -w

use strict;
use My::DB;

#  connect SQLITE
my $dbh = My::DB->new({server => 'sqlite', db => 'test.db'});

#  connect PostgreSQL
#my $dbh = My::DB->new({server => 'pg', db => 'test'});

my $sth_select = $dbh->prepare("SELECT * FROM tbl1");
$sth_select->execute;

while ( my @row = $sth_select->fetchrow ) {
        print "@row \n";
}
[download]

Edit: Updated using plobsing's suggestion.

Comment on Review and Suggestions - My First Module Select or Download Code

Replies are listed 'Best First'.
Re: Review and Suggestions - My First Module by plobsing (Friar) on Dec 10, 2007 at 04:57 UTC
Not everything is a class. And by many definitions, this isn't one (you can't get an instance of it, for example). Why bother passing the package name when you aren't blessing anything? Why bother keeping the package name around in `$class`? Why is it not just a normal subroutine? KISS	[reply] [d/l]
Re^2: Review and Suggestions - My First Module by awohld (Hermit) on Dec 10, 2007 at 05:03 UTC
You're right, I see how it's not a class. And I dropped keeping the class name around.	[reply]
Re: Review and Suggestions - My First Module by plobsing (Friar) on Dec 10, 2007 at 05:21 UTC
You might also consider file-scoped `my` variables in stead of globals. This lends itself to better encapsulation, but reduces some functionality (but then again, you didn't realy want to poke your username and password from outside this module did you?) `# Postgres Username/Password $My::DB::pg_username = 'my_username'; $My::DB::pg_password = 'my_password';` [download] becomes `# Postgres Username/Password my $pg_username = '...'; my $pg_password = '...';` [download]	[reply] [d/l] [select]
Re: Review and Suggestions - My First Module by plobsing (Friar) on Dec 10, 2007 at 06:20 UTC
You might want to catch typos for what kind of database it is. Sure this error will be caught eventually and you will be able to track it down, but right now, if you do this: `my $dbh = My::DB->new({server => 'foo', db => 'oops.db'})` [download] you will get errors related to your returning of `undef` at the first location where you try to use the value in stead of a nice Carp saying something to the effect of "Hey, I don't know what a 'foo' database is." which will be reported for the line where the mistake actually occurs. Try something like: `if ($data->{server} eq 'pg') { ... } elsif ($data->{server} eq 'sqlite') { ... } else { Carp::croak "Oops, you tried to access a '$data->{server}' type data +base, but I have no idea what that is"; }` [download]	[reply] [d/l] [select]
Re: Review and Suggestions - My First Module by narainhere (Monk) on Dec 10, 2007 at 05:20 UTC
Seems like you are hard-coding the user-name and password.why not make that one too as a argument? The world is so big for any individual to conquer	[reply]
Re^2: Review and Suggestions - My First Module by bradcathey (Prior) on Dec 10, 2007 at 13:50 UTC
That was something I noticed as well. I usually store my username and password in a .conf file in a another directory and then grab it with some form of Config::Simple. Here's an example I use with CGI::Application::Plugin::Config::Simple (thanks to gmax): `sub dbconnect { my $self = shift; my %attr = @_; my $user = $self->config_param($attr{'db'}.'.user'); my $pass = $self->config_param($attr{'db'}.'.pass'); my $dbh = DBI->connect_cached('DBI:mysql:'.$dbn.':'.$host, $user, $pass, {RaiseError => 1} ) or die "Can't connect: $DBI::errst +r\n"; return ($dbh); }` [download] —Brad "The important work of moving the world forward does not wait to be done by perfect men." George Eliot	[reply] [d/l]
Re^2: Review and Suggestions - My First Module by awohld (Hermit) on Dec 10, 2007 at 14:53 UTC
I don't want to have to remember my username and password.	[reply]
Re: Review and Suggestions - My First Module by apl (Monsignor) on Dec 10, 2007 at 12:36 UTC
I'd like to suggest that, in the future, you leave the original code alone so that the rest of us can see what the comments refer to. You could always post the revised code after the original code.	[reply]
Re^2: Review and Suggestions - My First Module by bradcathey (Prior) on Dec 10, 2007 at 13:39 UTC
Absolutely! My thoughts exactly. —Brad "The important work of moving the world forward does not wait to be done by perfect men." George Eliot	[reply]
Re: Review and Suggestions - My First Module by talexb (Chancellor) on Dec 10, 2007 at 14:59 UTC
I would make the configuration information into a table, or better yet, a configuration file. That way you don't need to fiddle with the module code every time anything changes the database. It also allows you to reuse the module for different applications, but change which configuration file you specify -- that's way better than having to edit the module for each location where you want to use it. But excellent plan to modularize that aspect of your programming .. modularity is definitely the way to go. Alex / talexb / Toronto "Groklaw is the open-source mentality applied to legal research" ~ Linus Torvalds	[reply]
Re: Review and Suggestions - My First Module by tcf03 (Deacon) on Dec 11, 2007 at 06:08 UTC
I do something similar but have a sub for each type of DB - often times you need to set environmental vars, or some databases require different information passed ( or not ). sub MSSQLConnect { my $ds = shift; my $db = shift; my $user = shift; my $pass = shift; $ENV{SYBASE} = '/usr/local'; $ENV{DSQUERY} = $ds; my $dbh = DBI->connect("dbi:Sybase:database=$db", $user, $pass ) or return "Unable to connext to $ds: $!"; return $dbh; } sub INFORMIXConnect { my $server = shift; my $db = shift; #my $user = shift; #my $pass = shift; $ENV{INFORMIXSERVER} = $server; $ENV{ONCONFIG} = "${server}.onconfig"; $ENV{INFORMIXDIR} = '/opt/informix'; #$ENV{DBDATE} = 'MDY4-'; #$ENV{DBTIME} = '%I:%M:%S'; $ENV{PATH} = '/usr/local/bin:/usr/bin:/bin:$ENV{INFORMIX +DIR}/bin'; my $dsn = "dbi:Informix:$db"; my $dbh = DBI->connect("$dsn") or #, $user, $pass) or return "Unable to connect to $server: $!\n"; return $dbh; } [download] Ted -- "That which we persist in doing becomes easier, not that the task itself has become easier, but that our ability to perform it has improved." --Ralph Waldo Emerson	[reply] [d/l]

The world is so big for any individual to conquer