It's been pointed out to me that many programmers have issues with my blog because I'm writing about allomorphism, exceptions as flow control and other topics that are, frankly, not something a beginner programmer is going to warm to. I think that's a fair point and I'm going to try to start including information aimed at those new to Perl and those new to programming.

I'll start with subroutines. Specifically, an old, horrible example.

Back in the days of CGI being king, you often saw a variant of the following routine to parse CGI data:

 1 # don't use this
 2 sub parse_cgi {
 3   my @pairs;
 4   if ( $ENV{'REQUEST_METHOD'} eq 'GET' ) {
 5     @pairs = split( /&/, $ENV{'QUERY_STRING'} );
 6
 7   }
 8   elsif ( $ENV{'REQUEST_METHOD'} eq 'POST' ) {
 9     read( STDIN, $buffer, $ENV{'CONTENT_LENGTH'} );
10     @pairs = split( /&/, $buffer );
11
12   }
13   else {
14     print "Content-type: text/html\n\n";
15     print "<P>Use Post or Get";
16   }
17   my %formdata;
18   foreach my $pair (@pairs) {
19     my ( $key, $value ) = split( /=/, $pair );
20
21     # Convert plusses to spaces
22     $key =~ tr/+/ /;
23
24     # Convert Hex values to ASCII
25     $key   =~ s/%([a-fA-F0-9] [a-fA-F0-9])/pack("C", hex($1))/eg;
26     $value =~ tr/+/ /;
27     $value =~ s/%([a-fA-F0-9] [a-fA-F0-9])/pack("C", hex($1))/eg;
28
29     $value =~ s/<!--(.|\n)*-->//g;
30
31     if ( $formdata{$key} ) {
32       $formdata{$key} .= ", $value";
33     }
34     else {
35       $formdata{$key} = $value;
36     }
37   }
38   return %formdata;
39 }

There are, frankly, so many things wrong with this (I won't touch on all of them because I could write for hours), but I'll touch on some of the highlights.

Note: this is very, very old code and we generally avoid CGI today, but I still see this and variants cropping up (fortunately, less than I used to).

Black Boxes

The first thing to remember about a subroutine is that it should be a "black box". You pass data in and get data out. Here you would actually want a second routine which reads the form data and and then hands it to the parse_cgi code. Why? See the POST handling? When you read from STDIN, you empty STDIN. That data cannot be reread without jumping through serious hoops (and who wants to explain how to tie STDIN?). What happens if you want to debug that? If you try to print the STDIN data after this subroutine, the data have been read and you have nothing to print. Print the data before this subroutine and the subroutine has no data to read. Oops. You lose.

So a better strategy would be to call it like this:

my %formdata = parse_cgi(read_form_data());

And at the top of the parse_cgi() code:

sub parse_cgi {
    my $raw_form_data = shift;
    my @pairs = split /&/, $raw_form_data;

That allows you to capture the form data before this call and do other things with it, if needed. Of course, it also allows you to do this, if you want:

my %formdata = parse_cgi(read_form_data_from_file($filename));

You might not need to do that, but by separating these behaviors and treating your subroutines as black boxes, you get a lot of flexibility.

Cohesion

Subroutines and methods should generally do one thing and do it well (we call this cohesion). The code in question both reads form data and attempts to parse it. In "Black Boxes", I showed how passing arguments to a subroutine makes it more flexible. This was achieved by separating the reading of data from the parsing of it. However, there's still another classic mistake here:

29     $value =~ s/<!--(.|\n)*-->//g;

I used to see this a lot in code. Aside from the fact that this regular expression is broken badly, what's it doing there? I've never seen anyone explain it, but my guess is that this was some strange attempt to strip server side includes (SSIs) from the data. If so, this is a terribly misguided attempt to provide some security for incoming data (there are much better ways of dealing with this, but I won't touch on that here).

If that's the case, what do you do if you want HTML comments and SSIs? Maybe you want to log the raw data, but since the substitution is poor, it has a good chance of mangling said data and you won't know because you tried to do too much in one routine (separating the reading and parsing can work around this). I can't imagine why you would want to keep them, but if you do, this code won't let you.

If you really do want to properly scrub your incoming data, though, there's a lot more than SSIs you want to eliminate. Thus, you should have dedicated code written for scrubbing the data, not trying to hack it into parsing code.

Understand Your Data

See this?

31     if ( $formdata{$key} ) {
32       $formdata{$key} .= ", $value";
33     }
34     else {
35       $formdata{$key} = $value;
36     }

There are two serious flaws here. First, let's look at a valid query string on a URL (and note that ';' is a valid separator, but the ampersand is all the code split on):

http://www.example.com/?sport=football;sport=baseball

It's perfectly OK to have multiple values for a single key. The venerable CGI.pm would allow you to fetch them with this:

my @sports = $cgi->param('sport');

This code, however, would return this (complete with extra space):

%formdata = (
    sport => "football, baseball",
);

Presumably this is to allow you to split on commas (plus that extra space tucked in there). But what if the value contains a comma? Oops. We have multiple values. That should be an array reference, not a string.

Now let's look at another valid URL:

http://www.example.com/?sport=football;sport=sport=baseball

Guess what your data structure has?

%formdata = (
    sport => "baseball",
);

Why it contains this is an exercise for the reader.

If you don't really understand the nature of your data, you're going to write bad code like this.

Reuse Code

Even for this relatively simple task (and there are plenty of other issues I haven't touched on), it's easy to misunderstand what's going on. That's why, when CGI was so popular, the Perl community enjoined everyone to use the CGI.pm module. We don't do that any more -- there are far better ways of approaching this problem -- but we still don't recommend that you write your own code.

Summary

Subroutines should be black boxes that do one thing well. Don't rely on globals and don't do too much. They'll be more flexible that way.

These "new user" friendly blog posts will be an "on again/off again" sort of thing, but if they turn out to be popular, I'll write more.