[ale] perl tips [ Was Re: [ale] Q: q'n'd script to make 'thumbnails' with ImageMagick? ]

Fletch fletch at phydeaux.org
Wed Jul 24 23:39:35 EDT 2002



        Random sugguestions (and not a personal attack; I'm just bored
and feeling constructively critical :).


> #!/usr/bin/perl
> 
> 
> # use diagnostics;

        use strict;!  use strict;!  use strict;!

        Those 11 little characters will save you inumerable
headaches.  Maybe not today, maybe not tomorrow, but sometime it'll
catch a mispelling and save you the 4-to-17+ minutes you'd otherwise
spend chasing it down.


> my $image_dir="gallery/images";
> 
> if ( ! -d "gallery" ) { `mkdir -p $image_dir`; }
> if ( ! -d "gallery/images" ) { `mkdir $image_dir`; }

        perldoc File::Path for an way to do this (mkdir -p, that is)
without resorting to external mkdir.  Also if you're simply making a
single directory perl has a perfectly servicable mkdir builtin.  Not
that it's a big deal doing it once, but in the general interest of
efficiency it's better not to backtick out (which starts up an
external process and possibly a /bin/sh) when perl can do the same
thing natively).


> open (INDEX, "> gallery/index.html");

        *Always* check the return value from open(); it's one of those
things like `use strict' that you'll be glad you got in the habit of
using (and to be fair, it is done for all of the other open() calls
below).


> print INDEX "<html><body bgcolor=\"white\" marginheight=0 marginwidth=0>\n" .
> 	"<div align=center>\n" . "<h1>More Student Photos</h1>\n" .
> 	"Select the small photo if you wish to see a larger image\n<p>\n" .
> 	"<table border=0 cellpadding=0 cellspacing=0>\n<tr>";

        This'd be more readable using either '', qq{}, or a heredoc.

print INDEX <<EOT;
<html>
<body bgcolor="white" marginheight="0" marginwidth="0">
...
EOT

        Also, print takes a list.  Concat'ing rather than using commas
just makes more work (ok, a very very small amount more work).  

        But the heredoc makes more sense in this case anyhow. :)



> 	$sf = substr($fn, index($fn, "."));

        "bob.jameson.jpg", "bob.anderson.jpg".  Oops.

        perldoc File::Basename, or use something like /^(.*)\.[^.]+$/.

> 	if ($random eq "y") {
> 		$prefix = (int rand 500) + 100;
> 		while ( -f $image_dir . $prefix . $sf ) {$prefix = int rand 500 + 100;}

        You've got a possible race condition in using -f.  You might
consider using sysopen and O_EXCL instead.  That would fail with $!
set to EEXIST if it already was there; if it succeeds you know that
anything else that tries the same with the same path will fail.

        Not that it's crucial in this context as you'd probably never
have two copies of this running at the same time, but . . . .


> 	print  "moving $fn to $image{$fn}\n";
> 	`convert -geom 190 $fn $image_dir/$thumb{$fn}`;
> 	`convert -geom 570 $fn $image_dir/$image{$fn}`;

        system() rather than backticks would be more conceptually
clear (since you're not doing anything with the output).


> 	print HTML "<html><title>Mrs. Myers' Photo Gallery</title>\n" .
> 		"<body marginwidth=0 marginheight=0 bgcolor=#ffffff>\n" .
> 		"<div align=center>\n" .
> 		"Next Image\n" .
[...]


        Again, a heredoc would be more readable (and you could still
interpolate everything in, and properly quote attributes without
getting all backwhacky).


-- 
Fletch                | "If you find my answers frightening,       __`'/|
fletch at phydeaux.org   |  Vincent, you should cease askin'          \ o.O'
770 933-0600 x211(w)  |  scary questions." -- Jules                =(___)=
770 294-0820 (m)      |                                               U

---
This message has been sent through the ALE general discussion list.
See http://www.ale.org/mailing-lists.shtml for more info. Problems should be 
sent to listmaster at ale dot org.






More information about the Ale mailing list