Actually I thought the full image files would be prefixed with a "P_" and the thumbnail files would be prefixed with "T_". This
would allow them to be identified if sets e.g. P_FILE1 is full and T_FILE1 is thumbnail.
This makes sense if you are updating the form that invokes
this script at the same time that you are updating the script.
I assume you are referring to parameter names here (strings
that will be passed to the CGI object's "param()" call).
This will entail changing a little more than just the "open()"
call, and it this point, it's worthwhile to go over the whole
thing to resolve some other issues. I'm no CGI guru, but
the opening part seems very strange and basically wrong:
$max_num_files ||= 1;
$Photo_Dir ||= $ENV{'DOCUMENT_ROOT'};
undef @bad_extensions if @good_extensions;
for(my $a = 1; $a <= $max_num_files; $a++) {
my $req = new CGI;
...
That's testing an array and two scalars for previous values
when nothing could have been assigned to them before this;
so $max_num_files is always 1 and @bad_extensions is always
undef. Then, why create a new CGI object on each iteration
of a loop (even given that the loop only operates once)?
Let me suggest the following -- with a grain of salt
since I can't test it (and I'm not a CGI guru) -- which
is based on assuming that the original script you posted was
all there is to it. I'm adding commentary, eliminating
redundant instructions, restructuring the loop, adding some
different error checking on output, and noting
where large blocks of code were removed because they would
never execute anyway (being based on testing uninitialized
variables):
use strict;
use CGI;
my $req = new CGI;
foreach my $type ( qw/P T/ ) # 'P' for Photo, 'T' for Thumbnail
{
my $indx = 0;
while ( 1 ) # we'll exit when $indx gets high enough
{
my $paramName = sprintf( "%s_FILE%d", $type, ++$indx );
last unless ( $req->param( $paramName ));
my $outname = my $inname = $req->param( $paramName );
$outname =~ s/^.*(\\|\/)//; # remove leading path
$outname =~ s/ +/\_/g; # don't want spaces
$outname =~ s/\"//g; # don't want quotes
$outname = "th_$outname" if ( $type eq "T" );
# removed long portion that depended on @good_extensions and/or
# @bad_extensions having contents : no evidence that they ever did
# removed long portion that depended on $auto_rename having a value of
# 1 or 2 and checking whether "$Photo_Dir/$outname" already exists :
# no evidence that $auto_rename is ever non-zero
# ( I think the original posted code forgot to open the input file )
my $insize = ( -s $inname );
( $insize && open( INFILE, $inname )) or next;
if ( open( OUTFILE, ">$ENV{DOCUMENT_ROOT}/$outname")) {
my $outsize = 0;
while ( my $nr = sysread( INFILE, my $buffer, 1024 )) {
my $nw = syswrite( OUTFILE, $buffer );
last if ( $nr != $nw );
$outsize += $nw;
}
close OUTFILE;
if ( $outsize != $insize ) {
last; # might want to report this somehow...
}
# removed long portion that depended on $max_size being non-zero :
# no evidence that it ever was
}
close INFILE;
}
}
Programmers often use code not written by themselves.
It is sometimes a good basis for learning...
I certainly do agree, and I endorse the practice. I think
the point is simply that when you say "I'm working on this
code that someone else left behind...", readers will understand
the situation better (and not blame you for lousy coding). |