criticize my code.. please?

[Sergei Shelukhin]

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();

 

[Ben Morrow]

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

 

[John W. Krahn]

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

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


John

 

[Ben Morrow]

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

Gah, yes of course; sorry...

Ben