Looking for comments on code

O.K. I've finished my code to create a MySQL db and/or table, =
and I'd appreciate hearing your comments. It works well, but it needs =
to be cleaned up before going live. I've left some testing code in. Go =
ahead and be brutally honest. I've learned so much from this list =
already.

Thanks,
Marc

---------

#!/usr/local/bin/perl

use warnings;
use strict;
use CGI::Carp "fatalsToBrowser";

use feature 'say';

use DBI();
use CGI;
my $q =3D CGI->new();
say $q->header(), $q->start_html( -title =3D> "Create MySQL db and =
table" );

my $username =3D 'root';
my $password =3D 'root';
my $host =3D "127.0.0.1:8889";
my $db =3D "test_dbb";
# table name is set below

# Connect to the server to create the db, if it doesn't exist already.
my $dbh_db =3D DBI->connect("DBI:mysql:database=3D;$host",
$username, $password,
{'RaiseError' =3D> 1});


my $qry_db =3D "CREATE DATABASE IF NOT EXISTS $db ";
$dbh_db->do($qry_db) or die ("Can't create db!\n");

if ($dbh_db) { say "Database '$db' was created successfully.
/>

" }

# Disconnect from the database.
$dbh_db->disconnect();


# Connect to the database to create the table and fields.
my $dbh_table =3D DBI->connect("DBI:mysql:$db;$host",
$username, $password,
{'RaiseError' =3D> 1});

my ($primary_key, @fieldnames);
my $file_path =3D "../htdocs/carts/sql/data/orders.tsv";
use File::Basename;
my $table =3D fileparse($file_path, qr/\.[^.]*/);

open (my $file_fh, '<', $file_path) || die ("Can't open =
$file_path for reading");
my $count =3D 0;

my $qry_table =3D "CREATE TABLE IF NOT EXISTS $table ( ";
while (my $line =3D <$file_fh>) {
last if $line =3D~ m/#/; # can place comments below the ### =
line
chomp $line;
next if (! $line); # skip any blank lines in the file...
$line =3D~ tr|a-zA-Z0-9,_\t \(\)||cd;
$count++;
my @fields =3D split (/\t/ , $line);
push ( @fieldnames, $fields[0] );
if ($count == 1) {
$primary_key =3D $fields[0];
}
my $key =3D $fields[0];
my $keytype =3D $fields[1];
if ($keytype) {
$qry_table .=3D "`$key` $keytype, ";
}
else {
$qry_table .=3D "`$key` TEXT, ";
}
}
$qry_table .=3D "PRIMARY KEY (`$primary_key`) ) ENGINE=3DMyISAM =
DEFAULT CHARACTER SET latin1 COLLATE latin1_general_ci;";
=09
$dbh_table->do($qry_table) or die ("Can't create table!\n");
if ($dbh_table) { say "Table '$table' was created =
successfully.\n" }
=09
close $file_fh;

say $q->end_html();

$dbh_table->disconnect();

__DATA__

Here is the partial contents of the orders.tsv file:


orderid smallint(6) unsigned NOT NULL
orderstatus text
userlogin text
user_uid text
date text
shiptype text
Credit text
minibasket text
totalcalcs text
shipcost text
chargetotal decimal(7,2)
comments text
couponid text
authamt text
PmtAVS text
PmtAuth text
PmtType text
TransID text
coupontype text
IP_Address text
taxexempt text
###
Comments:
The first field listed in this file is the Primary Key.=

--
To unsubscribe, e-mail: beginners-unsubscribe@perl.org
For additional commands, e-mail: beginners-help@perl.org
http://learn.perl.org/

Re: Looking for comments on code

Hi Marc,

On Saturday 11 June 2011 05:25:18 sono-io@fannullone.us wrote:
> O.K. I've finished my code to create a MySQL db and/or table, and I'd
> appreciate hearing your comments. It works well, but it needs to be
> cleaned up before going live. I've left some testing code in. Go ahead
> and be brutally honest. I've learned so much from this list already.
>=20

Very well, I'll comment on your code.

> Thanks,
> Marc
>=20
> ---------
>=20
> #!/usr/local/bin/perl
>=20
> use warnings;
> use strict;
> use CGI::Carp "fatalsToBrowser";
>=20
> use feature 'say';
>=20
> use DBI();

Better have a space between the "DBI" And the "()".

> use CGI;
> my $q =3D CGI->new();

I personally distaste calling the CGI instance "$q".

> say $q->header(), $q->start_html( -title =3D> "Create MySQL db and table"=
);
>=20
> my $username =3D 'root';
> my $password =3D 'root';
> my $host =3D "127.0.0.1:8889";
> my $db =3D "test_dbb";

I hope you are not connecting to the database as user root with password ro=
ot.

> # table name is set below
>=20
> # Connect to the server to create the db, if it doesn't exist already.
> my $dbh_db =3D DBI->connect("DBI:mysql:database=3D;$host",
> $username, $password,
> {'RaiseError' =3D> 1});
>=20
>=20
> my $qry_db =3D "CREATE DATABASE IF NOT EXISTS $db ";
> $dbh_db->do($qry_db) or die ("Can't create db!\n");

No need for the temporary variable. Just do:

$dbh_db->do("CREATE DATABASE IF NOT EXISTS $db ");

>=20
> if ($dbh_db) { say "Database '$db' was created successfully.
> />
" }

That's the wrong way to test for errors.

>=20
> # Disconnect from the database.
> $dbh_db->disconnect();
>=20

You should limit the scope of $dbh_db because it's not used after that:

{
my $dbh_db =3D DBI->connect(..)

.
.
.

$dbh_db->disconnect()
}

>=20
> # Connect to the database to create the table and fields.
> my $dbh_table =3D DBI->connect("DBI:mysql:$db;$host",
> $username, $password,
> {'RaiseError' =3D> 1});
>=20
> my ($primary_key, @fieldnames);
> my $file_path =3D "../htdocs/carts/sql/data/orders.tsv";
> use File::Basename;

Put all the use's at the top.

> my $table =3D fileparse($file_path, qr/\.[^.]*/);

What are you trying to do here?

>=20
> open (my $file_fh, '<', $file_path) || die ("Can't open $file_path for
> reading"); my $count =3D 0;

1. You should break these lines.

2. Put =ABmy $count =3D 0;=BB on a separate line.

>=20
> my $qry_table =3D "CREATE TABLE IF NOT EXISTS $table ( ";


> while (my $line =3D <$file_fh>) {
> last if $line =3D~ m/#/; # can place comments below the ### line

1. Is this a comment anywhere or only at the beginning of the string?

2. Always use last/next/redo/etc. with labels:

http://perl-begin.org/tutorials/bad-elements/#flow-stmts-wit hout-labels

3. You should split your code into paragraphs.

> chomp $line;
> next if (! $line); # skip any blank lines in the file...

The "!" will also match 0. Maybe do:

if ($line eq '')
{
next;
}

> $line =3D~ tr|a-zA-Z0-9,_\t \(\)||cd;
> $count++;
> my @fields =3D split (/\t/ , $line);
> push ( @fieldnames, $fields[0] );
> if ($count == 1) {
> $primary_key =3D $fields[0];
> }
> my $key =3D $fields[0];
> my $keytype =3D $fields[1];

1. You're using $fields[0] more than one time here.

2. You should unpack an array using:

my ($key, $keytype) =3D @fields;

> if ($keytype) {
> $qry_table .=3D "`$key` $keytype, ";
> }
> else {
> $qry_table .=3D "`$key` TEXT, ";
> }

1. This way you'll have a trailing L.

2. You can use =AB$keytype ||=3D 'TEXT'=BB instead here and have less dupli=
cate=20
code.

> }
> $qry_table .=3D "PRIMARY KEY (`$primary_key`) ) ENGINE=3DMyISAM DEFAULT
> CHARACTER SET latin1 COLLATE latin1_general_ci;";
>=20
> $dbh_table->do($qry_table) or die ("Can't create table!\n");
> if ($dbh_table) { say "Table '$table' was created successfully.\n" }
>=20
> close $file_fh;
>=20
> say $q->end_html();
>=20
> $dbh_table->disconnect();
>=20

Regards,

Shlomi Fish

=2D-=20
=2D--------------------------------------------------------- -------
Shlomi Fish http://www.shlomifish.org/
Escape from GNU Autohell - http://www.shlomifish.org/open-
source/anti/autohell/

Every successful open-source project will eventually spawn a sub-project.

Please reply to list if it's a mailing list post - http://shlom.in/reply .

--
To unsubscribe, e-mail: beginners-unsubscribe@perl.org
For additional commands, e-mail: beginners-help@perl.org
http://learn.perl.org/

Re: Looking for comments on code

On Jun 11, 2011, at 2:21 AM, Shlomi Fish wrote:

> Very well, I'll comment on your code.

Thanks, Shlomi. Much appreciated.

>> my $username =3D 'root';
>> my $password =3D 'root';
>> my $host =3D "127.0.0.1:8889";
>> my $db =3D "test_dbb";
> I hope you are not connecting to the database as user root with =
password root.

This is just a MAMP install for testing.


>> if ($dbh_db) { say "Database '$db' was created successfully." }
> That's the wrong way to test for errors.

What do you recommend instead?


>> my $table =3D fileparse($file_path, qr/\.[^.]*/);
> What are you trying to do here?

I'm using File::Basename to grab the file name from the path to =
use as the table name.


>> while (my $line =3D <$file_fh>) {
>> last if $line =3D~ m/#/; # can place comments below the ### =
line
> 1. Is this a comment anywhere or only at the beginning of the string?

I was placing a line of #### at the end of the file to allow me =
to add comments after it. However, I realized that someone might =
include a # in a field name, so I changed that line to be ### COMMENTS =
### instead, and now I'm testing for it like so:

last LINES if $line =3D~ m/### COMMENTS ###/;


>> if ($keytype) {
>> $qry_table .=3D "`$key` $keytype, ";
>> }
>> else {
>> $qry_table .=3D "`$key` TEXT, ";
>> }
>=20
> 1. This way you'll have a trailing L.


What do you mean by "trailing L"?


Thanks again,
Marc



--
To unsubscribe, e-mail: beginners-unsubscribe@perl.org
For additional commands, e-mail: beginners-help@perl.org
http://learn.perl.org/

Re: Looking for comments on code

On 2011-06-11 04:25, sono-io@fannullone.us wrote:

> # Connect to the server to create the db, if it doesn't exist already.
> my $dbh_db = DBI->connect("DBI:mysql:database=;$host",
> $username, $password,
> {'RaiseError' => 1});


The "RaiseError" attribute can be used to force errors to raise exceptions



> $dbh_db->do($qry_db) or die ("Can't create db!\n");

Because of RaiseError, that 'or die' is unreachable code. Etc.

--
Ruud

--
To unsubscribe, e-mail: beginners-unsubscribe@perl.org
For additional commands, e-mail: beginners-help@perl.org
http://learn.perl.org/

Re: Looking for comments on code

Since $count is nowhere used again, why increment it each time just for the sake of reading primary key. Just use a boolean "read_Primary_Key" (default false) and then true after reading the first line.


--
To unsubscribe, e-mail: beginners-unsubscribe@perl.org
For additional commands, e-mail: beginners-help@perl.org
http://learn.perl.org/

Re: Looking for comments on code

Dr. Ruud wrote:

> Because of RaiseError, that 'or die' is unreachable code

That's good to know. I wasn't aware of that. I'm reading up on =
it now. Thanks!


Gurpreet Singh wrote:

> Just use a boolean "read_Primary_Key" (default false) and then true =
after reading the first line.

Now why didn't I think of that! ;) Of course - that makes =
sense. The first time through the loop, $primary_key is undefined. So I =
added:

$primary_key =3D $key unless defined ($primary_key);

This allowed me to get rid of the redundant $count, and to =
shrink the number of lines by four, as well.

Thanks,
Marc=

--
To unsubscribe, e-mail: beginners-unsubscribe@perl.org
For additional commands, e-mail: beginners-help@perl.org
http://learn.perl.org/