listify() and Perl::Critic

Before I start, I just want to make clear to anyone unfamiliar with them, both perltidy and Perl::Critic are pretty comprehensively customizable. But, I am interested in the default settings. The reason is this: on arbitrary choices like indenting by 4 spaces or indenting by 3, if everyone stuck to the defaults, code would be much more uniform across code bases. I guess as I get older, I see much more of the value in conformity. :)

In my last blog entry, I made manual changes to make my code more clear. So, I ran the code through Perl::Critic to gain some additional clarity. This was a full example program and not just the subroutine from the previous blog entry. The final program is below:

#!/usr/bin/perl

use strict;
use warnings;
use version; our $VERSION = qv('0.1');
use Data::Dumper;

my @list = (
'Alabama',        'Alaska',       'Arizona',      'Arkansas',
'California',     'Colorado',     'Connecticut',  'Delaware',
'Florida',        'Georgia',      'Hawaii',       'Idaho',
'Illinois',       'Indiana',      'Iowa',         'Kansas',
'Kentucky',       'Louisiana',    'Maine',        'Maryland',
'Massachusetts',  'Michigan',     'Minnesota',    'Mississippi',
'Missouri',       'Montana',      'Nebraska',     'Nevada',
'New Hampshire',  'New Jersey',   'New Mexico',   'New York',
'North Carolina', 'North Dakota', 'Ohio',         'Oklahoma',
'Oregon',         'Pennsylvania', 'Rhode Island', 'South Carolina',
'South Dakota',   'Tennessee',    'Texas',        'Utah',
'Vermont',        'Virginia',     'Washington',   'West Virginia',
'Wisconsin',      'Wyoming',
);

listify( \@list, 11 );

print Data::Dumper::Dumper( \@list );

sub listify {
   my ( $aref, $cc ) = @_;
   if ( ref $aref eq 'ARRAY' && $cc > 0 ) {
       my $j;
       for ( my $i = 0 ; $i <= $#$aref ; $i += $cc ) {
           push @$j, [ @$aref[ $i .. $i + $cc - 1 ] ];
       }
       $#{ $j->[ $#{$j} ] } = $#$aref % $cc;
       @$aref = @$j;
       return 1;
   }
   return;
}

Perl::Critic comes with a command-line utility, perlcritic. The default minimum severity level for deviations from the standard is 5, the most severe. Since I was looking for enlightenment, I wanted to see everything it had. I ran perlcritic -severity 1 example.pl. This is an example of its output:

Code is not tidy at line 1, column 1. See page 33 of PBP.
(Severity: 1)

RCS keywords $Id$ not found at line 1, column 1. See page 441 of
PBP. (Severity: 2)

RCS keywords $Revision$, $HeadURL$, $Date$ not found at line 1,
column 1. See page 441 of PBP. (Severity: 2)

RCS keywords $Revision$, $Source$, $Date$ not found at line 1,
column 1. See page 441 of PBP. (Severity: 2)

No "VERSION" variable found at line 1, column 1. See page 404 of
PBP. (Severity: 2)

Code before warnings are enabled at line 6, column 1. See page
431 of PBP.  (Severity: 4)

Double-sigil dereference at line 35, column 25. See page 228 of
PBP. (Severity: 2)

C-style "for" loop used at line 41, column 9. See page 100 of
PBP. (Severity: 2)

I ignored perlcritic's demands for RCS stuff like $Id$ which I don't need in a throw-away program. Other than that, I addressed all of its complaints and made the following changes:

