5
\$\begingroup\$

I am a Java programmer and very new to Perl. I decided to work on a simple script that could solve a problem I encounter in my day to day work. This script will search the log file for a pattern and print the complete potentially multi line log statement that has a match for this pattern. Additionally, the matched pattern should be displayed in color for highlighting purpose.

I've used very basic features of Perl and as you can see, the code very verbose (like Java some might say ;) ) and there are two main areas I'm aiming to improve:

  • I want to see Perl's idiomatic ways to make the program less verbose without sacrificing too much readability.
  • Any poor practice is that I have used. I purpose fully left out "strict" and "warnings" as I want to use Perl for loose and less verbose features.

I'd be really grateful for some feedback to make this more elegant. I'll only be using Perl 5.8 as that is what available on most systems at my place.

#!/usr/bin/env perl

use Term::ANSIColor;

my $pattern = qr"[.a-zA-Z0-9]*Exception";
my $hilight = 'black on_yellow';
my $timestamp = qr"\d\d:\d\d"; #To identify beginning of new log entry.

my $begin = qr/^$timestamp/;

open(my $fn, '<', $ARGV[0]) or die "Could not open file '$ARGV[0]' $!";
while (<$fn>) {
    if ( $_ =~ m/($begin)/ ) {
        if($found) {
            foreach(@fh) {
                if ( $_ =~ m/($pattern)/ ) {
                    $matched = $1;
                    @parts = split /($pattern)/, $_;
                    foreach(@parts) {
                        if ($_ eq $matched) {
                            print colored($matched, $hilight);
                        } else {
                            print $_;
                        }
                    }
                } else {
                    print $_
                }
            }
        }

        if(defined @fh) {
            undef @fh;
        }
        $found = 0;
    }
    if ( $_ =~ m/($pattern)/ ) {
        $found = 1;
    }
    push (@fh, $_);
}

close $fn;
\$\endgroup\$
1
  • 3
    \$\begingroup\$ I'd put strict and warnings back for anything longer than 3 lines. In case of this script, you only need to decide the scope of $found and @fh. \$\endgroup\$
    – choroba
    Dec 2, 2016 at 8:04

1 Answer 1

3
\$\begingroup\$

The code definitely isn't bad. I'd still use strict and warnings, though. Minor remarks:

  • Why do you use " for qr? / or {} are canonical.
  • $_ is good for short loops, and its advantage is you don't have to type it. So, I'd name the main loop's variable, as its scope is rather wide.
  • Avoid deep indentation. Use subroutines.
  • defined @fh is deprecated.

My version:

#!/usr/bin/env perl
use warnings;
use strict;

use Term::ANSIColor;

my $pattern = qr/[.a-zA-Z0-9]*Exception/;


sub output {
    my @buffer = @_;
    my $hilight = 'black on_yellow';
    for (@buffer) {
        if (my ($matched) = /($pattern)/) {
            for (split /($pattern)/) {
                print $_ eq $matched ? colored($matched, $hilight) : $_;
            }
        } else {
            print;
        }
    }
}


my $begin   = qr/^\d\d:\d\d/;

my ($found, @buffer);
open my $fh, '<', $ARGV[0] or die "Could not open file '$ARGV[0]' $!";
while (my $line = <$fh>) {
    if ($line =~ /($begin)/) {
        output(@buffer) if @buffer && $found;
        @buffer = ();
        undef $found;
    }
    push @buffer, $line;
    $found = 1 if $line =~ /$pattern/;
}

# Output the last match if it ends with an EOF instead of a timestamp.
output(@buffer) if @buffer && $found;
\$\endgroup\$
6
  • \$\begingroup\$ Good. I did not knew how to pass array to sub routines and script kind of grew to deep nesting. I think you forgot to print $_. The first print statement should be print $_ eq $matched ? colored($matched, $hilight) : $_; \$\endgroup\$
    – sadiq.ali
    Dec 2, 2016 at 9:10
  • 1
    \$\begingroup\$ @sadiq.ali: print without parameters prints $_. I tested my script before posting. \$\endgroup\$
    – choroba
    Dec 2, 2016 at 9:56
  • \$\begingroup\$ I meant the print statement where you are using ternary operator to print colored text. In case of false, we have to print uncolored output, but you are giving empty set (). I ran in local system and for lines with matching pattern, your program is only printing matched pattern and ignoring other parts of the line. \$\endgroup\$
    – sadiq.ali
    Dec 2, 2016 at 10:31
  • \$\begingroup\$ @sadiq.ali: Ah, you're right. \$\endgroup\$
    – choroba
    Dec 2, 2016 at 10:51
  • 1
    \$\begingroup\$ I'd also rename $fn to $fh because that's more canonical. In the original code I got confused because of @fh and $fn. \$\endgroup\$
    – simbabque
    Dec 2, 2016 at 14:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.