in reply to Comments on my code, advices how to improve it.

Your new I would factor to something like this:
sub new { my ( $class, %config) = @_; # maybe check arg's bless { "_CONFIG" => \%config }, $class; }
but that is not of too much import.

I would have connect and disconnect warn or die when inappropriately called, i.e. connecting when already connected.

disconnect therefore would undef $self->{_DBH}.

Similiarly I would not be likely to have a connect call inside of the prepare routine. This makes the function name misleading and it just smells. There is a convenience factor but when I find my code having duplicate functionality like this it is a sign of losing track of the logical structure of the program. That I may have forgotten that connect was supposed to be called early on to setup the connection (or vice versa).

There are prettier ways to build your SQL in execute. I am not facile at building up strings of this sort--I find your SQL building to be understandable but error prone.

The continual linkage between $self and the %params might indicate that the object should be smarter. It is not possible to tell from the info you have given.

Be well, rir

Replies are listed 'Best First'.
Re^2: Comments on my code, advices how to improve it.
by techcode (Hermit) on Sep 01, 2005 at 19:51 UTC
    I would like to thank you all for nice suggestions on improving this. I especially liked the chanio's reply (I like it!) :)

    Didn't have much time to play on this as I'm in the middle of the project, but I have lots of ideas for future.

    Actually I'm using this AS IS in this project - it has lot's of simple SQL select/update which are suitable for this...

    package DBIx::Handy; use strict; use DBI; sub new { my ($class, %config) = @_; # Set default value for auto_connect # lazy - Auto connect on first execute method call DEF +AULT VALUE # startup - Right now ... # manual - You should take care of calling connect method $config{auto_connect} = $config{auto_connect} || 'lazy'; my $self = {'_CONFIG' => \%config}; bless ($self,$class); $self->connect() if ($config{auto_connect} eq 'startup'); return $self; } sub DESTROY { my $self = shift; $self->disconnect(); } sub connect { my $self = shift; # Connection configs my $c = $self->{_CONFIG}; unless (defined $self->{_DBH}){ $self->{_DBH} = DBI->connect('dbi:' . $c->{driver} . ':database=' . $c->{database} . ';host=' . $c->{host}, $c->{username}, $c->{password}, {'RaiseError' => 1}) or die "Could not connect to + database. Error message: $!"; } else { warn "Trying to connect but already connected!"; } return $self->{_DBH}; } sub disconnect { my $self = shift; if (defined $self->{_DBH}){ # Finish the STH if needed. $self->{_STH}->finish() if defined $self->{_STH}; $self->{_DBH}->disconnect(); undef $self->{_DBH}; } else { warn "Trying to disconnect but already disconnected!"; } # If it fails, it's already disconnect ... return 1; } sub dbh { my $self = shift; if(defined $self->{_DBH}){ return $self->{_DBH}; } else { warn "Trying to get DBH but not connected to database!"; return; } } sub prepare { my $self = shift; unless (defined $self->{_DBH}){ die "You need to be connected to database to prepare the queries!" +; } return $self->{_DBH}->prepare(shift); } sub do { my $self = shift; return $self->execute(sql => shift); } sub execute { my $self = shift; my %params = @_; # Check - should we connect unless(defined $self->{_DBH}){ $self->connect() if $self->{_CONFIG}->{auto_connect} eq 'lazy'; } # If we received sth in params - it's prepared earlier so we dont +do it now. $self->{_STH} = $params{sth} || $self->prepare($params{sql}); $self->{_STH}->execute(@{$params{data}}); if(defined $params{method}){ my $method = $params{method}; return $self->{_STH}->$method( @{ $params{method_params} } ); } else { return $self->{_STH}; } } sub insert { my $self = shift; my %params = @_; my @fields = $self->_GET_FIELDS($params{table}); my $data = $params{data}; my $sql = 'INSERT INTO ' . $params{table} . ' ('; my ($sql_part1, $sql_part2, @data); foreach (@fields){ if(defined($data->{$_}) && (length($data->{$_}) >= 1) && ($data->{ +$_} ne '')){ $sql_part1 .= "$_,"; $sql_part2 .= '?,'; push (@data,$data->{$_}); } } chop($sql_part1); chop($sql_part2); # to remove last , $sql_part1 .= ')'; $sql_part2 .= ')'; $sql .= $sql_part1 . ' VALUES (' . $sql_part2; return $self->DB_execute(sql => $sql, data => \@data,); } sub update { my $self = shift; my %params = @_; my @fields = $self->_GET_FIELDS($params{table}); my $data = $params{data}; my $sql = 'UPDATE ' . $params{table} . ' SET '; my @data; foreach (@fields){ if(defined($data->{$_}) && (length($data->{$_}) >= 1) && ($data->{$_} ne '') && ($_ ne $params{id_field}) ){ $sql .= $_ . ' = ?,'; push @data,$data->{$_}; } } chop($sql); # to remove last , $sql .= ' WHERE ' . $params{id_field} . ' = ?'; # where id_field = + id_value push @data, $data->{$params{id_field}}; return $self->DB_execute(sql => $sql, data => \@data,); } sub _GET_FIELDS { my $self = shift; my $results = $self->execute(sql => 'SHOW COLUMNS FROM ' . shift , method_params => 'Field'); return(keys %{$results}); } 1;
    And a sample of how it's being used.
    #!perl use strict; use warnings; use DBIx::Handy; my $DB = DBIx::Handy->new(driver => 'mysql', database => 'test', host => 'localhost', username => 'alex'); my $res = $DB->execute(sql => 'SELECT * FROM tabela WHERE id = 1', method => 'fetchrow_hashref'); print <<EOL; Record number : $res->{id} Email : $res->{email} Password : $res->{password} EOL
    Of course the new method is actualy in the cgiapp_init of my main module (I'm using CGI::App), it's parameters come from CGI::App::Plugin::Config::Simple - so I just type : $self->{_DB}->execute(...) in some RunMode ... and ussualy pass the result to HTML::FillInForm and HTML::Template ...

    One thing I didnt understand is this last (The continual linkage between $self and the %params might ...). Would you explain a bit better.

    Also if any body has any other idea I would appreciate it. Thanks!