views:

170

answers:

2

I have to maintain the following Perl script:

#!/usr/bin/perl -w

die "Usage: $0 <file1> <file2>\n" unless scalar(@ARGV)>1;

undef $/;
my @f1 = split(/(?=(?:SERIAL NUMBER:\s+\d+))/, <>);
my @f2 = split(/(?=(?:SERIAL NUMBER:\s+\d+))/, <>);

die "Error: file1 has $#f1 serials, file2 has $#f2\n" if ($#f1 != $#f2);

foreach my $g (0 .. $#f1) {
    print (($f2[$g] =~ m/RESULT:\s+PASS/) ? $f2[$g] : $f1[$g]);
}

print STDERR "$#f1 serials found\n";

I know pretty much what it does, but how it's done is difficult to follow. The calls to split() are particulary puzzling.

It's fairly idiomatic Perl and I would be grateful if a Perl expert could make a few clarifying suggestions about how it does it, so that if I need to use it on input files it can't deal with, I can attempt to modify it.

It combines the best results from two datalog files containing test results. The datalog files contain results for various serial numbers and the data for each serial number begins and ends with SERIAL NUMBER: n (I know this because my equipment creates the input files)

I could describe the format of the datalog files, but I think the only important aspect is the SERIAL NUMBER: n because that's all the Perl script checks for

The ternary operator is used to print a value from one input file or the other, so the output can be redirected to a third file.

+6  A: 

The undef $/ line deactivates the input record separator. Instead of reading records line by line, the interpreter will read whole files at once after that.

The <>, or 'diamond operator' reads from the files from the command line or standard input, whichever makes sense. In your case, the command line is explicitely checked, so it will be files. Input record separation has been deactivated, so each time you see a <>, you can think of it as a function call returning a whole file as a string.

The split operators take this string and cut it in chunks, each time it meets the regular expression in argument. The (?= ... ) construct means "the delimiter is this, but please keep it in the chunked result anyway."

That's all there is to it. There would always be a few optimizations, simplifications, or "other ways to do it," but this should get you running.

JB
Perhaps I should have shown as much focus on the problem area. +1 for a significantly less daunting explanation than mine.
Chris Lutz
Thanks JB for a different perspective. This idea of chunks fits the observed behaviour. The script cuts the datalog files into chunks and puts together a third file (by redirection) which consists of the best bits (judging by RESULT:) from the two input files. It worried me at first that if corresponding serial numbers from BOTH files passed it would use the chunk from @f2, but I think it's ok: after all, they BOTH PASSED. I'm always careful to use this script for combining two files containing test data for the same product, and only in emergencies.
pavium
BTW it was a good explanation, too. That Chris Lutz guy told me some stuff I already knew. Don't tell him I said that.
pavium
+7  A: 

This may not be what I would call "idiomatic" (that would be use Module::To::Do::Task) but they've certainly (ab)used some language features here. I'll see if I can't demystify some of this for you.

die "Usage: $0 <file1> <file2>\n" unless scalar(@ARGV)>1;

This exits with a usage message if they didn't give us any arguments. Command-line arguments are stored in @ARGV, which is like C's char **argv except the first element is the first argument, not the program name. scalar(@ARGV) converts @ARGV to "scalar context", which means that, while @ARGV is normally a list, we want to know about it's scalar (i.e. non-list) properties. When a list is converted to scalar context, we get the list's length. Therefore, the unless conditional is satisfied only if we passed no arguments.

This is rather misleading, because it will turn out your program needs two arguments. If I wrote this, I would write:

die "Usage: $0 <file1> <file2>\n" unless @ARGV == 2;

Notice I left off the scalar(@ARGV) and just wrote @ARGV. The scalar() function forces scalar context, but if we're comparing equality with a number, Perl can implicitly assume scalar context.

undef $/;

Oof. The $/ variable is a special Perl built-in variable that Perl uses to tell what a "line" of data from a file is. Normally, $/ is set to the string "\n", meaning when Perl tries to read a line it will read up until the next linefeed (or carriage return/linefeed on Windows). Your writer has undef-ed the variable, though, which means when you try to read a "line", Perl will just slurp up the whole file.

my @f1 = split(/(?=(?:SERIAL NUMBER:\s+\d+))/, <>);

This is a fun one. <> is a special filehandle that reads line-by-line from each file given on the command line. However, since we've told Perl that a "line" is an entire file, calling <> once will read in the entire file given in the first argument, and storing it temporarily as a string.

Then we take that string and split() it up into pieces, using the regex /(?=(?:SERIAL NUMBER:\s+\d+))/. This uses a lookahead, which tells our regex engine "only match if this stuff comes after our match, but this stuff isn't part of our match," essentially allowing us to look ahead of our match to check on more info. It basically splits the file into pieces, where each piece (except possibly the first) begins with "SERIAL NUMBER:", some arbitrary whitespace (the \s+ part), and then some digits (the \d+ part). I can't teach you regexes, so for more info I recommend reading perldoc perlretut - they explain all of that better than I ever will.

Once we've split the string into a list, we store that list in a list called @f1.

my @f2 = split(/(?=(?:SERIAL NUMBER:\s+\d+))/, <>);

This does the same thing as the last line, only to the second file, because <> has already read the entire first file, and storing the list in another variable called @f2.

die "Error: file1 has $#f1 serials, file2 has $#f2\n" if ($#f1 != $#f2);

This line prints an error message if @f1 and @f2 are different sizes. $#f1 is a special syntax for arrays - it returns the index of the last element, which will usually be the size of the list minus one (lists are 0-indexed, like in most languages). He uses this same value in his error message, which may be deceptive, as it will print 1 fewer than might be expected. I would write it as:

die "Error: file $ARGV[0] has ", $#f1 + 1, " serials, file $ARGV[1] has ", $#f2 + 1, "\n"
  if $#f1 != $#f2;

Notice I changed "file1" to "file $ARGV[0]" - that way, it will print the name of the file you specified, rather than just the ambiguous "file1". Notice also that I split up the die() function and the if() condition on two lines. I think it's more readable that way. I also might write unless $#f1 == $#f2 instead of if $#f1 != $#f2, but that's just because I happen to think != is an ugly operator. There's more than one way to do it.

foreach my $g (0 .. $#f1) {

This is a common idiom in Perl. We normally use for() or foreach() (same thing, really) to iterate over each element of a list. However, sometimes we need the indices of that list (some languages might use the term "enumerate"), so we've used the range operator (..) to make a list that goes from 0 to $#f1, i.e., through all the indices of our list, since $#f1 is the value of the highest index in our list. Perl will loop through each index, and in each loop, will assign the value of that index to the lexically-scoped variable $g (though why they didn't use $i like any sane programmer, I don't know - come on, people, this tradition has been around since Fortran!). So the first time through the loop, $g will be 0, and the second time it will be 1, and so on until the last time it is $#f1.

    print (($f2[$g] =~ m/RESULT:\s+PASS/) ? $f2[$g] : $f1[$g]);

This is the body of our loop, which uses the ternary conditional operator ?:. There's nothing wrong with the ternary operator, but if the code gives you trouble we can just change it to an if(). Let's just go ahead and rewrite it to use if():

    if($f2[$g] =~ m/RESULT:\s+PASS/) {
        print $f2[$g];
    } else {
        print $f1[$g];
    }

Pretty simple - we do a regex check on $f2[$g] (the entry in our second file corresponding to the current entry in our first file) that basically checks whether or not that test passed. If it did, we print $f2[$g] (which will tell us that test passed), otherwise we print $f1[$g] (which will tell us the test that failed).

print STDERR "$#f1 serials found\n";

This just prints an ending diagnostic message telling us how many serials were found (minus one, again).

I personally would rewrite that whole hairy bit where he hacks with $/ and then does two reads from <> to be a loop, because I think that would be more readable, but this code should work fine, so if you don't have to change it too much you should be in good shape.

Chris Lutz
Thanks Chris, that was a great explanation. It's pretty obvious that the script was written on golfing principles. I think modules were avoided because this kind of scripting was started here on peculiar one-of-a-kind equipment and there was no idea of using 'external' resources. I'm not even sure what modules are installed and how more could be added.
pavium
I don't think a module is strictly necessary for this particular task - my `use Module::To::Do::Task` was a joke, mostly - but it couldn't hurt to try. There may be a module out there for parsing and working with a data format that is very close to what you have, and modifying it slightly to handle your format should be easy. Although the tool you have certainly works, so don't throw it away immediately, and don't look to replace it just because it's kind of hack-y.
Chris Lutz
Since the impression I got from your explanation is that the code isn't actually BROKEN (well ... I knew that, it works) I'd be better off leaving it alone, so I will.
pavium
@Chris Lutz A clarification: Just using `@ARGV` in an `if`, `while`, `unless`, `until` is enough to provide scalar context.
Sinan Ünür