OSCON: Extreme Perl Makeover

This is a summary of the “Extreme Perl Makeover” talk today by Peter Scott. Basically overviewing refactoring legacy code to modern Perl.

“Why so many ugly Perl programs?” Answer: “There’s more than one way to do it.” Unfortunately, some of those ways stink.

White Space is for Wimps

When you come across code that is poorly formatted or has “random” indentation, try using perltidy. It will in some cases make the code longer, but in the end, it should be far more readable. perltidy can also be customized to meet your preferences (~/.perltidyrc).

The Documentation Hound

  • s/^#+\n//mg
  • Move extensive comments into POD documentation later in the file, or a separate .pod file.
  • Keep function/method signatures and descriptions.
  • Make the code tell the story.
  • Preserve comments that answer “Why?”.
  • For local programs, author and history information can be handled by version control.
  • For distributions:
    • Move author information to a README.
    • Move history information to a changelog.

Pa Didn’t Have Hashes

$found = 0
for ($i=0; $i<=$#clients; $i++) {
  if ($clients[$i] eq $client) {
      $counts[$i]++;
      $found = 1;
  }
}
if (!$found) {
    $clients[$i] = $client;
    $counts[$i] = 1;
}

Which can be better handled by:

$client_count{$client}++;

String Manipulation, BASIC style

# Turn groups of consecutive non-digits into a
# single space.
$repl = ' ';
for ($off = 0; $off < length($str); $off++) {
    $c = substr($str, $off, 1);
    if (index("012345789", $c) < 0) {
        substr($str, $off, 1, $repl);
        $off-- unless $repl;
        $repl = '';
    } else {
        $repl = ' ';
    }
}

Which can be better handled by:

$str =~ s/\D+/ /g;

Also, the longer version omitted the number ‘6’, which is more than likely a bug.

Nice formatting, but…

if ($month eq '1') { $month = '0' . $month; }
if ($month eq '2') { $month = '0' . $month; }
if ($month eq '3') { $month = '0' . $month; }
if ($month eq '4') { $month = '0' . $month; }
if ($month eq '5') { $month = '0' . $month; }
if ($month eq '6') { $month = '0' . $month; }
if ($month eq '7') { $month = '0' . $month; }
if ($month eq '8') { $month = '0' . $month; }
if ($month eq '9') { $month = '0' . $month; }

Which of course is repetitive and unnecessary:

$month = sprintf("%02d", $month);

Way too much time on their hands

print "<title>$title</title>\n";
...
print "<h1>$heading</h1>\n";

Instead of mixing language types so closely, use template modules to put the HTML content in a separate file.

The Perils of Copy and Paste

my ($sec, $min, $hour, $mday, $mon, $year, $wday, $yday, $isdst) = localtime(time);
# But now only use $mon and $mday...

Perphas instead…

my ($mday, $mon) = (localtime)[3,4];

Scope? What is this thing you call scope?

Have you ever seen dozens of variables declared at the top of a subroutine?

Move variable declaration to latest possible point. An exception to this rule is configuration settings (put those in a separate file if appropriate).

Use inline declarations for loop variables:

foreach my $dog (@schnauzers)
while (my $imp = shift @demons)

You can even carry this further:

getopt('dq:v', \my %Opt);

TrackBack

TrackBack URL for this entry:
http://bradchoate.com/mt/feedback/tb/1371

1 Comments

TweezerMan said:

Under "The Perils of Copy and Paste", the variable values are accidentally transposed in the shortening of the code. It should be:

my ($mday, $mon) = (localtime)[3,4];

About

This article was published on July 28, 2006 11:29 AM.

The article previously posted was OSCON: Subversion Best Practices.

The next article is OSCON: Perl Hacks you Never Knew Existed.

Many more can be found on the home page or by looking through the archives.

Powered by Movable Type