csiepka has asked for the wisdom of the Perl Monks concerning the following question:

I have this script:
#!/usr/bin/perl use warnings; use strict; my $dircom="completes"; my (@exch, @files, @completefile); my ($su_adapter,$su_date,$su_time,$su_gateway,$su_message); my ($dc_adapter,$dc_date,$dc_time,$dc_gateway,$dc_message); my ($file, $exch); my ($line, $downloadline, $startline, $i, $in); opendir MYDIR, "$dircom"; @files = grep !/^\.\.?$/, readdir MYDIR; close (MYDIR); @exch = ("EXCH1","EXCH2","EXCH3","EXCH4","EXCH5"); open (LIS, "+>list.txt") || die ("Can't open list.txt"); foreach $file(@files) { open (FIL, "completes\\$file") || die ("test"); my $cnt=0; while (<FIL>) { $in="$_"; chomp($in); $completefile[$cnt]="$in"; $cnt++; }; close FIL; print LIS "\n$file\n"; foreach $exch(@exch) { for($i=0; $i<=$#completefile; $i++) { #no warnings 'uninitialized'; if($completefile[$i]=~ m/^.+\\(.+)_.+\..+?:(.+?)\s-\s(.+?) +\s.+\/($exch)\s\|.+::(.*Up)\(\)/) { $su_adapter=$1; $su_date=$2; $su_gateway=$4; $su_message=$5; $su_time=$3; $line=$i; }; }; for($i=0; $i<=$#completefile; $i++) { #no warnings 'uninitialized'; if($completefile[$i]=~ m/^.+\\(.+)_.+\..+?:(.+?)\s-\s(.+?) +\s\|\s.+?\/($exch)\s\|\s([Cc]ontract download complete)/){ $dc_adapter=$1; $dc_date=$2; $dc_gateway=$4; $dc_message=$5; $dc_time=$3; $downloadline=$i; }; }; for($i=0; $i<=$#completefile; $i++) { #no warnings 'uninitialized'; if($i == $line) { print LIS "$su_adapter, $su_date, $su_time, $su_gatewa +y, $su_message\n"; }; if($i == $downloadline) { print LIS "$dc_adapter, $dc_date, $dc_time, $dc_gatewa +y, $dc_message\n"; }; }; }; }; close LIS;
Sorry guys/gals, I should have posted this right from the beginning.... But here is the entire script.

... where I need to grab and use the info in the matched string. My problem is that I can't seem to figure out how to clear the variable for the next match. If the "$exch" doesn't exist in the file, it continues on, but keeps and prints out the info from the previous run where the $exch existed. So I end up getting data from a previous match listed in the next output. I've undef'd the $su_adapter, etc, but I believe it's being held in the $1, $2, because there was nothing to overwrite it.

I hope I'm explaining the well enough....

