David Gilden <[EMAIL PROTECTED]> wrote:
:
: Please let me know how I can Improve my script and if
: you see anything that is bad style/practice.
I just did a quick copy & paste. Here are my initial
reactions. You're using strict and warnings. Kudos. You
are using white space and indentation but you are not
using the same indentation throughout your program.
You are using all your variables at file scope. Try
to avoid this. Use the smallest scope possible and get
out of the habit of declaring your variables all at the
same time. declare them as you use them.
Call subroutines without the '&' prefix. Use
initialize_dbi() instead of &initialize_dbi. In fact it
would be best to return the database handle:
# line up like terms when appropriate
my $user_name = "####";
my $user_password = "####";
my $sql_server = "#####";
# return the database handle rather that setting a
# global from within the sub
my $dbh = initialize_dbi( $sql_server, $user_name, $user_password )
.
.
.
sub initialize_dbi {
my( $sql_server, $user_name, $user_password ) = @_;
# I didn't know you had to this?
# I haven't done DBI much though.
my $drh = DBI->install_driver( 'mysql' );
my $dbh = DBI->connect("DBI:mysql:$user_name:$sql_server",
$user_name, $user_password);
# Do not return this error message in production code.
die "Cannot connect: $DBI::errstr" unless $dbh;
return $dbh;
}
We now know where $dbh came from. And where $sql_server,
$user_name, and $user_password went to. In the sub we know
what was passed in and what was passed out.
In most functions you retrieve some data, process it,
and return a result. The processing should not affect
or be effected by an outside variable that was not passed
in (except in the case of constants, persistent variables,
and some rare occasions). Using global variables like
you're doing *will* bite you down the line.
Here's a conversion (I didn't test it) for
run_statement():
# Get total records
my $sth = run_statement( $dbh, "select * from guestbook;");
my $maxEntries = $sth->rows;
# This may also work. You'll need to test it.
my $maxEntries = run_statement( $dbh, "select * from guestbook;" )->rows;
sub run_statement {
my( $dbh, $statement ) = @_;
my $sth = $dbh->prepare( $statement );
$sth->execute;
return $sth;
}
Nothing in run_statement() is affected by or effects
variables outside its scope other than through its return
value.
Stick to the same style for separating words in your
variables and subs throughout the entire program. I prefer
using the underscore: run_statement(), not RunStatement().
Whichever you choose, stick to it. And try not to abbreviate
When you need help later on, it might be someone who doesn't
use English as a first language trying to help.
HTH,
Charles K. Clarkson
--
Head Bottle Washer,
Clarkson Energy Homes, Inc.
Mobile Home Specialists
254 968-8328
--
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
