http://qs321.pair.com?node_id=214904


in reply to Confessions of an Initiate: a first Perl program

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'