Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > Perl > Perl Misc > criticize my code.. please?

Reply
Thread Tools

criticize my code.. please?

 
 
Sergei Shelukhin
Guest
Posts: n/a
 
      10-16-2004
Hi
Here I prase opml blogroll into linkage database for my embarasingly
primitive blog engine I wrote slowly to study Perl basics.
One thing I already know is that common is an evil name for custom module. I
will hcange that in production version
I am also aware of being able to refactor query-prepare out from the loop,
running away to th uni now

#!/usr/bin/perl
use strict;
use lib qw(/usr/local/lib/perl5/site_perl/5.6.1);
use lib qw(/home/virtual/html/raven.is.rathercute.com/cgi);
use DBI;
use XML:arser;
use XML:OM;
use IO::Handle;
use common;
################################################## ##########################
######

my $db = Common::connect();
my $io = new IO::Handle;
my $fname;
my $new = 0;
my $old = 0;
my $p = new XML:OM:arser();
print "OPML file name: ";
if (!$io->fdopen(fileno(STDIN),"r"))
{
die "error initialising i/o";
}
$fname = $io->getline;
my $opml = $p->parsefile($fname) or die "File not found";
my $nodes = $opml->getElementsByTagName("outline");
my $n = $nodes->getLength;
$db->do("ALTER TABLE Linkage ADD COLUMN SaveMe bit");
$db->do("UPDATE Linkage SET SaveMe = 0");
$db->do("UPDATE Linkage SET SaveMe = 1 WHERE Blog = 0 OR Feed = ''");
for (my $i = 0; $i < $n; $i++)
{
my $node = $nodes->item ($i);
next if ($node->hasChildNodes); #outline cats
my ($title,$url,$feed) = ($node->getAttributeNode("title")
,$node->getAttributeNode("htmlUrl"),$node->getAttributeNode("xmlUrl"));
$title = $title->getValue if $title;
$feed = $feed->getValue if $feed;
if ($url )
{
$url = $url->getValue
}
else
{
# print "URL for the \"$title\" not found, please supply the url: ";
# $url = $io->getline;
$url = '';
}
my $query = $db->prepare("SELECT * FROM Linkage WHERE Feed = ?");
$query->execute($feed);
if (my $row = $query->fetchrow_hashref())
{
$db->do("UPDATE Linkage SET SaveMe = 1 WHERE Feed = ?",undef,$feed);
++$old;
}
else
{
$db->do("INSERT INTO Linkage (Name,Url,Feed,Blog,SaveMe) VALUES
(?,?,?,1,1)",undef,$title,$url,$feed);
++$new;
}
}
$db->do("DELETE FROM Linkage WHERE SaveMe = 0");
$db->do("ALTER TABLE Linkage DROP COLUMN SaveMe");
print "$new new feeds, $old old feeds\n";
$io->close;
$db->disconnect();


 
Reply With Quote
 
 
 
 
Ben Morrow
Guest
Posts: n/a
 
      10-16-2004

Quoth "Sergei Shelukhin" <(E-Mail Removed)>:
> Hi
> Here I prase opml blogroll into linkage database for my embarasingly
> primitive blog engine I wrote slowly to study Perl basics.
> One thing I already know is that common is an evil name for custom module. I
> will hcange that in production version
> I am also aware of being able to refactor query-prepare out from the loop,
> running away to th uni now
>
> #!/usr/bin/perl
> use strict;


use warnings;

> use lib qw(/usr/local/lib/perl5/site_perl/5.6.1);
> use lib qw(/home/virtual/html/raven.is.rathercute.com/cgi);
> use DBI;
> use XML:arser;
> use XML:OM;
> use IO::Handle;
> use common;
> ################################################## ##########################
> ######
>
> my $db = Common::connect();

^
If you wish code to be criticised, please first make sure it runs. (Of
course, this *may* run, I suppose, if you have a case-insensitive
filesystem and the module calls itself Common internally, but then
you'll find your import is mysteriously not getting called...)

> my $io = new IO::Handle;


Why are you using IO::Handle? At least IMHO, it's much easier to use
5.6's lexical FHs.

> my $fname;
> my $new = 0;
> my $old = 0;
> my $p = new XML:OM:arser();


It is best not to declare variables until you need them.

> print "OPML file name: ";
> if (!$io->fdopen(fileno(STDIN),"r"))


Why are you doing this? What's wrong with STDIN?

> {


GNU-style indenting is not common in the Perl world.

if (...) {
# stuff
}

is much more common. See perlstyle.

> die "error initialising i/o";
> }
> $fname = $io->getline;
> my $opml = $p->parsefile($fname) or die "File not found";
> my $nodes = $opml->getElementsByTagName("outline");
> my $n = $nodes->getLength;
> $db->do("ALTER TABLE Linkage ADD COLUMN SaveMe bit");
> $db->do("UPDATE Linkage SET SaveMe = 0");
> $db->do("UPDATE Linkage SET SaveMe = 1 WHERE Blog = 0 OR Feed = ''");
> for (my $i = 0; $i < $n; $i++)


for my $i (0..$n) {

> {
> my $node = $nodes->item ($i);
> next if ($node->hasChildNodes); #outline cats
> my ($title,$url,$feed) = ($node->getAttributeNode("title")
> ,$node->getAttributeNode("htmlUrl"),$node->getAttributeNode("xmlUrl"));


Why do you use getAttributeNode instead of getAttribute?

It would be clearer to use a map here:

my ($title, $url, $feed) =
map { $node->getAttribute($_) } qw/title htmlURL xmlURL/;

> $title = $title->getValue if $title;
> $feed = $feed->getValue if $feed;
> if ($url )
> {
> $url = $url->getValue
> }
> else
> {
> # print "URL for the \"$title\" not found, please supply the url: ";
> # $url = $io->getline;
> $url = '';
> }
> my $query = $db->prepare("SELECT * FROM Linkage WHERE Feed = ?");
> $query->execute($feed);
> if (my $row = $query->fetchrow_hashref())
> {
> $db->do("UPDATE Linkage SET SaveMe = 1 WHERE Feed = ?",undef,$feed);
> ++$old;
> }
> else
> {
> $db->do("INSERT INTO Linkage (Name,Url,Feed,Blog,SaveMe) VALUES
> (?,?,?,1,1)",undef,$title,$url,$feed);
> ++$new;
> }
> }
> $db->do("DELETE FROM Linkage WHERE SaveMe = 0");
> $db->do("ALTER TABLE Linkage DROP COLUMN SaveMe");
> print "$new new feeds, $old old feeds\n";
> $io->close;
> $db->disconnect();


With warnings on you will get warned about statement handles still
existing when you disconnect. Destroy them by either scoping them so
they go out of scope before you get here (best) or calling $sth->finish.

You appear not to be using transactions; I would strongly advise setting
AutoCommit off on the database handle and doing a $db->commit at the
end.

Ben

--
I've seen things you people wouldn't believe: attack ships on fire off
the shoulder of Orion; I watched C-beams glitter in the dark near the
Tannhauser Gate. All these moments will be lost, in time, like tears in rain.
Time to die. http://www.velocityreviews.com/forums/(E-Mail Removed)
 
Reply With Quote
 
 
 
 
John W. Krahn
Guest
Posts: n/a
 
      10-16-2004
Ben Morrow wrote:
> Quoth "Sergei Shelukhin" <(E-Mail Removed)>:
>>
>>my $n = $nodes->getLength;
>>$db->do("ALTER TABLE Linkage ADD COLUMN SaveMe bit");
>>$db->do("UPDATE Linkage SET SaveMe = 0");
>>$db->do("UPDATE Linkage SET SaveMe = 1 WHERE Blog = 0 OR Feed = ''");
>>for (my $i = 0; $i < $n; $i++)

>
> for my $i (0..$n) {


for my $i ( 0 .. $n - 1 ) {


John
--
use Perl;
program
fulfillment
 
Reply With Quote
 
Ben Morrow
Guest
Posts: n/a
 
      10-17-2004

Quoth "John W. Krahn" <(E-Mail Removed)>:
> Ben Morrow wrote:
> > Quoth "Sergei Shelukhin" <(E-Mail Removed)>:
> >>
> >>for (my $i = 0; $i < $n; $i++)

> >
> > for my $i (0..$n) {

>
> for my $i ( 0 .. $n - 1 ) {


Gah, yes of course; sorry...

Ben

--
If I were a butterfly I'd live for a day, / I would be free, just blowing away.
This cruel country has driven me down / Teased me and lied, teased me and lied.
I've only sad stories to tell to this town: / My dreams have withered and died.
(E-Mail Removed) (Kate Rusby)
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
Please criticize my website. Luigi Donatello Asero HTML 336 10-23-2005 08:29 PM
Re: Please criticize my website. Luigi Donatello Asero HTML 25 09-28-2005 08:17 AM
Re: Please criticize my website. Luigi Donatello Asero HTML 72 09-26-2005 12:29 AM
Re: Please criticize my website. Luigi Donatello Asero HTML 0 09-24-2005 01:27 PM
Re:Please criticize my website. Luigi Donatello Asero HTML 8 09-21-2005 03:03 PM



Advertisments