Recently I encountered a Perl script that had some issues. e.g. Lack of use strict.

Let me show a quick refactoring of it:

Part of the original code

examples/code_snippet0.pl

my $copyrightsfile = shift @ARGV;

open COPYR,"<$copyrightsfile" or die "Can't open copyrights $copyrightsfile";
while(<COPYR>){
    chomp;
    next if /^#/;
    @COPYRIGHT=split /,/;
    next unless scalar(@COPYRIGHT)==2;
    push @COPYRIGHTS,{
        firstname=>$COPYRIGHT[0],
        lastname=>$COPYRIGHT[1],
    };
}
close COPYR;

use Data::Dumper qw(Dumper);
print Dumper \@COPYRIGHTS;

There are some interesting things here:

  • missing use strict
  • But declaring the global variable $copyrightsfile using my
  • Not declaring @COPYRIGHTS (in plural) at all even though it is a global variable.
  • Not declaring @COPYRIGHT (in singular) either, even though it should be locally scoped. And the confusion of having two arrays with very similar names.
  • Using the implicit $_ might be considered "perlish" by some, but I prefer to have a named variable in its place.
  • I addedd the Data::Dumper code to make it easier to see the results.

Sample imput file

examples/code_snippet.txt

Jane,Doe
Joe,
Mary

Result:

$VAR1 = [
          {
            'firstname' => 'Jane',
            'lastname' => 'Doe'
          }
        ];

Add use strict, Use lexical file-handle

Lexical file-handles and 3-part open.

That forces us to declare our varibales with my.

examples/code_snippet1.pl

use strict;
use warnings;

my $copyrightsfile = shift @ARGV;
my @COPYRIGHTS;

open my $COPYR, '<', $copyrightsfile or die "Can't open copyrights $copyrightsfile";
while(<$COPYR>){
    chomp;
    next if /^#/;
    my @COPYRIGHT=split /,/;
    next unless scalar(@COPYRIGHT)==2;
    push @COPYRIGHTS,{
        firstname=>$COPYRIGHT[0],
        lastname=>$COPYRIGHT[1],
    };
}
close $COPYR;

use Data::Dumper qw(Dumper);
print Dumper \@COPYRIGHTS;

Replace @COPYRIGHT, by $firstname, $lastname

examples/code_snippet2.pl

use strict;
use warnings;

my $copyrightsfile = shift @ARGV;
my @COPYRIGHTS;

open my $COPYR, '<', $copyrightsfile or die "Can't open copyrights $copyrightsfile";
while(<$COPYR>){
    chomp;
    next if /^#/;
    my ($firstname, $lastname) = split /,/;
    next if not defined $lastname or $lastname eq '';
    push @COPYRIGHTS,{
        firstname => $firstname,
        lastname => $lastname,
    };
}
close $COPYR;

use Data::Dumper qw(Dumper);
print Dumper \@COPYRIGHTS;

Use explicit $line instead of $_ in the while loop

examples/code_snippet3.pl

use strict;
use warnings;

my $copyrightsfile = shift @ARGV;
my @COPYRIGHTS;

open my $COPYR, '<', $copyrightsfile or die "Can't open copyrights $copyrightsfile";
while(my $line = <$COPYR>){
    chomp $line;
    next if $line =~ /^#/;
    my ($firstname, $lastname) = split /,/, $line;
    next if not defined $lastname or $lastname eq '';
    push @COPYRIGHTS,{
        firstname => $firstname,
        lastname => $lastname,
    };
}
close $COPYR;

use Data::Dumper qw(Dumper);
print Dumper \@COPYRIGHTS;