my @files = map {&makefiles} @in;

sub
makefiles
{
    my ($count, $file) = split;

    $file =~ s/^$in_prefix//;

    my ($campaign, $month, $fname) = split /\//, $file;

    return {        # not strictly necessary, but *readable*
        campaign => $campaign,
        month => $month,
        file => $fname,
        count => $count,
        sort => "$campaign:$month:$file"
    };
}
[download]

Still, I think that the second snippet is not optimised for readability: functions like map and grep are sometimes obscure even for programmers with decent experience.

Moreover, the block that map evaluates returns a hash reference, which could easily taken as a code block.

Why not simply using the first block, pushing the hash reference into another array, not @in, like this:

push @files, { campaign => $campaign,
         month => $month,
         file => $fname,
         count => $count,
         sort => "$campaign:$month:$file" };
[download]

Just my €0.02

-- TMTOWTDI

I don't agree that grep and map should be avoided. In a Unix environment grep is a tried, tested, and well known tool. Grep in Perl is essentially the same thing. Map is a gloried foreach loop simplified to assign a list to an lvalue returned from the associated code block. In fact, the word "map" is straight out of beginning algebra where we talk about functions mapping inputs to outputs.

Which brings me to the root of this thread-- and the fact that I agree and disagree with davorg. I agree that he might refactor (and comment briefly) this routine. And I agree that map is the way to go. But why not take the code block out from inside the loops and make this an actual sub? Then we can give the block of code a name that offers a clue as to its function (alleviating some of the need for comments along those lines), and we can access the code from outside our map statement if we want/need to. And we can still call it from a map at that point:


my @files = map {func($_)} @in;
sub func{ ... }

#or if we really hate the idea of a whole sub for this
#function that keeps the block hanging around when it's 
#finished being useful
my $func = sub { ... }
my @files = map {$func->($_)} @in;
[download]


Which keeps our map from getting obfuscated due to the length of the block it surrounds-- and probably the first example I give makes it easier to introduce newer programmers to this powerful feature of Perl. It makes it very plain that what's going on here is that a list of inputs is being passed into func and mapped onto a list of outputs.

Using a sub we can throw this function after our main block and keep its internals out of sight and out of mind when we are reading the rest of the enclosing block. That way we don't obscure that layer of code with details about how we transformed the lines of the wc output into data for our program. Much more readable, imho.

Hmm. Not sure I agree the second one is clearer than the first. A multi-line map can be hard to follow, and it's not clear to me that "Creating a new array has got to be better than mysteriously transforming one."

To a great extent, understandability is in the eye of the beholder. Nonetheless, I think a couple of very minor changes or additions to your first code would have made your intent clearer, more so than using map to cons up a new array. Here's what I'd suggest:

1. Add a comment just above the loop saying # Modify input array in-place.Now your intent is very clear.
2. Name the array element instead of using $_. I personally love and use $_ a lot, but if I have to refer to it explicitly, I stop and ask whether a named variable would be better. In this case, I don't know what's in @in, so I'd have to settle for $elem, $node, or something.
3. A very minor point, but I really dislike leaning-toothpick regular expressions. I'd change the second split to use delimiters other than /.

# Modify the input array in-place
foreach my $elem (@in) {
  my ($count, $file) = split /\s+/, $elem;

  $file =~ s/^$in_prefix//;

  my ($campaign, $month, $fname) = split |/|, $file;

  $elem = {
         campaign => $campaign,
         month => $month,
         file => $fname,
         count => $count,
         sort => "$campaign:$month:$file"
  };
}
[download]

Update:
Changed split to split $elem, not $_. Thanks, runrig! (But let the goof caught by Hofmator stand. ;-)
Fixed split again. This is not my day....

I'd change the second split to use delimiters other than /.

Good point, VSarkiss, but when using a different delimiter you have to write

split m|/|;

# instead of
split |/|;
[download]

I agree fully with your suggestions and like especially your point about making the comment. This solution was not mentioned elsewhere. A perl programmer should know that $_ (or $elem) is an alias for the actual element. Nevertheless overreading this is easy, so the comment is a good solution.

-- Hofmator

Good point, VSarkiss, but when using a different delimiter you have to write

split m|/|;

# instead of
split |/|;
[download]

Or you can say: split "/"; (though " " is, of course, a special case, but for other simple one character (non-metacharacter) splits I like to use quotes :) If you're splitting on literal ".", you can't use "." but you could use "\\." but then I'd just assume use /\./ ...Got all that?

  #  Delete (something) from the input stream, separate the
  #  count from the file name.

  s/$in_prefix//;
  my ( $count, $file ) = split;
[download]

instead.

As a matter of coding style I use brackets when calling something like split; Since the delimiter is a slash, I'd also avoid the leaning toothpicks by using ' delimiters. So I would write

  #  Decompose directory/sub-directory/file into campaign,
  #  month and file.

  my ( $campaign, $month, $fname ) = split ( '/', $file );
[download]

Finally, I'd probably add the comment that you mentioned already at the top.

#  Build an array of hash references from the output from
#  the output wc -l command containing campaign, month and
#  count data.

my @files = map {
[download]

However, this looks like a situation where a hash of hashes might be more useful -- especially if you want to do subtotals across various months -- or campaigns.

"Excellent. Release the hounds." -- Monty Burns.

foreach my $line (@data) {
  my ($linecount, $filename) = split(/\s+/, $line);

  $filename =~ s/^$in_prefix//; # strip base directory from filename

  my ($campaign, $month, $fname) = split('/', $filename); # info is st
+ored in the directory nesting

  push @files, { campaign => $campaign,
                 month => $month,
                 file => $fname,
                 count => $count,
                 sort => "$campaign:$month:$file" };
}
[download]

But hey, you asked for opinions, right?

-jackdied

foreach (@in) {
  next unless m!(.*?)        # Count
              \s+
              ($in_prefix)?
              (([^/]+)/      # campaign
              ([^/]+)/       # month
              ([^/]+))!x;    # fname

  $_ = { campaign => $4,
         month => $5,
         file => $3,
         count => $1,
         sort => "$4:$5:$6" };
}
[download]