Learning from Bad Code

Posted on 16 September, 201228 September, 2014 by Dave Cross

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;
}

#!/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;
}

#!/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?

13 thoughts on “Learning from Bad Code”

Olivier Mengué (dolmen) says:

17 September, 2012 at 12:19

Go further with “use constant” !

Reply
Mike Saunders says:

17 September, 2012 at 13:40

Hi,

It’s me, Mike Saunders, the author of the article. While you’re correct that the code isn’t perfect, elegant, refined Perl, that wasn’t the goal. It’s not an in-depth, advanced tutorial for wannabe professional Perl developers. No, it’s “do something cool in different languages” — making people aware that there’s more than C/SDL.

Hence “Unusual Level” for Perl (we know that many of our readers think of Perl as unusual and esoteric). Claiming that it has something to do with my “loyalties” is just silly. As is saying “I’ve never been able to work out how to make arrays of arrays work in Perl”. These are just daft little assumptions and snide remarks, and not worth me bothering with 🙂

For space reasons, I had to fit the code into ~30 lines, hence the hard-coded values. So again, I know it’s not God-tier Perl, and I’m not a Perl guru. But that wasn’t at all what the tutorial was about.

Cheers,
Mike

Reply
1. Dave Cross says:
  
  17 September, 2012 at 14:18
  
  Hi Mike, thanks for taking the time to comment.
  
  On reflection, I do agree that some of my comments were a little over the top. But perhaps now you see how I feel reading a magazine where every mention of Perl comes with a snide comment attached.
  
  We in the Perl community are aware that the language has an image problem. We’re doing what we can to address that, but articles like yours don’t help.
  
  I know that the programming world is full of people who dabbled in Perl briefly fifteen years ago and whose knowledge of the language was largely gained from reading code from Matt’s Script Archive. It’s fine that those people still throw together the occasional hacky Perl program to get their job done. No-one would object to that at all.
  
  The problem comes when code like that is published in a magazine like LXF. That legitimises the bad practices that the Perl community is trying to bury.
  
  I know a bit of Python and Ruby. I can hack up a quick script in either of them. But if I was asked to include some Python or Ruby code in an article I’d get it checked out by someone who knew more about the language than I do.
  
  No-one expects “God-tier Perl” in an article like this. My replacement version was just what I would write as a throwaway version of your program. It was the level of Perl that I’d expect any programmer to be able to reach easily.
  
  So I’m sorry about the intemperate remarks in my blog post. But please stop publishing amateur fifteen-year-old code as an example that people will copy and learn from.
  
  Graham Morrison has my email address. Please feel free to use me for informal code reviews of any Perl code you might be tempted to publish.
  
  Reply
2. Christopher Cashell says:
  
  23 September, 2012 at 06:44
  
  You make some valid points, and Dave may have been a bit harsher than necessary, but I’m going to have to agree with his overall critique. If you’re going to write code for publication, that code should follow at least basic best practices. This isn’t “God-tier” or “guru” Perl, but simply proficient Perl.
  
  I can’t help but wonder if it’s your readers that think of Perl as “unusual and esoteric” (I’ve never heard it described that way), or your own view foisted on them. Certainly, the poor quality of the Perl code from the article suggests a level of unfamiliarity that might have someone seeing the language through the lens of ignorance.
  
  My experience with the Perl community is that they are very welcoming of people making an honest effort. I imagine that if you posted your code to any number of places prior to publication, requesting review or suggestions, you would receive plenty of assistance, and you could avoid people pointing out the flaws post-publication.
  
  Reply
Lars Balker says:

17 September, 2012 at 13:54

There are exactly 10 empty lines in Dave’s 40 lines, so clearly the argument that you didn’t have room is invalid.

Reply
DJ Adams says:

17 September, 2012 at 13:57

Hi Mike

I thought it was interesting that many readers of Linux Format think of Perl as “unusual and esoteric”. I read Linux Format occasionally, and I’m not one of them. As a Linux user, I would consider Perl as fairly standard, certainly not esoteric, perhaps old. After all, it’s arguably one of the most Linux/Unix focused scripting languages if you consider its influences. Perhaps classing Perl in the “Unusual Level” category only serves to perpetuate this confusion.

cheers
dj

Reply
Simon Cozens says:

17 September, 2012 at 13:58

Also, all the constants could be put into one line with a my ($a, $b, $c) = ... construct.

Anyhow. Just my personal practice, but if I’m not particularly excellent at something, I try to avoid writing magazine articles about it.

Reply
Abigail says:

18 September, 2012 at 10:11

Well, Mikes code uses less than half the memory of yours. Mike uses three NVs per additional star, you use three NV and an PVHV for each star. Which more than doubles the memory. For 100,000 stars, that’s less than 10Mb for Mike, versus over 23Mb for you.

On top of that, you’re using string literals (the hash keys) to access your data. You’re gung ho about use strict and warnings, but you’re throwing out most of its advantages by using string literals — they won’t give compile time errors on typos; at best you get a run time warning (but not, say, if you typoed on the LHS of a -=).

Reply
Aristotle Pagaltzis says:

19 September, 2012 at 14:50

Things I would have done:
use strict
fix indirect the method notation
add constants
switch to foreach loops (1..$numstars for the first, 0..$#star_x for the second)

But changing the data structure seems gratuitous to me.

Reply
1. Jim says:
  
  9 November, 2012 at 11:12
  
  Hi,
  
  Late to the party, but I still feel the need to stick my oar in 🙂
  
  I think that changing the data structure is important. For a long time I’ve felt that hashes are at the heart of perl, and until you’re fluent with them you haven’t really “got” perl.
  
  If you’re aiming to give people a taste of a different language I think that you should showcase the differences rather than write the same code with slightly different syntax. For a snippet like this I might have foregone the good practice stuff (use strict, declaring constants and so on) in favour of the hash data structure and foreach loop. Would populating @stars with a map have been a step too far?
  
  Anyhow, I would like to have seen something to pique the reader’s curiosity, to make them think “how does that work?” rather than “you can write BASIC in any language if you try hard enough” 🙂
  
  Reply
Pingback: Week Notes 37 & 38 | Davblog
Pingback: COMPUTER MAGAZINE – [PDF] – Linux Format – SEPTEMBER 2012 (UK) | My Blog
Pingback: Commandments of Good Code - Webnesday