Nice--I never realized that GD could do all that. I recall it using 8-bit paletted color only. Looks like it might be a light-duity replacement for Image::Magick now?

Some comments:

my $param = "$ARGV[0]";
[download]

Why are you interpolating that into an otherwise empty string? You are using it in an ordinary way, so I don't see any point to it.

How about writing lines 59-63 as

my ($param,$dir,$newWidth,$newHeight,$newQuality)= @ARGV;
[download]

on 65-66,

my @tmp = split(/\\/,$0);
my $name = $tmp[-1];
[download]

You leave @tmp laying around even though you only needed it for the next line. There are various ways of not doing that. But, look at more specific ways of getting the basename (see File::Basename module). But since $name is never used anyway, why bother at all?

Add use warnings; along with the use strict;. Replace comment block on lines 1-11 with POD.

On line 18, if ( @_ ) { wraps the entire function. So, if passed no arguments it does nothing with no error? What if there's only 1 parameter? The check doesn't seem very useful. And if you do make it, don't wrap the whole thing like that, instead do something like

croak "Wrong number of parameters at" unless @_ == 3;
[download]

Also in that function, my $file = "$_[0]"; my $newWidth = $_[1]; again, what's with the quotes (sometimes)? The usual way is:

my ($file, $newWidth, $newHeight) = @_;
[download]

Ugh: my $newFile = "new".$file; try replacing 21-22 with simply

open(JPEG,">new$file")
[download]

that is, interpolate $file right where you need it. And don't forget the or croak "Cannot open [new$file] for writing. OS says $!"; Always follow open with "or die..." or other check.

Take a look at if ( $_[3] ) { $quality = $_[3] if ( $_[3] =~ /[1..100]/ ) ; } If you're checking the range to be between 1 and 100 number, that won't do it! [1..100] is a character class, meaning the same as [10.] or matching any character that's a 1, 0, or dot. You will pass the test if your string contains any of those characters somewhere. So 43 fails, but "hi." passes.

Give it a name, not $_[3]. Then, don't just silently ignore it if it doesn't match your needs!

So, to rewrite that part, follow me:
Start by adding $quality to the param list. If not passed in, it will be undef. Then, say $quality||=100; so it will be 100 if it was undef (or zero). Then, check for range: croak "quality must be 1..100 at" unless $quality >=1 && $quality <= 100; On 32-34, check this out: my ($currentWidth,$currentHeight)=$myImage->getBounds(); See, use multiple return values without using an array variable. Cool, huh?

On lines 38-44, you correct the aspect ratio by changing the requested height to be consistant with the width, only if it wasn't square to begin with. You say if it was square, go ahead and allow arbitrary resizing? If that's a peculuarity of your problem, it should be noted in the comments at the top. If you remove the check, why even have the user specify a height? You always ignore it and figure it from the new width. Leave it out, or get fancier (ask me when you're ready).

On line 72, don't put the & before the function name. That's really old syntax. For the pattern, japhy can tell you more about patterns. But I know that the leading .* is useless. Even without, it means the same thing: match if ".jpg" is present anywhere in the string. I think you want /\.jpg$/, to match the end. Better yet, /\.jpe?g$/i. Again, you silently do nothing but ignore the request if the file doesn't end in .jpg.

It also seems to me that instead of doing tests to assure that the name's directoryness matches the -a/-d parameter, why not use the -f test to decide? That is, if the user give a file name do the file, if he gives a directory name do the directory! No need to have the first parameter in your script.

   if (-f $dir) { changesize ($dir ...    }
   else {  # do the loop
[download]

Nice trick on the grep -f to get files. Why not do the same thing for the .jpg extension, instead of using the if statement?

my @onlyFiles= grep {{-f "$dir/$_" and /jpe?g$/} readdir(DIR);
[download]

---

I hope that's helpful. I went over this in detail since you said you are learning, and learning by jumping in and doing is the way I like best! Keep at it.

—John

---

---

• Are you posting in the right place? Check out Where do I post X? to know for sure.
• Posts may use any of the Perl Monks Approved HTML tags. Currently these include the following:

  <code> <a> <b> <big> <blockquote> <br /> <dd> <dl> <dt> <em> <font> <h1> <h2> <h3> <h4> <h5> <h6> <hr /> <i> <li> <nbsp> <ol> <p> <small> <strike> <strong> <sub> <sup> <table> <td> <th> <tr> <tt> <u> <ul>

• Snippets of code should be wrapped in <code> tags not <pre> tags. In fact, <pre> tags should generally be avoided. If they must be used, extreme care should be taken to ensure that their contents do not have long lines (<70 chars), in order to prevent horizontal scrolling (and possible janitor intervention).
• Want more info? How to link or How to display code and escape characters are good places to start.