in reply to If line matches, print column, else print file name

Don't close your file handle within the while loop reading it.
I'm getting "Use of uninitialized value" warnings, when I initialize all variables with "my" beforehand.
You got it wrong, "my" is used to declare a variable, but it doesn't initialized the variable. Initializing a variable is to give it an initial value.

If you correct the split (as per GotToBTru's previous answer) and move the closing of the filehandle after the end of the while loop, you'll probably have less of these messages, because many of them seem to come from the fact that you are reading from a filehandle that has been closed too early, so that you don't get anything into the $_ variable and the @fields array.

Replies are listed 'Best First'.
Re^2: If line matches, print column, else print file name
by Yakup (Novice) on Oct 26, 2016 at 11:33 UTC

    Laurent_R, GotToBTru thanks a lot to both of you for pointing out mistakes in my code. I don't know how could I not notice them before. I have followed your advices to edit the code and also edited few other things and now it works as expected.

    #!/usr/bin/env perl use strict; use warnings; use File::Basename; my $lab_root = dirname $0; opendir( DH, $lab_root) or die "Cannot open $lab_root: $!\n"; my @kickstarts = grep ( /\.ks$/, readdir(DH)); my @bsname ; my @predefined; my $hostname; for my $kickstart (@kickstarts) { open my $fh, $kickstart or die "Cannot open $kickstart: $!"; while (<$fh>) { chomp; if ( /(?s)^((?!#).)*--hostname=/ ) { my @fields = split /[=\s]/; $hostname = $fields[1]; push @bsname , $hostname; push @predefined , $kickstart; } } close $fh; next if $kickstart ~~ @predefined; $hostname = (split /\./, $kickstart )[0]; push @bsname, $hostname; }

      Hi Yakup,

      Glad to hear your code is doing what you want. I do have to admit I found the logic a bit hard to follow, but that isn't necessarily a problem. I would however recommend you run the code through enough test cases, just to make sure.

      I just wanted to point out a few potential improvements:

      • Using FindBin to locate the directory that the Perl script is located in is much more reliable than dirname $0.
      • Using glob is easier than opendir+readdir (with the caveat that glob does things a little bit differently, for example it ignores files whose name begin with a dot by default; see File::Glob for all the details); alternatively there are modules like Path::Class or Path::Tiny to help.
      • This is just a stylistic thing, but personally I wouldn't make the regex as complex. I might write something like next if /^\s*#/; if (/--hostname=/) ...
      • Smart match (~~) has been placed into experimental status and its implementation might change in the future, so in this case something like List::Util's any is better for now. Or, if all you're using @predefined for is to check whether certain strings have been seen before, a hash would be much better.
      • The code (split /\./, $kickstart)[0] will only return the first part of $kickstart up to the first dot anywhere in the string, so it will not do what you want (strip the .ks suffix) for strings like "some.file.name.ks" or "/some.path/some.file.ks". I would recommend a regex, or the all-out solution is File::Basename's fileparse.
      • Also, I'd recommend the three-argument form of open, and to limit the scope of your variable declarations to where they are needed.

      If I implement those suggestions, I get:

      #!/usr/bin/env perl use strict; use warnings; use FindBin; use File::Basename qw/fileparse/; use List::Util qw/any/; my $lab_root = $FindBin::Bin; my @kickstarts = glob "$lab_root/*.ks"; my @bsname; my @predefined; for my $kickstart (@kickstarts) { open my $fh, '<', $kickstart or die "Cannot open $kickstart: $!"; while (<$fh>) { chomp; next if /^\s*#/; if (/--hostname=/) { my @fields = split /[=\s]/; push @bsname, $fields[1]; push @predefined, $kickstart; } } close $fh; next if any {$kickstart eq $_} @predefined; my $hostname = fileparse($kickstart, qr/\.ks$/i); push @bsname, $hostname; }

      Note that in this code compared to yours, the filenames now all have their pathnames prefixed. Since I don't know what the rest of your code does, this may or may not be a problem for you. If it is, you could for example use the same fileparse function to get only the filename.

      Hope this helps,
      -- Hauke D

        Hello Hauke, thanks for your comment! I find your suggestions very helpful, especially glob and fileparse functions really make things easier and more flexible.