• perlcritic complains that code is not "tidy". That's pretty damn cool (though not unexpected as it is in PBP). I ran perltidy example.pl -o example.out, compared the differences, then copied example.out over example.pl . The prominent changes were:
  • Lines longer than 80 characters were broken up into multiple lines
  • Other whitespace changes were made. It did actually move a line up: where I had ended my if ( with a close-paren semicolon on its own line, it brought that up to the previous line. Not sure I agree with that choice, but I kept it in the name of Zen.
• It wanted me to use warnings. I have mixed feelings about warnings because I hate things like having to no warnings 'once';, but I can't deny it has helped me before. Typically, I only compile with warnings (perl -Wc example.pl) as that usually gives me all I need.
• It wanted me to use version. Even though it's a throw-away, I complied.
• It did not like my c-style for loop. Mixed feelings again here - this particular for loop is very simple in structure. I re-wrote it as an exercise, though.
• Final element in array should be -1, not $#array. Doh. $#array is just something I've been in the habit of using. But when you are actually referencing the final element, -1 looks a whole lot nicer.
• In response to many double-sigil complaints, I created variable $aref_elements to reduce clutter. This was something that I should have discovered during refactoring anyway.
• Perl::Critic prefers $#{$aref} to $#$aref. That is interesting. My initial feeling is that it looks much busier with the curlies, but I can see Damian's point. Once you are comfortable looking at them, consistent use erases a lot of ambiguity.

Clever Code

One criticism of opponents of Perl is that it is a "write-only" language - meaning that once the code is written, it is extremely difficult to maintain because it is difficult to understand upon re-examination. As with many criticisms, this should be aimed at those undisciplined developers who are writing the code, and not their tool of choice.

Having said that, I think it is also fair to say that Perl makes it very easy to write difficult-to-decipher code. This is the double-edged sword which is the shorthand Perl gives us to be very expressive in a small amount of space. A negative application of this is obfuscated Perl (where the author intentionally makes his code difficult to read), while a more positive application is the craft of creating Perl "one-liners" (trying to include a great deal of functionality in a single line of code). A one-liner can be a powerful weapon in the arsenal of a system administrator.

As a side note, I don't think obfuscated code is inherently evil. I think the Obfuscated Perl Contests have some pretty nifty entries, and when I spent time writing a couple obfuscated perls of my own, I learned a great deal about the Perl parser, so it was a good learning experience as well as fun.

Back to the point, listify() is a subroutine I wrote a while back that takes a reference to an array as its first argument, and a number n as its second argument, and transforms the original array into an array of arrays each having no more than n elements.

Implementation of listify() is here:

sub listify {
   my ($aref,$cc) = @_;
   if( ref $aref eq 'ARRAY' && $cc > 0 ) {
       my $j;
       for(my $i=0; $i<=$#$aref; $i+=$cc) {
           push @$j, [@$aref[$i..$i+$cc-1]];
       }
       $#{$j->[$#{$j}]}=$#$aref%$cc;
       @$aref = @$j;
       return 1;
   }
   return;
}

The point of the routine was to take very long lists of e-mail addresses that we use to notify customers of upcoming changes, and break them apart into smaller recipient lists of reasonable size that can be handled in a single outgoing e-mail.

Given that:

@my_array = ( 'one', 'two', 'three', 'four', 'five', 'six', 'seven',
'eight', 'nine', 'ten' );

listify(\@my_array, 4) transforms @my_array into this:

@my_array = (
   [ 'one',  'two', 'three', 'four'  ],
   [ 'five', 'six', 'seven', 'eight' ],
   [ 'nine', 'ten' ],
);

So, the line that I'm citing as my "clever" line is this one:

$#{$j->[$#{$j}]}=$#$aref%$cc;

Full disclosure: I was pretty impressed with myself for this one at the time, I guess because I was jazzed to have written something so cryptic. And since it was a tiny part of a not-often-used routine, I wasn't worried about maintainability.

The purpose of this line of code is to truncate the final array so that, using the above example, we don't end up with this instead:

@my_array = (
   [ 'one',  'two', 'three', 'four'  ],
   [ 'five', 'six', 'seven', 'eight' ],
   [ 'nine', 'ten', undef,   undef   ],
);

So, what can be done to make this line more readable?

$#{$j->[$#{$j}]}=$#$aref%$cc;

One thing it's missing is whitespace to separate the different parts:

$#{ $j->[ $#{$j} ] } = $#$aref % $cc;

$#array yields the final index of @array. So, $#$array is the notation we'd use if $array is a reference to an array. $#{$array} is the same as $#$array, so we can reduce $#{$j} accordingly.

$#{ $j->[ $#$j ] } = $#$aref % $cc;

We can't do the same with the first $# because of the -> dereference following $j, which is evaluated first.

There's no reason $j has to be an array reference. It can just as easily be an array. We can also give the variables better names while we're at it.

$#{ $result_array[$#result_array] } = $#$in_aref % $elements_per_array;

Often, a line of code can benefit from being more than one line of code. Let's try this:

my $final_aref = $result_array[$#result_array];
$#$final_aref = $#$in_aref % $elements_per_array;

Now we still have our ugly $#$ but at least there's less going on. Then we can break off another piece and add some good whitespace, and a comment for good measure.

my $final_aref        = $result_array[ $#result_array ];
my $elements_in_final = $#$in_aref % $elements_per_array;

# Truncate final array
$#$final_aref = $elements_in_final;

This is certainly much easier to read than our original:

$#{$j->[$#{$j}]}=$#$aref%$cc;

Applying a few of the same principles to the rest of the routine, we get this:

sub listify {
   my ( $in_aref, $elements_per_array ) = @_;

   return if (
       ref $in_aref ne 'ARRAY' or
       $elements_per_array <= 0
   );

   my @result_array;
   for( my $i = 0; $i <= $#$in_aref; $i += $elements_per_array ) {
       push @result_array, [
           @$in_aref[ $i..$i + $elements_per_array - 1 ]
       ];
   }
   my $final_aref        = $result_array[ $#result_array ];
   my $elements_in_final = $#$in_aref % $elements_per_array;

   # Truncate final array
   $#$final_aref = $elements_in_final;
   @$in_aref = @result_array;
}

Much better. I realize there are lots of elegant solutions out there that I would happily break into multiple lines to the original developer's horror. For me, elegance is less importan than clarity. But, breaking up some lines can increase execution time, and that's certainly a factor to be weighed.

Clever code is fun to write, but it has no place in a production environment.

In my opinion, the best reference for writing maintainable code is Perl Best Practices by Damian Conway. It's Perl-centric but much of the advice can be taken into other languages. The module Perl::Critic is intended to critique code against the standards set forth in this book.

You can also "perldoc perlstyle" and check out perltidy for automated formatting.