Confessions of an Initiate: a first Perl program

[mooseboy]

Fellow monks,

From time to time, all of us, whatever our level, have something to confess. As a humble Initiate, I must therefore confess that the code I place before you now is not just my first Perl program. It is my first program in any language. Thus I ask for your kind guidance and suggestions as to how it could be improved, and beg your forgiveness for whatever egregious Perl sins may be contained therein. Thank you, brothers.

#!/usr/bin/perl
use diagnostics;
use warnings;
use strict;
use CGI;

my %all_quotas;
my @members = ('Algeria', 'Indonesia', 'Iran', 'Iraq', 'Kuwait', 'Liby
+a', 'Nigeria', Qatar', 'Saudi Arabia', 'UAE', 'Venezuela', 'total OPE
+C' );

# Sorry about the ghastly ASCIIbetical hack. Suggestions welcome!

my $query = CGI->new();

print $query->header("text/html"),
      $query->start_html( -title   => "QuotaBase: a database of OPEC o
+il production quotas",
                          -bgcolor => "#cbcbcb" ),
      $query->h1("Welcome to QuotaBase!"),
      $query->start_table(),
      $query->Tr(),
      $query->td({-width => "600"}),
      $query->p("QuotaBase is an interactive database of OPEC oil prod
+uction quotas. By default, the <b>current quotas</b> are displayed. T
+o view <b>historical quota information</b>, select the period you wan
+t from the drop-down list and click the 'Show quotas' button. A table
+ of all the quotas for that period will be displayed."),
      $query->end_td(),
      $query->end_Tr(),
      $query->end_table(),
     $query->start_form(),
      "Choose a period: ",
      "&nbsp;",
      $query->popup_menu( -name=>'period',
                          -values=>['Jan 02 - Dec 02', 'Sep 01 - Dec 0
+1', 'Apr 01 - Aug 01', 'Feb 01 - Mar 01', '31 Oct 00 - Jan 01', '1 Oc
+t 00 - 30 Oct 00'],
                          -default=>'Jan 02 - Dec 02'),
      "&nbsp;&nbsp;",
      $query->submit(-name=>'period', -value=>'Show quotas'),
      "&nbsp;&nbsp;",
      $query->defaults('Reset current quotas'),
      $query->endform;
      $query->end_p(),

build_database();

my @$period = $query->param('period'); 
process_query(@$period);

print $query->end_html;

##### subs #####

sub build_database {     # thanks to Zaxo
    while (<DATA>) {
    ( my $period, $_ ) = split ':';
    @{$all_quotas{$period}}{@members} = split ' ';
    }
}

sub process_query {
    ($period) = @_;
    if ($period) {
        print_table();
    } else {
        $period = 'Jan 02 - Dec 02';
    print_table();
    }
}

sub print_table {
        print "<table border=1>\n";
        print "<tr><td colspan=2 align=center><b>$period</b></td></tr>
+\n";
        foreach my $member ( sort keys %{ $all_quotas{$period} } ) {
            print "<tr><td width=130 align=left>$member</td>
            <td width=130 align=right>$all_quotas{$period}{$member}</t
+d></tr>\n";
        }
        print "</table>";
}

__DATA__
Jan 02 - Dec 02:        693 1125 3186 0 1741 1162 1787 562 7053 1894 2
[phone]
Sep 01 - Dec 01:        741 1203 3406 0 1861 1242 1911 601 7541 2025 2
[phone]
Apr 01 - Aug 01:        773 1255 3552 0 1941 1296 1993 627 7865 2113 2
[phone]
Feb 01 - Mar 01:        805 1307 3698 0 2021 1350 2075 653 8189 2201 2
[phone]
31 Oct 00 - Jan 01:     853 1385 3917 0 2141 1431 2198 692 8674 2333 3
+077 26700
1 Oct 00 - 30 Oct 00:   837 1359 3844 0 2101 1404 2157 679 8512 2289 3
+019 26200

[fruiture]

First of all: this does definetely not look like a first program. You have already done things lots of people need years to grok: strict, warnings, CGI.pm .

First hint: often it's just more beatiful to hard-code a list using qw//, not quting each element of the list...

my @members = qw/Algeria Indonesia .../;

Second: The HTML-generating functions of CGI.pm work differently. You must nest the functions like the resulting HTML.

print $query->td( { -style => 'width:600px' },
    $query->p( '....' )
);

Avoid the use of globals like your $period variable (a lexical that works like global here): First, it's confusing, especially when you have a lexical $period within a sub and second: it's obfuscating anyways. Rather pass such things as parameters.

Finally you're inconsistent about the use of CGI.pm to generate HTML. A very personal note: please consider using XHTML.

--
http://fruiture.de

[VSarkiss]

Minor nit: He can't use qw because he has entries like Saudi Arabia that have spaces in them.

[Chmrr]

I'd second most of the suggestions that Thesus and fruiture offered. Furthermore, I'd comment that as this is a purely linear script, there are few advantages to using subroutines other than the visual partitioning. The downside is that it promotes either the use of globals, as you did, or rediculous amounts of variable passing. And globals, as we all know "are always bad."^{1} As such, were I writing this, I would do away with the subroutines and just deliniate the sections with comments.

Also, you hard-code the values of the popup box -- why, when you can get at them far easier by using keys %all_quotas? This will also prevent errors caused by typos, as well as allowing for expansion if you add more lines of data. The same concept applies to the default value of the popup.

Another tip is that most of the lines of process_query boil down to:

sub process_query {
  $period = $query->param('period') || (keys %all_quotas)[0];
  print_table;
}

Note the use of the || operator to set a default -- that's one of its primary uses. Lastly, the gigantic paragraph that you put in quotes looks ripe for being a HERE document.

You can see the agglomeration of the changes here, and the result here. Ignore how I removed your __DATA__ section -- I had to do so because they don't play nice with mod_perl. It is an excellent and appropriate use of __DATA__, but not one which works with mod_perl, unfortunatly.

{1} I hate generalizations like this. But in this case, it's usually good advice. There are times to use 'em, but here we don't need or want 'em.

perl -pe '"I lo*`+$^X$\"$]!$/"=~m%(.*)%s;$_=$1;y^`+*^e v^#$&V"+@( NO CARRIER'

[mooseboy]

Wow, many thanks to all who responded so far! I'll have to spend a while to try and synthesize all the thoughtful suggestions and see if I can come up with an improved version.

Cheers, mooseboy

[mooseboy]

Chmrr, many, many thanks for your "native Perl" rewrite of my effort! One question: in your version, the dates in the pop-up menu are not ordered -- is there a way to order them so that the most recent is first, then the next most recent, etc, as in the original?

<Thanks, mooseboy

[Chmrr]

Not to sound cliche or anything, but you must first understand the cause of the problem; then the solution will become obvious. The reason the popup ordering isn't in order is because it is gotten through keys %some_hash -- which, as we know, produces said keys in no (easily) predictable or useful order. While it is possible that one could attempt to re-sort the list that it returns into some useful order, the current date format looks to make that rather hard.

Given that, what we really want is to keep track of the keys of the hash as we add them, in some structure which preserves order -- say, like an array or something..

Thus, we do is have an @dates array (declared next to where we define %quotas, say), and push $period onto the end of @dates as we go though loading the data. Later on, we can use @dates instead of keys %quotas if the ordering matters.

I've intentionally not provided much explicit code here. Consider it an "excercise left to the reader." ;>

perl -pe '"I lo*`+$^X$\"$]!$/"=~m%(.*)%s;$_=$1;y^`+*^e v^#$&V"+@( NO CARRIER'

[Theseus]

It's a hell of a lot nicer than my first Perl program, that's for sure. I learned Perl by dissecting scripts I found on the web, so I picked up all the bad practices involved with that(imagine learning from Matt Wright's scripts, and other such scripts available on the web). I didn't use strict or warnings, all global variables, stole a CGI parsing routine(one very similar to this, with some tweaks I added of my own), used nothing but flatfile ascii databases with "|:|" or some other uncommon string separating fields... It was a nightmare.

Be thankful you started out on the right path, it took me a long time to learn the error of my ways and become comfortable with the concepts that I should have been learning from the beginning(thank god for Coping with Scoping, that's all I can say).

[ibanix]

This can't be your first program.

This program actually works.

/rgds,
ibanix


<-> In general, we find that those who disparage a given operating system, language, or philosophy have never had to use it in practice. <->