criticize my code.. please?

Discussion in 'Perl Misc' started by Sergei Shelukhin, Oct 16, 2004.

  1. 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();
     
    Sergei Shelukhin, Oct 16, 2004
    #1
    1. Advertising

  2. Sergei Shelukhin

    Ben Morrow Guest

    Quoth "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 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, Oct 16, 2004
    #2
    1. Advertising

  3. Ben Morrow wrote:
    > Quoth "Sergei Shelukhin" <>:
    >>
    >>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
     
    John W. Krahn, Oct 16, 2004
    #3
  4. Sergei Shelukhin

    Ben Morrow Guest

    Quoth "John W. Krahn" <>:
    > Ben Morrow wrote:
    > > Quoth "Sergei Shelukhin" <>:
    > >>
    > >>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.
    (Kate Rusby)
     
    Ben Morrow, Oct 18, 2004
    #4
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Luigi Donatello Asero

    Please criticize my website.

    Luigi Donatello Asero, Sep 21, 2005, in forum: HTML
    Replies:
    336
    Views:
    7,700
    Luigi Donatello Asero
    Oct 23, 2005
  2. Luigi Donatello Asero

    Re:Please criticize my website.

    Luigi Donatello Asero, Sep 21, 2005, in forum: HTML
    Replies:
    8
    Views:
    513
    Luigi Donatello Asero
    Sep 21, 2005
  3. Luigi Donatello Asero

    Re: Please criticize my website.

    Luigi Donatello Asero, Sep 21, 2005, in forum: HTML
    Replies:
    72
    Views:
    2,014
  4. Luigi Donatello Asero

    Re: Please criticize my website.

    Luigi Donatello Asero, Sep 23, 2005, in forum: HTML
    Replies:
    25
    Views:
    964
    Neredbojias
    Sep 28, 2005
  5. Ray

    Please Criticize My Code

    Ray, Aug 20, 2005, in forum: Python
    Replies:
    9
    Views:
    330
Loading...

Share This Page