[Melbourne-pm] Working with old Perl code

[Kahlil Hodgson]

Hi All,

After Tony's discussion at Wednesdays meeting, I've had some thoughts about
generic strategies for cleaning up old perl code.  I totally agree with 
Tony's
reluctance to mess with the structure of the code too early: the 
potential to
change or break functionality before it is well understood is just to 
great.
One fairly safe strategy, however, is to clean up the use of variables and
references.

The code in Tony's example was clearly from a time before lexical variables
either existed or where in common use.  Simply walking through this code and
inserting 'my' in the appropriate places to bring non-global variables 
into a
lexical scope should be fairly safe (provided you carefully check that the
variables are not being used globally).  More importantly, the process helps
to pin-point the nasty global variables, which you can mark and fix up 
later.
As part of this code walk, you will probably find uses of 'local' that are
really just being used for scoping.  Figuring out which instances can be
replaced with 'my' helps to point out the genuine uses of localization (if
any) which you may also decide to mark and fix up later.

This 'first pass' can be done fairly quickly and will make the code look and
behave a lot more like a modern perl

Older code sometimes uses *TYPEGLOBs instead of variable references. 
Now, for
those who don't know, *TYPEGLOB's are references into the symbol table (the
place where we keep global variables) and simultaneously point to all 
'things'
(or types?) which match (or glob to?) a particular symbol name.  Okay, 
that's
a _very_ rough definition, but *TYPEGLOBs (or *TYPEGOBLINs?) are _very_ odd
creatures.  I found the first chapter of "Advanced Perl Programming" by 
Simon
Cozens particularly enlightening on this topic (contact me off-list if 
you want
to borrow a copy).

Believe it or not, there are sensible places to use *TYPEGLOBs, but they are
relatively rare in modern perl and mostly live in CPAN modules (mostly)
written by people who "really know what they are doing".

Using *TYPEGLOBs in place of variable references is _not_ a sensible 
thing to
do in modern perl.  The code that Tony's is dealing with is a good 
example.

 From memory we had something like:

     &read_config(*my_config);
         ...
     %my_config{'some_key'} = 'some_value';

We could try to replace this with

     my $my_config_ref = {};
     &read_config($my_config_ref);
         ...
     $my_config_ref->{'some_key'} = 'some_value';

But then we're going to have to change the code in 'read_config' to get 
it to
work, and that's not in spirit of making small changes and not breaking
functionality.  You can get around this with the following trick:

     my $my_config_ref = {};
     *my_config = $my_config_ref; # temp until we fix read_config
     &read_config(*my_config);
         ...
     $my_config_ref->{'some_key'} = 'some_value';

The trick changes the 'hash' slot for the symbol 'my_config' to point to the
same hash that $my_config_ref points to.

The following little test script may clarify for those who doubt

     #!/usr/bin/perl

     use v5.10;

     sub read_config {
         local (*my_config) = @_; # NB: can't use 'my' on a TYPEGLOB
         $my_config{'pre'} = 'from config';
     }

     my $my_config_ref = {};
     *my_config = $my_config_ref;
     read_config(*my_config);

     say $my_config_ref->{'pre'};

     $my_config_ref->{'post'} = 'after config';
     say $my_config_ref->{'post'};

     say $my_config_ref->{'pre'};

Once you are happy that this is all working correctly, you can then go into
'read_config' and replace the *TYPEGOBLIN and get rid of the trick (I'm
assuming nothing else calls read_config this way).

Getting rid of the superfluous '&' in front of subroutine calls is another
quick fix.

Finally, after doing all that you can probably try

     use strict;
     use warnings;

and see if the remaining badness is manageable.

Thanks to Tony for bringing up this topic and I hope my suggestions help.

Happy Hacking!

Kal

-- 
Kahlil (Kal) Hodgson                       GPG: C9A02289
Head of Technology                         (m) +61 (0) 4 2573 0382
DealMax Pty Ltd                            (w) +61 (0) 3 9008 5281

Suite 1415
401 Docklands Drive
Docklands VIC 3008 Australia

"All parts should go together without forcing.  You must remember that
the parts you are reassembling were disassembled by you.  Therefore,
if you can't get them together again, there must be a reason.  By all
means, do not use a hammer."  -- IBM maintenance manual, 1925