Skip Menu |
 

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the Perl-Critic CPAN distribution.

Report information
The Basics
Id: 36129
Status: open
Priority: 0/
Queue: Perl-Critic

People
Owner: Nobody in particular
Requestors: EDAVIS [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Normal
Broken in: 1.082
Fixed in: (no value)



Subject: BuiltinFunctions::ProhibitReverseSortBlock too strict
Download (untitled) / with headers
text/plain 1.4k
BuiltinFunctions::ProhibitReverseSortBlock is based on PBP 152, which particularly deprecates this code: sort { $b cmp $a } @array; Damian rightly says you should 'reverse sort' instead. Clearly the policy should check for this. Arguably, the policy can go a step further and generalize Damian's advice to also forbid sort { $b <=> $a } @array; and require you to write instead reverse sort { $a <=> $b } @array; This is more controversial because a quick benchmark on perl-5.10.0 shows the reverse sort is about 40 times slower. But still, it is perhaps more readable. Damian doesn't explicitly rule one way or the other. But there are some cases where you really do need to compare $b and $a the 'wrong way round'. For example, my %h; # Sort by lowest score first, or failing that, reverse alphabetical. my @x = sort { $h{$a} <=> $h{$b} || $b cmp $a } keys %h; # Hmm, perlcritic complains about that, perhaps I should say this: my @y = reverse sort { $h{$b} <=> $h{$a} || $a cmp $b } keys %h; But in both cases BuiltinFunctions::ProhibitReverseSortBlock complains! Short of obfuscating the code somehow so it doesn't notice what is going on in the sort block, I don't see how to satisfy it. The PBP book doesn't issue a general prohibition on mentioning the variable $b in a sort block before a mention of $a. It talks about the particular common error of sort { $b cmp $a }. So I think this policy needs to be weakened.
Subject: Re: [rt.cpan.org #36129] BuiltinFunctions::ProhibitReverseSortBlock too strict
Date: Fri, 23 May 2008 08:13:53 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Chris Dolan <chris [...] chrisdolan.net>
On May 23, 2008, at 4:20 AM, EDAVIS via RT wrote: Show quoted text
> ... > > But there are some cases where you really do need to compare $b and $a > the 'wrong way round'. For example, > > my %h; > # Sort by lowest score first, or failing that, reverse alphabetical. > my @x = sort { $h{$a} <=> $h{$b} || $b cmp $a } keys %h; > > # Hmm, perlcritic complains about that, perhaps I should say this: > my @y = reverse sort { $h{$b} <=> $h{$a} || $a cmp $b } keys %h; > > But in both cases BuiltinFunctions::ProhibitReverseSortBlock > complains! > Short of obfuscating the code somehow so it doesn't notice what is > going on in the sort block, I don't see how to satisfy it. >
Then just use "##no critic (ProhibitReverseSortBlock)" on that line or in a block around it. In my opinion, the ProhibitReverseSortBlock is too complex already, so adding more complexity is not realistic. Like all bug checkers, P::C is imperfect and will always require human interpretation. Blindly following the rules makes your code worse, not better. Chris
Download (untitled) / with headers
text/plain 339b
I agree that making it more complex is not a good idea. I think it would be simpler for the rule to prohibit just sort { $b cmp $a } and not complain about anything else. To me, that seems to match the advice in the PBP book, which doesn't talk about more general sort subroutines that might happen to have $b before $a somewhere.
Subject: Re: [rt.cpan.org #36129] BuiltinFunctions::ProhibitReverseSortBlock too strict
Date: Fri, 23 May 2008 22:36:15 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Chris Dolan <chris [...] chrisdolan.net>
Download (untitled) / with headers
text/plain 862b
On May 23, 2008, at 1:05 PM, EDAVIS via RT wrote: Show quoted text
> > Queue: Perl-Critic > Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=36129 > > > I agree that making it more complex is not a good idea. I think it > would be simpler for the rule to prohibit just > > sort { $b cmp $a } > > and not complain about anything else. To me, that seems to match the > advice in the PBP book, which doesn't talk about more general sort > subroutines that might happen to have $b before $a somewhere.
Interesting. I see the merit in that argument. If the above applies to sort {$b <=> $a} too, then I can see splitting the policy in too: a higher priority one that just does the above and a lower priority one that attacks the complex booleans too. Then people could ignore the stricter one and just use the more targeted one if they prefer. Chris
From: EDAVIS [...] cpan.org
Download (untitled) / with headers
text/plain 718b
On Fri May 23 23:37:47 2008, chris@chrisdolan.net wrote: Show quoted text
>If the above applies >to sort {$b <=> $a} too, then I can see splitting the policy in too: >a higher priority one that just does the above and a lower priority >one that attacks the complex booleans too. Then people could ignore >the stricter one and just use the more targeted one if they prefer.
That would be a good answer. But I'm not convinced that sort { $b <=> $a } is always bad; as I mentioned, a quick benchmark shows it to be forty times faster than reverse sort { $a <=> $b } on a large list of integers. Damian doesn't particularly mention this idiom, just alphabetical sorting. I would move { $b <=> $a } to the stricter policy too.
Download (untitled) / with headers
text/plain 176b
Show quoted text
>>I think it >>would be simpler for the rule to prohibit just >> >> sort { $b cmp $a } >> >>and not complain about anything else.
Shall I make a patch implementing this?
Subject: Re: [rt.cpan.org #36129] BuiltinFunctions::ProhibitReverseSortBlock too strict
Date: Wed, 19 Aug 2009 22:55:25 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
Download (untitled) / with headers
text/plain 351b
EDAVIS via RT wrote: Show quoted text
>>> I think it >>> would be simpler for the rule to prohibit just >>> >>> sort { $b cmp $a } >>> >>> and not complain about anything else.
> > Shall I make a patch implementing this?
I prefer the current default behavior. If you want to add an option to the policy that changes the behavior to this, I'll go along with it.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.