criticize my code.. please?
 

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::Parser;
use XML::DOM;
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::DOM::Parser();
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();

Re: criticize my code.. please?
 

Quoth "Sergei Shelukhin" <raven_at@home.domonet.ru>:
> 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::Parser;
> use XML::DOM;
> 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::DOM::Parser();

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. ben@morrow.me.uk

Re: criticize my code.. please?
 

Ben Morrow wrote:
> Quoth "Sergei Shelukhin" <raven_at@home.domonet.ru>:
>>
>>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

Re: criticize my code.. please?
 

Quoth "John W. Krahn" <krahnj@telus.net>:
> Ben Morrow wrote:
> > Quoth "Sergei Shelukhin" <raven_at@home.domonet.ru>:
> >>
> >>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.
ben@morrow.me.uk (Kate Rusby)