Some simple Perl and Regex

Question

Reads from a flat file of three columns, delimited by at least one tab character. Filters the three columns based on input; returns list of corresponding emails. Arguments passed like key=value or key= for blank.

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

sub get_list {
   my $cols   = {'list'=>0, 'email'=>1, 'sub'=>2};  
   my $list   = '[a-z0-9_\- ]+';
   my $email  = '[a-z0-9@\._\-]+';
   my $subls  = '[a-z0-9\. ]+'; 
   my $return = $cols->{'email'}; 
   foreach (@ARGV) {
      my ($key, $value) = split '=';
      $value = q{} if not defined $value;  
      if    ($key eq 'list')   { $list  = $value; }
      elsif ($key eq 'email')  { $email = $value; }
      elsif ($key eq 'sub')    { $subls = $value; }
      elsif ($key eq 'return') { 
         $return = $cols->{$value} 
                   if defined $cols->{$value}; 
      }
      else  { die "invalid key: $key\n"; }
   }   
   my $file = 'input.txt';  
   open FH, '<', $file 
         or die "Could not open '$file' $!\n";   
   my %emails = map { (split /\s*\t\s*/)[$return] => 'tacos' }
                grep m/^${list}\s*\t\s*${email}(\s*\t\s*${subls})?\s*$/gi, 
                <FH>; 
   close FH; 
   return (join ', ', keys %emails);   
} 

print get_list(@ARGV);
print "\n"; 

# test input... three columns delimited by at least one tab
# third column may be blank.  nothing should contain spaces
# in real life, but i'm allowing it... 

# archive   [email protected] success
# archive       [email protected]    fail
# archive   [email protected]    success
# archive   [email protected]    
# archive   [email protected] fail    
# taco  [email protected] success
# taco  [email protected]    fail
# taco  [email protected]    success
# taco meat     [email protected]    
# tacor [email protected] fail    

I realize some people may not be down with the $thing = $value if defined $value; arrangement but I went ahead and used it since the logic wasn't complex. I prefer it over using space for brackets on one line conditionals. Also, please note, I'm not trying to validate email addresses with regex and I these are truly the only characters the strings will contain.

---

for the future person... this is what I ended up with:

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

sub get_list {
   my $file = 'input.txt';
   my %emails; 

   my $cols  = {'list'=>0, 'email'=>1, 'sub'=>2, 'subls'=>2};
   my $index = $cols->{'email'};

   my $dlmtr = qr{\s*\t\s*};
   my $list  = qr{[a-z0-9_\- ]+};
   my $email = qr{[a-z0-9@\._\-]+};
   my $subls = qr{[a-z0-9\. ]+};

   foreach (@ARGV) {
      my ($key, $value) = split qr{=};
      $value //= qr{};
      if    ($key eq 'list')   { $list  = $value; }
      elsif ($key eq 'email')  { $email = $value; }
      elsif ($key eq 'sub')    { $subls = $value; }
      elsif ($key eq 'return' && defined $cols->{$value}) {
         $index = $cols->{$value};
      }
      else  { die "invalid key: $key\n"; }
   }

   open FH, '<', $file or die "Could not open '$file' $!\n"; 
   while (<FH>) {
      chomp;

      # i couldn't get the syntax in the accept answer to parse for me
      # so i changed it to this... also, %emails is better as a hash
      # b/c it prevents emails added multiple times... and for my thing,
      # the first two columns are required but the third may be blank.

      if (m/^${list}${dlmtr}${email}(${dlmtr}(${subls}))?\s*$/gi) { 
         my $value   = (split $dlmtr)[$index]; 
            $value //= q{}; 
         $emails{ $value } = 'tacos';  
      }
   }
   close FH;

   return join ' ', keys %emails;
}

Answer 1

my $cols   = {'list'=>0, 'email'=>1, 'sub'=>2};

I'd call this

my %index_of = { 'list' => 0, 'email' => 1, 'sub' => 2 };

and later, change $return to

my $index = $index_of{'email'};

The following is used oddly:

print get_list(@ARGV);
foreach (@ARGV) {

You are calling the function with parameters @ARGV but you never use the parameters in the function. Instead, you use @ARGV again. Better would be to use the function parameters:

my @arguments = @_;
foreach my $argument (@arguments) {
   my ($key, $value) = split {=}, $argument;

I also switched to using a named element instead of letting it default to $_.

Alternately, you could pass nothing into the function and use @ARGV in the foreach as you did. That would work but isn't a best practice.

You may not have realized but the '=' was a regular expression with single quotes as the delimiters. I prefer to use curly brackets for regular expressions. If you wanted to be really plain about it, you could have said qr{=}.

# default patterns that can be overridden by command line arguments
my $list   = qr{[a-z0-9_\- ]+};
my $email  = qr{[a-z0-9@\._\-]+};
my $subls  = qr{[a-z0-9\. ]+};
  $value = qr{} if not defined $value; 

Using qr{} here makes it more obvious that these are regular expression patterns.

open my $fh, '<', $file 

I'd prefer a variable here rather than a globally scoped bareword.

my $SEPARATOR = qr{\s*\t\s*}
my %emails = map { (split $SEPARATOR)[$return] => 'tacos' }
             grep m{^${list}${SEPARATOR}${email}(${SEPARATOR}${subls})?\s*$}gi, 
             <$fh>;

By defining $SEPARATOR, we make the regular expressions clearer and reduce the amount of repeated code.

my @entries = ();
while ( my $line = <$fh> ) {
   @columns = m{\A(${list})${SEPARATOR}(${email})(${SEPARATOR}(${subls}))?\s*\z}gi;
   if ( scalar @columns >= 2 ) {
      push @entries, $columns[$index];
   }
}
close $fh;

return (join ', ', @entries); 

This is clearer about what it is doing than the map of the grep results. Also, it doesn't parse each line twice. And of course it doesn't define a hash just to get the keys.

In general, I would make more use of paragraphing. This is extra new lines to separate blocks of code. This allows you to easily see what pieces of code go together. For example, note how I separate the return from the file handling code.

You have comments explaining what the file format should be, but you don't explain what the command line arguments should be. A user would have to read the code to figure it out.

Your function does two distinct things. First, it reads the command line arguments. Second, it opens a file and processes it based on the command line arguments. It would be better if you could separate that into two functions.

Answer 2

A few tiny things on top @Brythan's excellent review.

---

my $subls  = '[a-z0-9\. ]+';

You don't need to escape \. within [ ... ] inside a regex. So this is the same and simpler:

my $subls  = '[a-z0-9. ]+';

---

Instead of this:

  $value = q{} if not defined $value;

This is shorter and easier to type:

  $value = '' if not defined $value;  

---

  else  { die "invalid key: $key\n"; }

If you omit the newline \n, then die will append the line number and a newline to the message, which is useful for debugging. (Maybe you already knew this and put the newline there on purpose to hide the line number, but maybe not, so I thought this is worth mentioning.)

---

The parentheses are unnecessary here:

   return (join ', ', keys %emails);

Answer 3

return join ", ", map {
  s/\s*$//;
  my @r = split /\s*\t\s*/;
  my $want_it = ($r[0] eq $list and $r[1] eq $email);

  # returning empty list '()' acts like grep
  $want_it ? $r[$return] : ();
}                
<FH>;

instead of

 my %emails = map { (split /\s*\t\s*/)[$return] => 'tacos' }
              grep m/^${list}\s*\t\s*${email}(\s*\t\s*${subls})?\s*$/gi, 
              <FH>; 
 close FH; 
 return (join ', ', keys %emails); 

Benefits are more clear code, no need for hash, and elimination of redundant operations.