John Bokma MexIT
freelance Perl programmer

CGI, templates, and bad Perl code

Tuesday, November 22, 2005 | 2 comments

In reply to a question in the Usenet group alt.www.webmasters on how to create space between elements in HTML I replied with a Perl snippet (as a joke, of course):

print "<br>\n" for 1 .. 100000;         # outer space

Of course this was followed by advice to use cascading style sheets (CSS) to set margin(s) on the elements that require some space.

Anyway, M.B. Stevens suggested a template solution instead of generating HTML like I did. And in many cases he's right. There are several ways to generate HTML via Perl, and using a templating system might be a very good solution. In his reply he also included a link to his article: "Substitutes for the CGI module's HTML functions". I read the article, and also the code. I have included the code below, because it's code I certainly wouldn't recommend to use.

Don't use the code given below

 1  # Read, substitute, print to browser and close
 2  sub dofile {
 3      my $ff = shift;
 4      open(XIN, "$ff") || err('open', $ff );
 5      @dat = <XIN> ;
 6      make_substitutions @dat;
 7      print "@dat\n";
 8      close XIN || err('close', $ff );
 9  }
10  # Substitute strings into commands in template data
11  sub make_substitutions {
12      foreach my $s (@_) {
13          $s =~ s/TEMPLATE_REPLACE_FNAME/$nm/eg;
14          $s =~ s/TEMPLATE_REPLACE_TITLE/$title/eg;
15          # and so on for whatever variables
16          #   you might need to replace
17          #   inside the template data.
18      }
19  }

Edit: as of November the 29th, 2005 M.B. Stevens has removed the above code, and replaced with code very similar to what I posted on Usenet (and given below). Instead of crediting me in some way (I did argue with him at great length on what was wrong with his code), he decided to quote similar code from a book written in 2002, since according to him: that's the original. And I am guilty of plagiarism...

Reading the template file: dofile

I will now step by step explain what is wrong with the above code. Most things I also mentioned in my reply on Usenet. I have added line numbers for easy reference to the code.

The first two remarks have to do with picking good names. The name "dofile" (line 2) is very badly chosen. It's as generic as using "doit". A better name would describe what the function actually does. In this case it reads in a template file, and substitutes certain strings with the actual value of some global variables. A better name would be: use_template_file.

A similar thing can be said about $ff (line 3). One could say that the function is very small, and it should be obvious that $ff is the filename, but why not use $filename? Since using $filename makes clear at a glance what the expected first parameter of the function should be: a filename, and hence we get self documenting code.

At line 4 the template file is opened for reading. Several things could have be done better. First of all a file handle, XIN is used, without taking some precautions. This means that if dofile is called from code that also uses the XIN file handle, things might go wrong. In order to prevent this clash, before the open the following line should have been added:

local *XIN;

which uses a typeglob as explained on page 79 of Programming Perl, 3rd edition (which also explains that there are other ways to generate new file handles, I come back to that soon).

On the same line is a very common mistake made by novices: putting a scalar between double quotes when it's not needed.

The biggest mistake on the same line is quite a bad one. When the opening for reading fails an error reporting function is called, which based on the parameters given to it just will report open and the filename. Not what exactly went wrong, which is stored in $!, and should have been passed to the error reporting function. Moreover, I would recommend to use "die" (combined with using CGI::Carp 'fatalsToBrowser'). To summarize, a much better version:

open my $fh, $filename
or die "Couldn't open '$filename' for reading: $!";

Which uses a lexical scoped (my) variable to store the file handle (see page 748 of Programming Perl 3rd edition, or perldoc perlopentut: Indirect Filehandles).

At line 6 the make_substitutions function is called. The way it's called (without brackets) doesn't work in the code given, since in order to make it work like that make_substitutions must have been seen before the dofile sub. Also, since a template can have quite a number of lines I recommend to pass a reference to make_substitutions instead (and also to not read all lines into one array). However, this might be just a matter of taste.

The next line has another common mistake. When an array variable is interpolated the elements are joined by the value of $" (which contains a space by default). This means that all lines except the first in the template get an extra space at the beginning. And in HTML whitespace can be significant, for example when the PRE element is used. The addition of a newline at the end of the result is something I wouldn't do, but that's a matter of taste.

For line 8 the same error handling recommendation as for line 4 holds: report the value of $!.

Doing the substitutions: make_substitutions

Another criticism on picking good names: $s is not a good name. If, on the other hand you can live with $s (since it's a short piece of code), you can also live with $_, which is what foreach and the substitution operator default to.

Also, the e option is not required with substitutions (line 13, line 14) since the right hand side acts like double quotes by default, and the substitution of the variables does take place without this option. Hence the foreach loop could have been written as follows:

for ( @_ ) {

    # and so on for whatever variables
    #   you might need to replace
    #   inside the template data.

Note that for is just a shorter way of writing foreach.

In case you are wondering where $nm and $title are coming from: those variables are globals. I certainly wouldn't use globals like this, but probably a matter of taste.

A better template solution

To illustrate my criticism to M.B. Stevens (and other readers) I included a better solution in my reply, which follows below:

 1  sub use_template_file {
 3      my ( $filename, $variables ) = @_;
 5      open my $fh, $filename
 6          or die "Can't open template '$filename' for reading: $!";
 7      while ( <$fh> ) {
 9          s{TEMPLATE_REPLACE_(\w+)}{
11              defined $variables->{ lc $1 }
12                  ?  $variables->{ lc $1 }
13                  :  "'\L$1\E' not defined"
14          }ge;
16          print;
17      }
19      close $fh
20          or die "Can't close template '$filename' after reading: $!";
21  }
24  use_template_file(
26      'helloworld.tmpl', {
28          title => 'Hello, world!',
29          :
30          :
31      }
32  );

This example reads the file, and processes lines just after they are read. By using the implicit variable $_ the code looks maybe a bit cryptic at first, remember that both the substitution and the print use this variable as a default.

The substitution is where a lot of things happen at the same time. In the pattern match (left hand side) the part after TEMPLATE_REPLACE_ is captured and stored in $1. In the replacement (right hand side) an expression is used, and hence in this case the e option is required.

What the expression does is checking if the lower case version of the part after TEMPLATE_REPLACE (a "word") has a defined value associated in the hash passed by reference to the use_template_file function. If this is the case, the match is replaced with this value. If it's not defined, a warning message is used as a replacement.

Finally, an example on how to call the use_template_file function is given. This will replace TEMPLATE_REPLACE_TITLE in the template with: Hello, world!

Note: originally (in my post on Usenet) the template filename used the extension tmp. At moment of writing this blog entry I considered that confusing, since it might be mistaken for a temporary file. Also the warning when a variable is not defined has been slightly changed (line 13). The \L forces all following characters to lowercase until \E is encountered and hence the contents of $1 is made lowercase, and hence the same as the corresponding lowercase key in the hash. Finally, a disadvantage of this method is that TEMPLATE_REPLACE_TITLE (for example) can't be directly followed by a character belonging to the \w class.


Before you are going to add the above code snippet to your own CGI programs make sure to check out the comprehensive Perl archive network (CPAN) first for existing template handling modules. You might find a much better solutions to your problem, for example: Text::Template.


Please post a comment | read 2 comments, latest by John Bokma | RSS feed
Tree frogs and vinegaroons near Teocelo >
< A Perly Day