while loop into database insert not working

Discussion in 'Perl Misc' started by Unknown, Apr 15, 2005.

  1. Unknown

    Unknown Guest

    Hello All,

    I am working on this script which will parse several Backup Exec logs
    and place key details into a database for viewing as statistics
    on a web based front end. I posted all the code, but I am only having
    trouble with the while loop (inside the foreach). It is not behaving the
    way I expect it to in that it prints several copies of each record
    instea of just one record for one text file.

    thanks
    Rocky



    #!/usr/bin/perl
    use warnings;
    use strict;
    use DBI;
    use DBD::pg;

    my $filedir = '/var/backuplogs';
    my $servername;
    my $jobname;
    my $jobstarted;
    my $medialabel;
    my $logfile;
    my $jobended;
    my $status;
    my @inuse;
    my @skipped;
    my @corrupt;
    my @skippednames;
    my $skipped;
    my $corrupt;
    my $inuse;


    opendir(DIR, $filedir) or die "could not open logfile directory: $!";
    my @filestoparse = grep { /\.txt$/ } readdir(DIR);
    closedir(DIR);

    foreach my $filename (@filestoparse) {
    my $complete = "$filedir/" . "$filename";
    open(FH, "$complete") or die "Cannot find file $complete: $!";
    while (<FH>) {
    if ($_ =~ /^Job\sserver:\s+(.*)$/) { $servername = $1;}
    if ($_ =~ /^Job\sname:\s+(.*)$/) { $jobname = $1;}
    if ($_ =~ /^Job\sstarted:\s+(.*)$/) { $jobstarted = $1;}
    if ($_ =~ /^Media.*Label:\s+(.*)$/) { $medialabel = $1;}
    if ($_ =~ /^Job\sended:\s+(.*)$/) { $jobended = $1;}
    if ($_ =~ /^Job\scompletion\sstatus:\s+(.*)$/) { $status = $1;}
    if ($_ =~ /^(\d+)\s+files\swere\sin.*$/) {push(@inuse, $1);}
    if ($_ =~ /^(\d+)\s+items/) {push(@skipped, $1);}
    if ($_ =~ /^(\d+)\s+corrupt/) {push(@corrupt, $1);}
    if ($_ =~ /^.*item(.*\s-\sskipped.*)$/) {push(@skippednames, $1);}

    $corrupt = &addnums(@corrupt);
    $skipped = &addnums(@skipped);
    $inuse = &addnums(@inuse);
    $logfile = $filename;

    }
    close(FH);
    print "$jobname,$jobstarted,$logfile,$servername\n";

    my $dbh = DBI->connect( "dbi:pg:dbname=backup", "[snip]", "[snip]") or die
    "Cannot connect to PGSQL DB\n"; my $sth = $dbh->prepare("INSERT INTO
    backuplogs(jobname,jobstarted,logfile,servername,status,inuse,jobended,skipped,corrupt,medialabel,testlogfile)
    VALUES
    ('$jobname','$jobstarted','$logfile','$servername','$status','$inuse','$jobended','$skipped','$corrupt','$medialabel','<a
    href=http://[snip]/oldlogs/$logfile>$logfile</a>')")
    or die "Error preparing row: $DBI::errstr\n"; $sth->execute() or die "
    $DBI::errstr\n"; $dbh->disconnect;
    $servername = "";
    $jobname = "";
    $jobstarted = "";
    $medialabel = "";
    $jobended = "";
    $status = "";
    }


    sub addnums {
    my @number = @_;
    my $total;
    my $digit;
    foreach $digit (@number) {
    $total += $digit;
    }

    if (defined($total)) {
    $total = $total;
    }
    else
    {$total = 0;}
    return $total;
    }
    Unknown, Apr 15, 2005
    #1
    1. Advertising

  2. Unknown

    Guest

    Unknown <> wrote:
    > I am working on this script which will parse several Backup Exec logs
    > and place key details into a database for viewing as statistics
    > on a web based front end. I posted all the code, but I am only having
    > trouble with the while loop (inside the foreach). It is not behaving the
    > way I expect it to in that it prints several copies of each record
    > instea of just one record for one text file.


    I can see nothing obviously wrong with it... although the while loop has
    no print statements - the only print statement is within the foreach
    loop.

    > foreach my $filename (@filestoparse) {
    > my $complete = "$filedir/" . "$filename";
    > open(FH, "$complete") or die "Cannot find file $complete: $!";
    > while (<FH>) {
    > [snip]
    > }
    > close(FH);
    > print "$jobname,$jobstarted,$logfile,$servername\n";


    In such cases I would place a few diagnostic print statements to make
    sure that your data is what you are expecting - e.g. the list of files
    to be opened and so on.

    Axel
    , Apr 15, 2005
    #2
    1. Advertising

  3. Unknown <> wrote:

    > I am working on this script which will parse several Backup Exec
    > logs and place key details into a database for viewing as
    > statistics on a web based front end. I posted all the code, but
    > I am only having trouble with the while loop (inside the
    > foreach). It is not behaving the way I expect it to in that it
    > prints several copies of each record instea of just one record
    > for one text file.


    I can't tell what might be causing that behaviour, but I do have a
    few comments that I hope will be helpful.

    By the way, I do hope your code is formatted better than it appears
    in this post. Proper indentation makes code much easier to read
    and debug -- and makes it more likely that someone in this newsgroup
    will bother to try and help.

    > #!/usr/bin/perl
    > use warnings;
    > use strict;
    > use DBI;
    > use DBD::pg;
    >
    > my $filedir = '/var/backuplogs';
    > my $servername;
    > my $jobname;
    > my $jobstarted;
    > my $medialabel;
    > my $logfile;
    > my $jobended;
    > my $status;
    > my @inuse;
    > my @skipped;
    > my @corrupt;
    > my @skippednames;
    > my $skipped;
    > my $corrupt;
    > my $inuse;


    Most of these variables are only used inside the foreach loop, so
    it's best to declare them at the top of the loop, confining their
    scope to ONLY the foreach loop. That way you don't have to worry
    about re-initializing them at the end of the loop. (Maybe this is
    the problem?)

    > opendir(DIR, $filedir) or die "could not open logfile directory: $!";
    > my @filestoparse = grep { /\.txt$/ } readdir(DIR);
    > closedir(DIR);
    >
    > foreach my $filename (@filestoparse) {


    Why not just say

    foreach my $logname (@filestoparse) {

    Then you would have one fewer variable to keep track of.

    > my $complete = "$filedir/" . "$filename";


    This is slightly odd. The double quotes around $filename are
    unnecessary. See 'perldoc -q quoting'. If you really insist
    on using the string concatenation operator, I'd probably
    write it as

    my $complete = $filedir . '/' . $filename;

    but it's probably easiest to read and write as

    my $complete = "$filedir/$filename";

    Or, since I recommended getting rid of an extra variable,

    my $complete = "$filedir/$logname";


    > open(FH, "$complete") or die "Cannot find file $complete: $!";


    The parentheses aren't really necessary when you use the
    low-precedence or (instead of ||).

    > while (<FH>) {
    > if ($_ =~ /^Job\sserver:\s+(.*)$/) { $servername = $1;}


    You can shorten this. Pattern matches automatically look at $_
    if you don't specify a string.

    if ( /^Job\sserver:\s+(.*)$/ ) { $servername = $1;}


    > if ($_ =~ /^Job\sname:\s+(.*)$/) { $jobname = $1;}
    > if ($_ =~ /^Job\sstarted:\s+(.*)$/) { $jobstarted = $1;}
    > if ($_ =~ /^Media.*Label:\s+(.*)$/) { $medialabel = $1;}
    > if ($_ =~ /^Job\sended:\s+(.*)$/) { $jobended = $1;}
    > if ($_ =~ /^Job\scompletion\sstatus:\s+(.*)$/) { $status = $1;}
    > if ($_ =~ /^(\d+)\s+files\swere\sin.*$/) {push(@inuse, $1);}


    Later on you just add up the numbers collected in @inuse, so why not
    eliminate the array and add them up as you go?

    if ( /^(\d+)\s+files\swere\sin.*$/) { $inuse += $1 }

    > if ($_ =~ /^(\d+)\s+items/) {push(@skipped, $1);}


    Ditto....

    > if ($_ =~ /^(\d+)\s+corrupt/) {push(@corrupt, $1);}
    > if ($_ =~ /^.*item(.*\s-\sskipped.*)$/) {push(@skippednames, $1);}


    I'm not sure why this line is even here. You don't use @skippednames
    anywhere else, so why bother to collect it?

    > $corrupt = &addnums(@corrupt);


    No need for the & on the call to addnums(). If you don't need the effect
    that the & gives you, don't bother with it. See 'perldoc perlsub'.


    > $skipped = &addnums(@skipped);
    > $inuse = &addnums(@inuse);
    > $logfile = $filename;
    >
    > }
    > close(FH);
    > print "$jobname,$jobstarted,$logfile,$servername\n";
    >
    > my $dbh = DBI->connect( "dbi:pg:dbname=backup", "[snip]", "[snip]") or die
    > "Cannot connect to PGSQL DB\n";


    I'd put this outside the foreach loop. Connect ONCE, then insert new
    rows as necessary.


    > my $sth = $dbh->prepare("INSERT INTO
    > backuplogs(jobname,jobstarted,logfile,servername,status,inuse,jobended,skipped,corrupt,medialabel,testlogfile)
    > VALUES
    > ('$jobname','$jobstarted','$logfile','$servername','$status','$inuse','$jobended','$skipped','$corrupt','$medialabel','<a
    > href=http://[snip]/oldlogs/$logfile>$logfile</a>')")
    > or die "Error preparing row: $DBI::errstr\n"; $sth->execute() or die "
    > $DBI::errstr\n";
    > $dbh->disconnect;


    And I'd move the disconnect to be after the foreach loop.

    > $servername = "";
    > $jobname = "";
    > $jobstarted = "";
    > $medialabel = "";
    > $jobended = "";
    > $status = "";


    These re-initializations can be dropped when you confine the variables
    to the smallest possible scope (as mentioned above).


    > }
    >
    >
    > sub addnums {
    > my @number = @_;
    > my $total;
    > my $digit;
    > foreach $digit (@number) {
    > $total += $digit;
    > }
    >
    > if (defined($total)) {
    > $total = $total;
    > }
    > else
    > {$total = 0;}
    > return $total;
    > }


    This sub becomes unnecessary if you accumulate the sum as you go. But
    even so, it could be shorter (but not *too* short, I hope).

    sub addnums {
    return 0 unless @_;
    my $total = 0;
    local $_; # don't step on $_
    $total += $_ for @_;
    return $total;
    }
    David K. Wall, Apr 15, 2005
    #3
  4. Unknown

    Guest

    David K. Wall wrote:

    > sub addnums {
    > return 0 unless @_;


    Don't create unecessary special cases

    > my $total = 0;
    > local $_; # don't step on $_


    The for statement qualifier will localize $_ properly anyhow.

    local($_) on the other hand, will do bad things if $_ happens to be an
    alias for an element of a tied agregate.

    > $total += $_ for @_;
    > return $total;
    > }



    sub addnums {
    my $total = 0;
    $total += $_ for @_;
    return $total;
    }
    , Apr 15, 2005
    #4
  5. * David K. Wall schrieb:
    > Unknown <> wrote:
    >>
    >> my $filedir = '/var/backuplogs';


    [...]

    >> my $inuse;

    >
    > Most of these variables are only used inside the foreach loop, so
    > it's best to declare them at the top of the loop, confining their
    > scope to ONLY the foreach loop. That way you don't have to worry
    > about re-initializing them at the end of the loop. (Maybe this is
    > the problem?)


    Additionally, I propose to use a hash for this. Then you have not to
    declare all those vars and you can simplify the sql statement below.

    >
    >> opendir(DIR, $filedir) or die "could not open logfile directory: $!";
    >> my @filestoparse = grep { /\.txt$/ } readdir(DIR);
    >> closedir(DIR);
    >> foreach my $filename (@filestoparse) {
    >> my $complete = "$filedir/" . "$filename";

    >
    > Why not just say
    >
    > foreach my $logname (@filestoparse) {
    > my $complete = "$filedir/$logname";


    You can do all of this in one step by using glob(). But then you lose
    the var $logname also. If necessary, determine it afterwards, e.g.

    foreach my $complete ( glob "$filedir/*.txt" ) {
    my $hash;
    ( $hash{logfile} ) = $complete =~ m#([^/]+)$#;

    >
    >> while (<FH>) {
    >> if ($_ =~ /^Job\sserver:\s+(.*)$/) { $servername = $1;}

    >
    > You can shorten this. Pattern matches automatically look at $_
    > if you don't specify a string.
    >
    > if ( /^Job\sserver:\s+(.*)$/ ) { $servername = $1;}
    >
    >> if ($_ =~ /^Job\sname:\s+(.*)$/) { $jobname = $1;}
    >> if ($_ =~ /^Job\sstarted:\s+(.*)$/) { $jobstarted = $1;}
    >> if ($_ =~ /^Media.*Label:\s+(.*)$/) { $medialabel = $1;}
    >> if ($_ =~ /^Job\sended:\s+(.*)$/) { $jobended = $1;}
    >> if ($_ =~ /^Job\scompletion\sstatus:\s+(.*)$/) { $status = $1;}
    >> if ($_ =~ /^(\d+)\s+files\swere\sin.*$/) {push(@inuse, $1);}

    >
    > Later on you just add up the numbers collected in @inuse, so why not
    > eliminate the array and add them up as you go?
    >
    > if ( /^(\d+)\s+files\swere\sin.*$/) { $inuse += $1 }
    >
    >> if ($_ =~ /^(\d+)\s+items/) {push(@skipped, $1);}
    >> if ($_ =~ /^(\d+)\s+corrupt/) {push(@corrupt, $1);}


    Here, I prefer to put the condition behind the assignment: This saves
    many parentheses and provides more readability.

    $hash{servername} = $1 if /^Job\sserver:\s+(.*)$/;
    $hash{jobname} = $1 if /^Job\sname:\s+(.*)$/;
    $hash{jobstarted} = $1 if /^Job\sstarted:\s+(.*)$/;
    $hash{medialabel} = $1 if /^Media.*Label:\s+(.*)$/;
    $hash{jobended} = $1 if /^Job\sended:\s+(.*)$/;
    $hash{status} = $1 if /^Job\scompletion\sstatus:\s+(.*)$/;

    $hash{inuse} += $1 if /^(\d+)\s+files\swere\sin.*$/;
    $hash{skipped} += $1 if /^(\d+)\s+items/;
    $hash{corrupt} += $1 if /^(\d+)\s+corrupt/;

    >> }

    >
    >> my $sth = $dbh->prepare("INSERT INTO
    >> backuplogs(jobname,jobstarted,logfile,servername,status,inuse,jobended,skipped,corrupt,medialabel,testlogfile)
    >> VALUES
    >> ('$jobname','$jobstarted','$logfile','$servername','$status','$inuse','$jobended','$skipped','$corrupt','$medialabel','<a
    >> href=http://[snip]/oldlogs/$logfile>$logfile</a>')")
    >> or die "Error preparing row: $DBI::errstr\n"; $sth->execute() or die "$DBI::errstr\n";


    Since all needed values are gathered in the new hash ('testlogfile'
    could be added separately), we could build up this statement a little
    bit easier, I think.

    my $keys = join ',', keys %hash;
    my $bind = join ',', ('?') x keys %hash;
    my $sth = $dbh->prepare( "INSERT INTO backuplogs ($keys) VALUES ($bind)" )
    or die "Error preparing row: $DBI::errstr\n";
    $sth->execute( values %hash ) or die "$DBI::errstr\n";

    This will work because keys() and values() return their lists for one
    hash in the same order. And using binding vars in the statement (»?«)
    will be less fragile than that *ugly* self-made-quoting.

    >
    >> $servername = "";
    >> $jobname = "";
    >> $jobstarted = "";
    >> $medialabel = "";
    >> $jobended = "";
    >> $status = "";

    >
    > These re-initializations can be dropped when you confine the variables
    > to the smallest possible scope (as mentioned above).


    But the arrays @inuse, @skipped and @corrupt keep their values, there is
    no reset for them. As a result, the sums $inuse etc contain the sum of
    all numbers ever found. If this is really wanted? I don't know. But due
    to using a hash, we could delete some vars explicitly. Perhaps by

    my @keys = qw( servername jobname jobstarted medialabel jobended status );
    @hash{ @keys } = ('') x @keys;

    Otherwise its easier to declare the hash inside of the for loop.

    >
    > This sub becomes unnecessary if you accumulate the sum as you go. But
    > even so, it could be shorter (but not *too* short, I hope).
    >
    > sub addnums {
    > return 0 unless @_;


    You can throw away this line as well if you want to shorten this sub. If
    @_ is *false* the loop below won't entered, so $totel remains at zero.
    And returning zero is nothing else you're doing here ;-)

    > my $total = 0;
    > local $_; # don't step on $_
    > $total += $_ for @_;
    > return $total;
    > }


    We do not need this sub, but don't "don't step on $_" here. A for loop
    is localizing $_ implicitly. Try out:

    $_ = 'sum = ';
    my $total = 0;
    $total += $_ for 1, 2, 3;
    print $_, $total;
    __END__
    sum = 6

    regards,
    fabian


    Postscript: Finally, I want to summarize the code -- just for clarity.

    #!/usr/bin/perl
    use warnings;
    use strict;
    use DBI;
    use DBD::pg;

    my $filedir = '/var/backuplogs';
    my %hash = ();
    my $dbh = DBI->connect( "dbi:pg:dbname=backup", "[snip]", "[snip]")
    or die "Cannot connect to PGSQL DB\n";

    foreach my $file ( glob "$filedir/*.txt" ) {

    # read in the file
    open FH, '<', $file or die "Cannot open file '$file': $!";
    while ( <FH> ) {
    $hash{servername} = $1 if /^Job\sserver:\s+(.*)$/;
    $hash{jobname} = $1 if /^Job\sname:\s+(.*)$/;
    $hash{jobstarted} = $1 if /^Job\sstarted:\s+(.*)$/;
    $hash{medialabel} = $1 if /^Media.*Label:\s+(.*)$/;
    $hash{jobended} = $1 if /^Job\sended:\s+(.*)$/;
    $hash{status} = $1 if /^Job\scompletion\sstatus:\s+(.*)$/;

    $hash{inuse} += $1 if /^(\d+)\s+files\swere\sin.*$/;
    $hash{skipped} += $1 if /^(\d+)\s+items/;
    $hash{corrupt} += $1 if /^(\d+)\s+corrupt/;
    }
    close FH;
    ( $hash{logfile} ) = $file =~ m#([^/]+)$#;
    $hash{testlogfile} = "<a href=http://[snip]/oldlogs/$hash{logfile}>$hash{logfile}</a>";

    # send it to db
    my $keys = join ',', keys %hash;
    my $bind = join ',', ('?') x keys %hash;
    my $sth = $dbh->prepare( "INSERT INTO backuplogs ($keys) VALUES ($bind)" )
    or die "Error preparing row: $DBI::errstr\n";
    $sth->execute( values %hash ) or die "$DBI::errstr\n";

    # reset some values
    my @keys = qw( servername jobname jobstarted medialabel jobended status );
    @hash{ @keys } = ('') x @keys;
    }

    $dbh->disconnect;
    __END__
    Fabian Pilkowski, Apr 15, 2005
    #5
  6. <> wrote:

    > David K. Wall wrote:
    >
    >> my $total = 0;
    >> local $_; # don't step on $_

    >
    > The for statement qualifier will localize $_ properly anyhow.
    >
    > local($_) on the other hand, will do bad things if $_ happens to
    > be an alias for an element of a tied agregate.


    I don't often have a need for tied variables in the little utility
    programs I mostly write, so I don't really see the problem. Could you
    elaborate on this?
    David K. Wall, Apr 15, 2005
    #6
  7. Unknown

    Anno Siegel Guest

    David K. Wall <> wrote in comp.lang.perl.misc:
    > Unknown <> wrote:


    [good advice]

    > > sub addnums {
    > > my @number = @_;
    > > my $total;
    > > my $digit;
    > > foreach $digit (@number) {
    > > $total += $digit;
    > > }
    > >
    > > if (defined($total)) {
    > > $total = $total;
    > > }
    > > else
    > > {$total = 0;}
    > > return $total;
    > > }

    >
    > This sub becomes unnecessary if you accumulate the sum as you go. But
    > even so, it could be shorter (but not *too* short, I hope).


    It's not short enough yet :)

    > sub addnums {
    > return 0 unless @_;


    Not necessary if you have the next line.

    > my $total = 0;


    Not necessary if you have the previous line.

    > local $_; # don't step on $_


    Not necessary. A for-loop localizes $/ for you.

    > $total += $_ for @_;
    > return $total;
    > }


    So:

    sub addnums {
    my $total = 0;
    $total += $_ for @_;
    $total;
    }

    or

    use List::Util;
    *addnums = \ &List::Util::sum;

    Anno
    Anno Siegel, Apr 18, 2005
    #7
  8. David K. Wall <> wrote:

    > <> wrote:
    >
    >> David K. Wall wrote:
    >>
    >>> my $total = 0;
    >>> local $_; # don't step on $_

    >>
    >> The for statement qualifier will localize $_ properly anyhow.
    >>
    >> local($_) on the other hand, will do bad things if $_ happens to
    >> be an alias for an element of a tied agregate.

    >
    > I don't often have a need for tied variables in the little utility
    > programs I mostly write, so I don't really see the problem. Could
    > you elaborate on this?


    [sound of crickets]

    As usual, an echoing silence means I should have checked the docs.

    In perltie, the Bugs section says this:

    "Localizing tied arrays or hashes does not work. After exiting the
    scope the arrays or the hashes are not restored."

    I suppose I could read the code for the relevant modules to find out
    exactly why, but I'm not /that/ interested.
    David K. Wall, Apr 18, 2005
    #8
    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. Steven

    while loop in a while loop

    Steven, Mar 24, 2005, in forum: Java
    Replies:
    5
    Views:
    2,206
    Tim Slattery
    Mar 30, 2005
  2. Harry Zoroc
    Replies:
    1
    Views:
    908
    Gregory Vaughan
    Jul 12, 2004
  3. Uday Bidkar
    Replies:
    4
    Views:
    471
    =?ISO-8859-15?Q?Juli=E1n?= Albo
    Dec 12, 2006
  4. Replies:
    1
    Views:
    135
    Bob Barrows [MVP]
    Jul 19, 2006
  5. Isaac Won
    Replies:
    9
    Views:
    343
    Ulrich Eckhardt
    Mar 4, 2013
Loading...

Share This Page