Return to Answer

• 1

• 145.5k
• 22
• 190
• 479

• 145.5k
• 22
• 190
• 479

As more than one user has already noted, you are reinventing-the-wheel. Nevertheless, the code is worth reviewing.

Using the & sigil to call a subroutine has gone out of style since Perl 5 was released in 1994.

Nesting a for-loop inside a while-loop is a weird way to express the flow of control, in my opinion. You want to iterate through @ARGV at most once, so it should look like a single loop. The for-loop isn't really a loop at all; it's just a way for last to goto the point after push(@args, $_). (The use of for would be acceptable in conjunction with the when keyword, as it would then be recognizable as a Perl switch statement.) I think that the following way would be a clearer way to write the loop:

while ($_ = shift @ARGV) {
 ($_ eq '-h' || $_ eq '--help') && do { usage(); };
 ($_ eq '-D' || $_ eq '--decode') && do { $encode = ! $encode; next; };
 ($_ eq '--') && do { push(@args, @ARGV); undef @ARGV; next; };
 ($_ =~ /^-./) && do { print STDERR "Unknown option: $_\n"; usage(); };
 push(@args, $_);
}

If a solitary - is passed on the command line, it is treated as a valid argument to be appended literally to @args. I'm not sure if that is intentional.

Explicitly asking for usage help differs from displaying help in the case of a usage error. In the former case, the exit status should be 0, and the display should go to STDOUT. In the latter case, the exit status should be non-zero, and the display should go to STDERR. In addition, I don't recommend burying the exit() call inside usage(), because it makes your option-parsing loop harder to follow.

sub usage {
 my ($status) = @_;
 select STDERR if $status;
 print ...;
 return $status;
}
exit usage(1) unless @ARGV;
while ($_ = shift @ARGV) {
 ($_ eq '-h' || $_ eq '--help') && do { exit usage(0); };
 ($_ eq '-D' || $_ eq '--decode') && do { $encode = ! $encode; next; };
 ($_ eq '--') && do { push(@args, @ARGV); undef @ARGV; next; };
 ($_ =~ /^-./) && do { print STDERR "Unknown option: $_\n"; exit usage(1); };
 push(@args, $_);
}

The use of $encode is problematic in three ways. First, you should use variables that reflect the UI. Also, prefer to use variables that default to 0 or a false value. Finally, toggling $encode whenever a --decode flag is encountered is inappropriate, because it is surprising that --decode --decode actually causes it to encode instead of decode. (Imagine what would happen if one defined a shell alias rm as rm -i, such that running rm -i expands to rm -i -i, only to find that file deletion is no longer interactive!)

In my opinion, the code formatting hinders readability. Aligning the do blocks would help. However, you might as well be less stingy and place statements on separate lines.

my $decode;
while ($_ = shift @ARGV) {
 if ($_ eq '-h' || $_ eq '--help') {
 exit usage(0);
 } elsif ($_ eq '-D' || $_ eq '--decode') {
 $decode = 1;
 next;
 } elsif ($_ eq '--') {
 push(@args, @ARGV);
 undef @ARGV;
 next;
 } elsif (/^-./) {
 print STDERR "Unknown option: $_\n";
 exit usage(1);
 } else {
 push(@args, $_);
 }
}

The usage of next and elsif are redundant. I think that the redundancy helps make the code easier to follow, but it's a matter of taste.