Tag Archives: bad perl

Learning from Bad Code

I’ve written before about Linux Format’s habit of sharing badly written Perl code. I thought things were improving, but in the new edition (November 2012, issue 163) they’re back to their old tricks.

This time it’s a tutorial called “Starfield: Learn new languages”. In this tutorial Mike Saunders writes similar starfield simulation code in C, Python and Perl. Mike’s loyalties are made perfectly clear when these three sections are entitled “Low Level”, “High Level” and “Unusual Level” respectively, so I wasn’t expecting much. But here’s what I got.

#!/usr/bin/perl

$numstars = 100;

use Time::HiRes qw(usleep);
use Curses;
$screen = new Curses;
noecho;
curs_set(0);

for ($i = 0; $i < $numstars ; $i++) {
  $star_x[$i] = rand(80);
  $star_y[$i] = rand(24);
  $star_s[$i] = rand(4) + 1;
}

while (1) {
  $screen->clear;

  for ($i = 0; $i < $numstars ; $i++) {
    $star_x[$i] -= $star_s[$i];
    if ($star_x[$i] < 0) {
      $star_x[$i] = 80;
    }

    $screen->addch($star_y[$i], $star_x[$i], ".");
  }

  $screen->refresh;
  usleep 50000;
}

Let’s be clear here. This code works exactly as Mike intended it to. But if you’re writing sample code for other people to learn from, I think you should be setting the bar a little higher than “works for me”. I think you should should be aiming to show people good quality code that can be easily mainitained. And this code falls well short of that aim.

Let’s look at some of the problems in more detail:

  • No use strict or use warnings. Let’s be generous and assume the editor removed them to save space.
  • Undeclared variables. Of course. if the code contained use strict then the variables would all have had to be declared. Not declaring them means that we’re using package variables rather than lexical variables. In code this simple it doesn’t make a difference. But it’s a bad habit to get into.
  • Indirect object notation. When creating the Curses object the tutorial uses the syntax $screen = new Curses. Again, not a problem in this program, but a really bad habit to be encouraging. In the article’s defence, the documentation for the Curses module only includes this flawed syntax.
  • Split data structures. The author of the article says “instead of having an array of stars containing coordinates and speeds for each one (i.e. an array of arrays), to make things simpler we’ve just set up three arrays.” I read this to mean “I’ve never been able to work out how to make arrays of arrays work in Perl so I’ve taken the easy way out.” This is, of course, a terrible idea. Linked data items should be stored in the same data structure.
  • C-style for loops. The mark of a C programmer who never really got to grips with Perl. The C-style for loop is rarely used in Perl code. The foreach loop almost always leads to more readable code.
  • Magic numbers. The size of the screen and the maximum speed of the stars appear as numbers in the code. Even if you’re not a Perl programmer, surely you would know that it’s good practice to move those into variables or constants.

With all that in mind, here’s my version of the program.

#!/usr/bin/perl

use strict;
use warnings;

use Time::HiRes qw(usleep);
use Curses;

my $numstars = 100;
my $screen_x = 80;
my $screen_y = 24;
my $max_speed = 4;

my $screen = Curses->new;
noecho;
curs_set(0);

my @stars;

foreach my $i (1 .. $numstars) {
  push @stars, {
    x => rand($screen_x),
    y => rand($screen_y),
    s => rand($max_speed) + 1,
  };
}

while (1) {
  $screen->clear;

  foreach my $star (@stars) {
    $star->{x} -= $star->{s};
    $star->{x} = $screen_x if $star->{x} < 0;

    $screen->addch($star->{y}, $star->{x}, '.');
  }

  $screen->refresh;
  usleep 50000;
}

What do you think? Is it more readable? Easier to maintain? Are there any problems or improvements that I’ve missed?