Replies are listed 'Best First'.
Re: undef a variable for a loop
by ikegami (Patriarch) on May 15, 2009 at 16:53 UTC

    I don't see why you have any problems. Cleaned up:

    for my $exch (@exch) { for my $i (0 .. $#completefile) { if (my ($su_adapter, $su_date) = $completefile[$i] =~ /.../) { print $su_adapter, $su_date; } } }

    Or is the print outside of the if? That wouldn't make any sense.

      Actually the print IS outside of the if, and that's only because all I want is the final match for the $exch, etc of each file.
        That's what it does already thanks to the leading /.+/. Well, you might have to replace the leading /.+/ with /(?s:.)+/ so that it matches newlines.
        Based on this additional information, you probably mean something like this:

        foreach $exch(@exch) { undef($su_adapter); undef($su_date); for($i=0; $i<=$#completefile; $i++) { if($completefile[$i]=~ m/^.+\\(.+)_.+\..+?:(.+?)\s-\s(.+?)\s.+ +?\/$exch\s\|.+::(.*Up)\(\)/) { $su_adapter=$1; $su_date=$2; } } print $su_adapter, $su_date; }

        Note that there are a bunch of easy modifications (as shown in ikegami's post) that can simplify your code and reduce its predilection for bugs.

Re: undef a variable for a loop
by kennethk (Abbot) on May 15, 2009 at 16:16 UTC

    Your posted code is a little sparse, particularly since the nesting structure of your code is ambiguous (You include several unterminated blocks, so we have to guess what level your statements exist at). If I assume your assignments with $1 and $2 are within your if block, you could simply add an else block where you explicitly undef your $su_adapter and $su_date. If this is incorrect, please post sufficient code (or pseudocode) for us to feed directly into our interpreters without getting compilation errors.

    A comment on formatting - once you have wrapped something in code tags, all characters are displayed literally, so there's no need for <br> tags. See Markup in the Monastery.

    Update: With the changes made to the OP, this answer is no longer relevant -- see Re: undef a variable for a loop.

      Got it, thank you kennethk!
Re: undef a variable for a loop
by kennethk (Abbot) on May 15, 2009 at 22:21 UTC
    Given the repost, a new thread seemed appropriate. There are a number of changes I would suggest, listed below:
    1. You should avoid just creating a series of script globals at the start of a file. Since my lets you create scoped variables, take advantage of that and keep your variables scoped. Proper scoping would have solved your problem, since the output variables would be set to undef on every iteration of the loop.
    2. If you do have script parameters, you should collect them at the top of the file. I found three values I thought that applied to (list.txt, the @exch list and $dircon).
    3. By using a lexical file handle instead of a bareword, you allow the same kind of typographical protections on your file/directory handles you get with you other variables.
    4. You should should use the three-argument form of open, especially when taking outside input for your file names. A maliciously named file in your "contents" directory would allow execution of arbitrary code.
    5. Foreach Loops are generally considered preferable to For Loops since they require less typing and are less prone to bugs.
    6. If you are going to read an entire file, angle brackets (<>) in list context return the entire contents of the file, split on the input record separator $/. As well, chomp acts on lists.
    7. Given how you were handling your outputs, it's dramatically easier to have 1 array vs. 5 scalars.
    8. Since you only want the last occurrence of your variables, why not start at the end of your file, and break with a last on the first hit?
    9. A regular expression in list context returns the capture values from the expression, allowing you to avoid using the variables $1, $2, ...
    10. A curly bracket closing a block does not require a semicolon.

    For you consideration, I'm implemented all of these comments into your newly posted code:

    #!/usr/bin/perl use warnings; use strict; my $dircom = "completes"; my $list_file = "list.txt"; my @exch = qw(EXCH1 EXCH2 EXCH3 EXCH4 EXCH5); opendir my($mydir), $dircom; my @files = grep !/^\.\.?$/, readdir $mydir; close ($mydir); open (my $lis, "+>", $list_file) || die ("Can't open $list_file"); foreach my $file (@files) { open (my $fil, "$dircom\\$file") || die ("test"); my @completefile = <$fil>; chomp @completefile; close $fil; print $lis "\n$file\n"; foreach my $exch (@exch) { my @su_output = (); my @dc_output = (); foreach (reverse @completefile) { @su_output = /^.+\\(.+)_.+\..+?:(.+?)\s-\s(.+?)\s.+\/($exc +h)\s\|.+::(.*Up)\(\)/; last if @su_output; } foreach (reverse @completefile) { @dc_output = /^.+\\(.+)_.+\..+?:(.+?)\s-\s(.+?)\s.+\/($exc +h)\s\|.+::(.*Up)\(\)/; last if @dc_output; } if(@su_output) { print $lis join(",",@su_output), "\n"; } else { print $lis "No su results for $exch"; } if(@dc_output) { print $lis join(",",@dc_output), "\n"; } else { print $lis "No dc results for $exch"; } } } close $lis;
      I really hope that one day I can write scripts like you just did. I'm still learning - something new every day. Thing is I love it and I hope to master it. I won't take your suggestions lightly and I appreciate the time you took to help me out. Thank you very much Kennethk! :)