Any automated code analysis will fail for lots of edge cases, sometimes in really amusing ways and other times in just plain dumb ways. Experience and context is almost always the key to figuring out if the analysis is telling you something important or not. The key phrase from your introduction is "which has detected a couple of issues" so that's a win.

For the rest, if it doesn't make sense to you you can most likely safely ignore it: you have the experience and know the context. For the unpacking @_ "issue", I'd write it as:

# Create Embedding object
sub new {
    my ($class, %attr)  = @_;
[download]

But that's how I'd have written it in the first instance and maybe that makes your eyes bleed, so what do I know? Either version works in terms of clarity in my view. I guess what the rule is really trying to clean up is progressive unpacking of the argument list through the function making the expected function signature hard to determine. That isn't an issue with your version so I'd say no problem exists.

G'day Bod,

Perl::Critic is based on the book Perl Best Practices by Damian Conway (aka TheDamian here at PerlMonks). He is very clear that his book offers a series of guidelines, not hard-and-fast rules. However, as you "wound up the severity", those guidelines and suggestions probably started to look more like rules and regulations.

In my opinion, the practices in the book are generally solid; I probably follow many of them without even thinking. There are some, however, that I don't agree with and, consequently, don't use.

If you look at the Perl::Critic distribution page, you'll see a huge number of Perl::Critic::Policy::* modules; these have explanations of why your (or anyone else's) code was criticised. Here's a quick rundown of the modules relating to the points you raised (I've omitted the Perl::Critic::Policy:: namespace from the link text in each case):

BuiltinFunctions::ProhibitStringyEval

In the main, I follow this policy. The most likely exceptions would be code like 'eval "require $module;";'. I'd question the usage you show. I don't know the context, but in isolation it looks odd:

$ perl -E 'our $V = q{0.1_1}; $V = eval $V; say $V'
0.11
[download]

It seems like extra work to achieve:

$ perl -E 'our $V = q{0.11}; say $V'
0.11
[download]

RegularExpressions::RequireDotMatchAnything

RegularExpressions::RequireExtendedFormatting

RegularExpressions::RequireLineBoundaryMatching

In my opinion, telling people to always use /xms promotes cargo-cult programming. I wonder how many people who use those modifiers actually understand them. None of them makes any sense with your split pattern.

Subroutines::RequireArgUnpacking

I almost always do this. I would have written your code as:

sub new {
    my ($class, %attr) = @_;
    ...
}
[download]

For a rare exception, see "Re: Optional Subroutine Arguments Like in print FILEHANDLE LIST":

sub queue {
    my $Q = ref $_[0] eq 'GLOB' ? shift : \*STDOUT;
    my @list = @_;
    ...
}
[download]

I'd say you have two basic options.

• Run your code through perlcritic. Pick and choose the criticisms you believe to be valid. Possibly modifying your code accordingly.
• Choose to make perlcritic a formal component of your code. Make all changes that it suggests. Avoid criticisms you strongly disagree with using "## no critic" — see "Perl::Critic: BENDING THE RULES".

I see you're using http://perlcritic.com/; I'm not familiar with that URL. See also: perlcritic (for on-the-fly, command line critique) and Test::Perl::Critic (to integrate with your test suite).

In addition to "Perl Best Practices" (2005 book) you may be interested in "Modern Perl Best Practices" (2014 video course) also by Damian Conway. I own a copy of the former and have read it a few times; I've only seen a trailer for the latter work.

Finally, I've voiced my opinions here; I'm well aware that others have differing opinions. Listen to all for a spread of advice then make your own mind up. :-)

I agree. You have to customize Perl::Critic to your own requirements. For example, i have upped the "maximum complexity score" and "maximum length of functions" of some parts of my code quite a bit. When you write complex state machines that (for example) handle HTTP connections. the main state machine winds up to be quite long. Yes, i could split that up into more functions, but in my case it would hinder readability and up the mental complexity while debugging.

Perl::Critic is certainly a helpful tool. But only after adapting it to the projects requirements and coding style. Plus, P::C regularly behaves stupid when new Perl features come out.

It's also important to remember that P::C only does static analysis. But only you can prevent forest fires the perl interpreter can properly parse Perl. And some P::C recommendations might even go against project/company policies.

PerlMonks XP is useless? Not anymore: XPD - Do more with your PerlMonks XP

Great reply++!

See also Perl::Critic::Community for a different set of perlcritic rules.

map{substr$_->[0],$_->[1]||0,1}[\*||{},3],[[]],[ref qr-1,-,-1],[{}],[sub{}^*ARGV,3]

Thanks for the comment and upvote.

Thanks also for the link. I checked through all of it and there's only three things I'm not compliant with:

• I never really liked FindBin; thankfully, I don't need it that often. I'm pleased to have found some alternatives.
• I agree with issues noted about JSON and JSON::XS: I'll be happy to trial JSON::MaybeXS.
• Typing 'while (<$fh>) {' is almost muscle-memory; I'll have to train muscles to add 'my $line ='. :-)

