Friday, July 11, 2008

Why Dollar-Underscore Should Be Avoided

This bit me recently, so I wrote it in a condensed script to share with you fine people:

#!/usr/bin/perl

use strict;
use Data::Dumper;

my @my_array = ( 'one', 'two', 'three' );

for (@my_array) {
    print "Processing: {$_}\n";
    unknown_author::his_proc();
    print "\$_ is now toast {$_}\n";
}

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

package unknown_author; # Hypothetically, someone else's module

sub his_proc {
    open I, $0;
    while (<I>) { # But $_ has been aliased to $my_array[index]
        # Doesn't even matter what's inside - the damage has been done
    }
    close I;
}
Output:

Processing: {one}
$_ is now toast {}
Processing: {two}
$_ is now toast {}
Processing: {three}
$_ is now toast {}
$VAR1 = [
          undef,
          undef,
          undef
        ];
Solution:

1. You can control this by changing your own code:

for my $item (@my_array) {
    print "Processing: {$item}\n";
    unknown_author::his_proc();
    print "\$item is fine: {$item}\n";
}
But in your modules/routines:

2. Don't modify $_ unless you know for sure what's in it.

3. If you feel you must modify $_ in your routine, first say:

local $_;

As in:

sub his_proc {
    local $_;

    open I, $0;
    while (<I>) { # This version of $_ is mine now
        # Now I can do whatever I want with $_
    }
    close I;
}
Remember that $_ is global!

Incidentally, this is covered in Perl Best Practices on page 85, "Dollar-Underscore", as the rule: "Beware of any modification via $_." I like my example better because it is more concise and it happened to me. Also, instead of "be careful", I think we should just avoid $_ altogether.

$_ offers a lot of conciseness, but it has large disadvantages in production code, such as introducing subtle bugs, and reducing readability (adding to the Perl-looks-like-line-noise meme). In addition, clarity suffers when using it as the default and unspecified argument to Perl builtins, such as print.

Save it for the one-liners!

Wednesday, June 11, 2008

Access is denied.

In a Perl program running under Windows, I had this instruction which should launch a browser with a Help document:
system(1,"start tkquiz.html");
For the uninitiated, "start" will use Windows extension-to-program mapping to find the appropriate program to execute in order to read that file. Think of it as a way to double-click on a file programmatically.

Anyway, the above instruction yielded this less-than-helpful error message:

Access is denied.
I experimented a bit. I copied tkquiz.pl to tkquiz.txt and tried this:
system(1,"start tkquiz.txt");
It worked.

Running ls -l on the directory using Cygwin, those two files look like this:

-rw-r--r--  1 David None  6108 Jun  5 22:12 tkquiz.html
-rwxr-xr-x  1 David None 18402 Jun 11 20:52 tkquiz.txt
So Windows wants me to have execute privilege on a file to "start" it. That is a bit of a surprise, since I think of the document being launched as just data. On the other hand, the launching program could be using the data file as code (as in the case of tkquiz.pl which would be launched using Perl).

Still under Cygwin, chmod u+x tkquiz.html fixes my problem. But, I was driving myself a bit batty trying to figure out the Windows way of doing it. I right-click on the file in Explorer and click Properties, but I don't see the Security tab I am used to seeing. Turns out that it isn't there by default.

I do wish Windows would stop saving me from myself. I understand that they have a wide variety of users, but there has to be a better way.

Monday, May 26, 2008

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.

Sunday, May 25, 2008

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.

Saturday, January 05, 2008

Right Tool For The Right Job

I made this map of the maze in an old game called The Lurking Horror. One of the particularly difficult aspects of the maze was that conventional directions (North, South, East, West, Up, Down) were unreliable. As the game explains, you are lost. Therefore, just because you travel north from room #1 to room #2, doesn't mean you can go south and end up back in room #1 (in fact, you must go east).

Therefore, in addition to the normal mapping you might do, I had to add labels to indicate which direction led where. What I ended up with on my first couple attempts to map it (MS Paint, then drawing by hand) was a twisted jumble that would be no use to anyone.

In order to be able to map, and simultaneously untwist the lines connecting the rooms easily, I decided to try Dia. Since I can connect a pathway from one room to another, this allows me to rearrange the rooms in whatever position I want without losing those paths. So, it was easy for me to create a map where no paths crossed, making the final product (convoluted as it is) much easier to read.

I exported it to JPEG format with Dia, then resized it with GIMP.