Why does Perl::Critic dislike using shift to populate subroutine variables?

Question

Lately, I've decided to start using Perl::Critic more often on my code. After programming in Perl for close to 7 years now, I've been settled in with most of the Perl best practices for a long while, but I know that there is always room for improvement. One thing that has been bugging me though is the fact that Perl::Critic doesn't like the way I unpack @_ for subroutines. As an example:

sub my_way_to_unpack {
    my $variable1 = shift @_;
    my $variable2 = shift @_;

    my $result = $variable1 + $variable2;
    return $result;
}

This is how I've always done it, and, as its been discussed on both PerlMonks and Stack Overflow, its not necessarily evil either.

Changing the code snippet above to...

sub perl_critics_way_to_unpack {
    my ($variable1, $variable2) = @_;

    my $result = $variable1 + $variable2;
    return $result;
}

...works too, but I find it harder to read. I've also read Damian Conway's book Perl Best Practices and I don't really understand how my preferred approach to unpacking falls under his suggestion to avoid using @_ directly, as Perl::Critic implies. I've always been under the impression that Conway was talking about nastiness such as:

sub not_unpacking {
    my $result = $_[0] + $_[1];
    return $result;
}

The above example is bad and hard to read, and I would never ever consider writing that in a piece of production code.

So in short, why does Perl::Critic consider my preferred way bad? Am I really committing a heinous crime unpacking by using shift?

Would this be something that people other than myself think should be brought up with the Perl::Critic maintainers?

Cluster

At a guess, Perl::Critic doesn't like it because it works using "magic," such as shift operating on @_ when no variable is specified. It would help to know which Perl::Critic rule wants to change this, though. – Sender

As an aside, if you use the list form of assignment, then Komodo Edit/IDE can tell you the arguments to a subroutine. – Doughman

Thanks Brad, I didn't realize that. I'm an old ViM user so usually I ignore what Komodo Edit is putting in front of me, but I've always wondered how that worked. – Cluster

Even under --brutal I don't get any complaints about unpacking arguments for your sample code. What version of Perl::Critic are you using? (I'm using 1.105) – Convey

Excellent catch Michael. In fact, I'm going to confirm when I get back to work tomorrow, but "shift" is ok with Perl::Critic 1.105 but "shift @_" isn't. I'll be submitting a bug report about this tomorrow. – Cluster

The list assignment is harder to read because you aren't used to it yet. Give it a chance. :) – Purr

Answer 1

The simple answer is that Perl::Critic is not following PBP here. The book explicitly states that the shift idiom is not only acceptable, but is actually preferred in some cases.

Monolithic

Answer 2

It's important to remember that a lot of the stuff in Perl Best Practices is just one guy's opinion on what looks the best or is the easiest to work with, and it doesn't matter if you do it another way. Damian says as much in the introductory text to the book. That's not to say it's all like that -- there are many things in there that are absolutely essential: using strict, for instance.

So as you write your code, you need to decide for yourself what your own best practices will be, and using PBP is as good a starting point as any. Then stay consistent with your own standards.

I try to follow most of the stuff in PBP, but Damian can have my subroutine-argument shifts and my unlesses when he pries them from my cold, dead fingertips.

As for Critic, you can choose which policies you want to enforce, and even create your own if they don't exist yet.

Kymberlykymograph

That's an excellent point and I agree with it, but my concern is whether or not Perl::Critic in its default state is correct in saying that using subroutine-argument shift's are bad. – Cluster

Answer 3

Running perlcritic with --verbose 11 explains the policies. It doesn't look like either of these explanations applies to you, though.

Always unpack @_ first at line 1, near 
'sub xxx{ my $aaa= shift; my ($bbb,$ccc) = @_;}'.
  Subroutines::RequireArgUnpacking (Severity: 4)
    Subroutines that use `@_' directly instead of unpacking the arguments to
    local variables first have two major problems. First, they are very hard
    to read. If you're going to refer to your variables by number instead of
    by name, you may as well be writing assembler code! Second, `@_'
    contains aliases to the original variables! If you modify the contents
    of a `@_' entry, then you are modifying the variable outside of your
    subroutine. For example:

       sub print_local_var_plus_one {
           my ($var) = @_;
           print ++$var;
       }
       sub print_var_plus_one {
           print ++$_[0];
       }

       my $x = 2;
       print_local_var_plus_one($x); # prints "3", $x is still 2
       print_var_plus_one($x);       # prints "3", $x is now 3 !
       print $x;                     # prints "3"

    This is spooky action-at-a-distance and is very hard to debug if it's
    not intentional and well-documented (like `chop' or `chomp').

    An exception is made for the usual delegation idiom
    `$object->SUPER::something( @_ )'. Only `SUPER::' and `NEXT::' are
    recognized (though this is configurable) and the argument list for the
    delegate must consist only of `( @_ )'.

Natividadnativism

Neither do, which is why I'm concerned about whether or not Perl::Critic is checking the "Always unpack @_" case validly... – Cluster

Answer 4

In some cases Perl::Critic cannot enforce PBP guidelines precisely, so it may enforce an approximation that attempts to match the spirit of Conway's guidelines. And it is entirely possible that we have misinterpreted or misapplied PBP. If you find something that doesn't smell right, please mail a bug report to [email protected] and we'll look into it right away.

Thanks,

-Jeff

Impaste

Thanks for posting here Jeff. I'm going to send an email tomorrow to the [email protected] email account about my concerns. I went back and re-read Section 9.3 in Conway's book and he even states, "Alternatively, you can use a series of separate shift calls as the subroutine's first 'paragraph'". – Cluster

Answer 5

I think you should generally avoid shift, if it is not really necessary!

Just ran into a code like this:

sub way {
  my $file = shift;
  if (!$file) {
    $file = 'newfile';
  }
  my $target = shift;
  my $options = shift;
}

If you start changing something in this code, there is a good chance you might accidantially change the order of the shifts or maybe skip one and everything goes southway. Furthermore it's hard to read - because you cannot be sure you really see all parameters for the sub, because some lines below might be another shift somewhere... And if you use some Regexes in between, they might replace the contents of $_ and weird stuff begins to happen...

A direct benefit of using the unpacking my (...) = @_ is you can just copy the (...) part and paste it where you call the method and have a nice signature :) you can even use the same variable-names beforehand and don't have to change a thing!

I think shift implies list operations where the length of the list is dynamic and you want to handle its elements one at a time or where you explicitly need a list without the first element. But if you just want to assign the whole list to x parameters, your code should say so with my (...) = @_; no one has to wonder.

Whist