Friday, 18 July 2008

Perl::Critic - new release

So a new release of Perl::Critic was released, and all I want to say is what a faff.

Some key new features:

1) You must check the return value of all eval statement - don't rely on $EVAL_ERROR/$@

Now, this is a good thing(tm) but it does have some pitfalls, such as where you might be evalling a transactional commit.

eval {
$transaction_state and $dbh->commit();
} or do {
...some 'croak' code
};

The problem here is that the eval isn't a croak if $transaction_state is 0 (as it could be inside a much larger transaction), but the return code would be 0, therefore firing off your 'croak' code.

So, to get round this, you need to return a true value after the statement;

eval {
$transaction_state and $dbh->commit();
1;
} or do {
...some 'croak' code
};

A bit off a faff, but it is better code for it.

2) Declaration of all numbers other than 0,1 and 2

So, as it could be difficult to understand what a number represents when reading the code, you now need to use Readonly to declare any numbers with a named variable at the top of your code.

So now instead of

$percentage = $x * 100/$y;

You need

use Readonly;
Readonly our $PERCENTAGE_MAKER => 100;
...
$percentage = $x * $PERCENTAGE_MAKER/$y;
...

This 'might' make sense to odd numbers floating around, but it also applies to indices on arrays. So if you want the 5th element on an array intstead of requesting

$wanted = $array[4];

You now need

use Readonly;
Readonly our $FIFTH_ARRAY_ELEMENT => 4;
...
$wanted = $array[$FIFTH_ARRAY_ELEMENT];
...

Now, I admit it is often bad to pick out many individual elements from an array by specific number, but it will seriously clutter code where this may be necessary. I admit, for a couple of lines in my code, I have now used ##no critic at the end.

I also wonder why specifically 0,1 and 2 get let off. If the problem is that you don't know if 60 means

i) minutes in an hour
ii) seconds in a minute
iii) degrees in the angle of an equilateral triangle

then 0 could mean off, false, nothing, 0
1 could mean on, true, positive, 1
2 could mean wheels on a bicycle, eyes on a face, hands, 2

Just some examples where I imagine it is because they are the most heavily used for certain features, it would almost impossible to change them (especially 1 as the return value of a perl module)

3) Declaring a variable, which you never (appear to) use

This is annoying if, like me, you use Class::Std to create inside out objects, as you need to declare a hash for your attributes, but this has is never referred to again in the code.

Now, whilst I understand that if you don't use a variable, don't declare it, in this case you are using it, just via the Class::Std methods of the accessors created. However, much like the declaration of numbers, it isn't looking at the context in which you use the declared variable. Again, in this case I have had to wrap these with a ## no critic {} ## use critic in order to not have it fail.

--

So as I said at the beginning, all I want to say is what a faff and the reason is as follows:

We are using Perl::Critic extensively here in New Pipeline Development, and it does force our hand to a Good Coding Practice, but I can't help but wonder here if a couple of these new standards are just a little too overzealous, and causing some things to be overcritised (i.e. a faff).

As with many things, it will take time to get used to programming in advance of critic'ing the code, but I think some of these new features need a little tweaking.

2 comments:

thaljef said...

First, I have to thank you for introducing me to the term "faff". I can't wait to use that at work next week!

I know it can be a pain to keep your code compliant as new Policies emerge. You might consider disabling the new Policies in your .perlcritirc file, until you've had time to evaluate them and decide if it is worth the effort to change up your code.

Also, you can configure ProhibitMagicNumbers to allow various types of numbers or ranges of values. So that may help you avoid some of those "## no critic" markers. See this link for details:

http://search.cpan.org/~thaljef/Perl-Critic-1.092/lib/Perl/Critic/Policy/ValuesAndExpressions/ProhibitMagicNumbers.pm#CONFIGURATION

Happy coding!

-Jeff

Andy Brown - SetitesUK said...

Thanks for your comment. I hope that you managed to use "faff" :)

We do tend to jump straight in with the new policies as with a large coding force and different groups being responsible for different aspects, it is useful to have something that forces us to 'sing from the same songsheet'. This ha, however, enforced us to declare a lot of stuff and do some quite large updates to code. Of course, moving forward, most of what we now need to do is nothing to much. I fall over with not putting use Carp and use English more than predeclaring with use Readonly. Luckily, however, noone is stopping us releasing with ## no critic if put in appropriately.

Cheers

Andy