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
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
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.
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
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
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;