I've been getting questions about using some form of perlcritic at $work. Although arguing against full-blown Perl::Critic usage, I think Perl::Critic::Community is probably something I can get behind. So, that link was very useful and thanks again.

— Ken

I was surprised it didn't like this use of eval:

our $VERSION = '0.1_1';
$VERSION = eval $VERSION;
[download]

As far as I remember, this is standard boilerplate introduced to ensure PAUSE will index the distribution with correct module versions. At one time, at least, PAUSE would scan the code for a version number in a fairly simplistic manner (specifically not running any code, for security concerns), so this ensured that it will see the string value as the version to index the code under while any actual code will see the eval'd value. I do not know if PAUSE still does that: I would expect by now that there would be ways to provide the version information more robustly by means of metadata.

For the rest of it: perlcritic by default is an implementation of PBP, and PBP was written with two goals in mind - for people who don't care, to provide a ready-made set of coding standards to use within an organization; for people who do care, to provide a starting point to think about how to assemble a set of coding standards.

Since Damian expected that organizations with the least Perl skills would be most likely to simply adopt PBP wholesale, the ready-made set is optimized for that group. The idea that every regexp should be written with //xms flags is an example of this: you can for example teach your developers that /./ means "match any character" without having to deal in complexities, and filling their head with edge cases. As kcott says, this is cargo-cult programming - but intentionally so.

The prescriptions in PBP were never intended to be a straitjacket for people who do care, and are prepared to put any time into thinking about how they want their code to look. I recommend reading it (or re-reading it) with that in mind before attempting to use them - even in second-hand form, as via perlcritic.

As far as I remember, this is standard boilerplate...

That is certainly my understanding...and I extrapolated to assume that it was best, or at least good practice to do it that way!

As others have said, perlcritic is opinionated (and intentionally so). Use the parts you agree with and ignore the parts you don't.

I struggle to see what benefit adding the /x flag would be to such a simple regular expression

There isn't any, so ignore it. FWIW, I literally never use /x as it causes more problems than it solves for me. YMMV.

How critical is it that code destined for CPAN passes critic, if at all?

Not at all. perlcritic is a tool for the author only to ensure that they have followed their own choice of policies (or more usually policies enforced upon them by someone else). Upload dists which pass, upload dists which don't - it makes no odds.

All that said, I am curious as to why you use the string eval in that context at all. Can you explain what it is doing that a simple scalar assignment won't achieve?

FWIW, here's a simplified version of my ~/.perlcriticrc:

severity = 3
verbose = 9
[RegularExpressions::RequireExtendedFormatting]
severity = 2
[Variables::ProhibitReusedNames]
severity = 5
[download]

For modules I like to increase the severity of ErrorHandling::RequireCarping to 4, and sometimes I need to reduce the severity of Modules::ProhibitExcessMainComplexity down to 2.

I've found this gives a decent balance between reminding me of best practices vs. annoying nitpickyness. I do make use of "## no critic (...)" as necessary.

I was surprised it didn't like this use of eval:

our $VERSION = '0.1_1';
$VERSION = eval $VERSION;
[download]

I wasn't. During many years of code reviewing inexperienced Perl coder's efforts over the years, I've noticed that block eval tends to be under-used, stringy eval over-used.

Bod, I would certainly grill you during our code review meeting to "please explain" why on earth you're resorting to the dreaded stringy eval for such a simple chore, and to please describe the alternatives you'd considered and rejected before doing so. :)

For more background on this topic see: string eval vs block eval

I would certainly grill you during our code review meeting to "please explain" why on earth you're resorting to the dreaded stringy eval for such a simple chore

The answer would be because I understand that is what I am supposed to do because Perl uses underscores in the version number to indicate a development release. This release is currently a development release 0.1_1 with an underscore.

I strongly suspect that it was you eyepopslikeamosquito who directed me towards this practice as you were the one who helpfully provided the information I needed to publish my first module Business::Stripe::WebCheckout. I used almost exclusively the links you provided to do that which included adding $VERSION.

I cannot find the original code but here is an article that advises using this technique...

The answer would be because I understand that is what I am supposed to do because Perl uses underscores in the version number to indicate a development release. This release is currently a development release 0.1_1 with an underscore.

Good answer!

I strongly suspect that it was you eyepopslikeamosquito who directed me towards this practice as you were the one who helpfully provided the information I needed to publish my first module

Yes Bod, I believe you are correct.

The version numbers should be boring link you mentioned by David Golden is indeed listed at Software Versioning links ... which is listed at Writing Solid CPAN Modules.

Update: Re: What do I use to release a module to CPAN for the first time? by davido similarly endorses $VERSION = eval $VERSION ... as do the official Perl docs perlmodstyle (in the Version numbering sub-section). Thanks Bod.

I would conclude our code review meeting by reaching for a towel to wipe the egg off my face. :)

I was sure I had seen it recommended somewhere (though I never chose to use it in my modules)

Exactly! It came from a recommendation...I cannot find it right now but I recall copying it from advice about module versioning.

Essentially, as I understand it, one might want the CPAN to see the underscore to tell that it's an alpha version...

And my version is 0.1_1!