Turning a simple working script into an elegant working script 1

[twitaboy]

My perl cgi script is working, however it is very ugly. I am hopeing that a guru could help to teach me how to write more elegant code.

The code below is a section of a script that reads and uploads a series of fields and files, for each of six products. The chunks of code to upload the data are almost exactly the same, except they differ in the filenames according to the product number (eg. the code below is for products 3 and 4).

I _think_ I need to get these into a conditional loop and work in an array of filenames or a counter. Any tips or code examples would be greatly appruciated.

Matt



#this section uploads 2 images (photo3_large,photo3) and several text and checkbox fields for product 3.

$filename3l = $query->param("photo3_large"
$filename3_text = $query->param("photo3_text"
$filename3_name = $query->param("photo3_name"
$filename3_price = $query->param("photo3_price"
$filename3_replace = $query->param("photo3_replace"
$filename3l =~ s/.*[\/\\](.*)/$1/;
$upload_filehandle3l = $query->upload("photo3_large"

open UPLOADFILE3l, ">$upload_dir/$filename3l";
while ( <$upload_filehandle3l> )
{
print UPLOADFILE3l;
}
close UPLOADFILE3l;


$filename3s = $query->param(&quot;photo3&quot;
$filename3s =~ s/.*[\/\\](.*)/$1/;
$upload_filehandle3s = $query->upload(&quot;photo3&quot;

open UPLOADFILE3s, &quot;>$upload_dir/$filename3s&quot;;
while ( <$upload_filehandle3s> )
{
print UPLOADFILE3s;
}
close UPLOADFILE3s;







#this section uploads 2 images (photo4_large,photo4) and several text and checkbox fields for product 4.

$filename4l = $query->param(&quot;photo4_large&quot;
$filename4_text = $query->param(&quot;photo4_text&quot;
$filename4_name = $query->param(&quot;photo4_name&quot;
$filename4_price = $query->param(&quot;photo4_price&quot;
$filename4_replace = $query->param(&quot;photo4_replace&quot;
$filename4l =~ s/.*[\/\\](.*)/$1/;
$upload_filehandle4l = $query->upload(&quot;photo4_large&quot;

open UPLOADFILE4l, &quot;>$upload_dir/$filename4l&quot;;
while ( <$upload_filehandle4l> )
{
print UPLOADFILE4l;
}
close UPLOADFILE4l;


$filename4s = $query->param(&quot;photo4&quot;
$filename4s =~ s/.*[\/\\](.*)/$1/;
$upload_filehandle4s = $query->upload(&quot;photo4&quot;

open UPLOADFILE4s, &quot;>$upload_dir/$filename4s&quot;;
while ( <$upload_filehandle4s> )
{
print UPLOADFILE4s;
}
close UPLOADFILE4s;

 

[fishiface]

Unless you require high performance (which I suspect you don't in a file upload scenario), consider using a data structure for each product: they are so easy to use in perl that it's often quicker overall (by the time you've found and blatted the various typos!).

Also seriously consider the :shortcuts parameter to CGI - it cuts out an awful lot of typing.

There is always a balance to be struck between the elegance of code and the legibility/maintainability: Each coder has their own taste. The following is slightly more verbose than I would normally be but is, I hope, clearer for it.

[tt]my $prd3 = get_product( 'photo3' );
my $prd4 = get_product( 'photo4' );

# can refer later to $prd4->{price}, etc;

sub get_file {
my $name = $_[0];
my $path = &quot;$upload_dir/$name&quot;;
my $handle = upload($name);
open( FH, &quot;>$path ) or die &quot;$0: can't open $path: $!&quot;;
print FH while (<$handle>);
close FH or die &quot;$0: can't close $path: $!&quot;;
}

sub get_product {
my $name = $_[0];
my %product = (
large => param( &quot;${name}_large&quot; ),
text => param( &quot;${name}_text&quot; ),
name => param( &quot;${name}_name&quot; ),
price => param( &quot;${name}_price&quot; ),
replace => param( &quot;${name}_replace&quot; )
);
$product{large} =~ s/.*[\/\\](.*)/$1/; # untaint
get_file( $product{large} );
get_file( param($name) ); # maybe untaint this?
return \%product;
}[/tt]

 

[twitaboy]

Cheers fishiface,
you have opened my eyes to the world of variable passing to subroutines. Up until this point I only used subroutines to improve the structure of the script without passing variables. I have another piece of similiar repeated code with different variable names that I will have a crack at using your method. Thanks of the help.